Update Hello and Swap to authenticated calls #218
Conversation
📝 WalkthroughWalkthroughThis pull request includes significant changes across multiple files, primarily focusing on the removal and modification of workflows, scripts, and smart contracts related to the ZetaChain ecosystem. Key alterations include the deletion of the GitHub Actions workflow for publishing NPM packages, updates to Slither and test workflows, the introduction of new Solidity contracts, and the restructuring of deployment scripts. Additionally, several tasks for interacting with smart contracts have been added or modified, enhancing the overall functionality and error handling of the contracts. Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@fadeev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 17 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| function deposit( | ||
| address receiver, | ||
| uint256 amount, | ||
| address asset, | ||
| RevertOptions memory revertOptions | ||
| ) external { | ||
| IERC20(asset).transferFrom(msg.sender, address(this), amount); | ||
| IERC20(asset).approve(address(gateway), amount); | ||
| gateway.deposit(receiver, amount, asset, revertOptions); | ||
| } |
Check warning
Code scanning / Slither
Unused return Medium
| function depositAndCall( | ||
| address receiver, | ||
| uint256 amount, | ||
| address asset, | ||
| bytes calldata message, | ||
| RevertOptions memory revertOptions | ||
| ) external { | ||
| IERC20(asset).transferFrom(msg.sender, address(this), amount); | ||
| IERC20(asset).approve(address(gateway), amount); | ||
| gateway.depositAndCall(receiver, amount, asset, message, revertOptions); | ||
| } |
Check warning
Code scanning / Slither
Unused return Medium
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
examples/token/contracts/Universal.sol (1)
Add nonReentrant modifier to protect against reentrancy attacks
The contract makes external calls to untrusted contracts in multiple functions without reentrancy protection:
transferCrossChain(): Makes external calls to IZRC20 and gatewayonCall(): Makes external calls to SwapHelperLib and gateway- State changes occur after these external calls
Required changes:
- Import ReentrancyGuard from OpenZeppelin
- Add nonReentrant modifier to
transferCrossChain()andonCall()- Consider following checks-effects-interactions pattern
🔗 Analysis chain
Line range hint
1-152: Verify reentrancy protection for external callsThe contract makes multiple external calls to Uniswap and the gateway. Let's verify the safety of these interactions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential reentrancy vulnerabilities in external calls # Look for state changes after external calls rg -A 5 "call\(" . rg -A 5 "transfer" . rg -A 5 "transferFrom" . # Check for nonReentrant modifiers ast-grep --pattern 'modifier nonReentrant'Length of output: 23631
examples/nft/contracts/Universal.sol (1)
Line range hint
116-124: Enhance token ID generation uniqueness.The current token ID generation using block number and contract address could potentially create collisions in high-frequency minting scenarios. Consider using a more robust approach:
-uint256 hash = uint256( - keccak256( - abi.encodePacked(address(this), block.number, _nextTokenId++) - ) -); -uint256 tokenId = hash & 0x00FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF; +uint256 tokenId = uint256( + keccak256( + abi.encode( + address(this), + block.timestamp, + block.number, + msg.sender, + _nextTokenId++ + ) + ) +);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
examples/nft/contracts/Universal.sol(5 hunks)examples/token/contracts/Universal.sol(5 hunks)
🔇 Additional comments (9)
examples/token/contracts/Universal.sol (8)
16-16: LGTM: Appropriate use of immutable state variable
The immutable declaration for uniswapRouter ensures the router address cannot be changed post-deployment, which is a security best practice.
29-29: LGTM: Improved error handling with custom errors
The use of custom errors instead of require statements is more gas-efficient and provides better error reporting.
38-45: Skip: Issue already reported
The missing zero address validation for uniswapRouterAddress has already been reported in previous reviews and remains valid.
72-72: LGTM: Enhanced message encoding with sender tracking
The addition of msg.sender to both the message and revert options improves cross-chain transaction traceability.
Also applies to: 80-80
106-111: LGTM: Clear and structured message decoding
The message decoding is well-structured and matches the encoding format used in transferCrossChain.
118-125: Skip: Slippage protection issue already reported
The missing slippage protection in swapExactTokensForTokens has already been reported in previous reviews and remains valid.
126-138: LGTM: Proper handling of cross-chain token transfers
The implementation correctly:
- Manages gas fees
- Formats messages consistently
- Handles token approvals
145-147: LGTM: Consistent revert message handling
The revert message decoding properly aligns with the encoding format used in the revert options.
examples/nft/contracts/Universal.sol (1)
26-26: Consider upgrade implications of immutable router.
While the immutable uniswapRouter provides gas optimization and security benefits, it may limit future upgrades if Uniswap deploys new router versions. Consider implementing an upgradeable router pattern if protocol evolution is anticipated.
Also applies to: 36-36, 48-55, 58-58
| bytes memory message = abi.encode( | ||
| receiver, | ||
| tokenId, | ||
| uri, | ||
| 0, | ||
| msg.sender | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add message format versioning for cross-chain compatibility.
The message encoding format has changed. Consider adding a version identifier to support backward compatibility and future format changes:
bytes memory message = abi.encode(
+ uint8(1), // format version
receiver,
tokenId,
uri,
0,
msg.sender
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bytes memory message = abi.encode( | |
| receiver, | |
| tokenId, | |
| uri, | |
| 0, | |
| msg.sender | |
| ); | |
| bytes memory message = abi.encode( | |
| uint8(1), // format version | |
| receiver, | |
| tokenId, | |
| uri, | |
| 0, | |
| msg.sender | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
examples/swap/contracts/Swap.sol (3)
Line range hint
34-52: Add input validation for message parsingThe message parsing logic lacks validation for message length and content, which could lead to out-of-bounds reads or invalid data processing.
Consider adding these validations:
) external onlyGateway { Params memory params = Params({target: address(0), to: bytes("")}); if (context.chainID == BITCOIN) { + require(message.length >= 40, "Invalid Bitcoin message length"); params.target = BytesHelperLib.bytesToAddress(message, 0); params.to = abi.encodePacked( BytesHelperLib.bytesToAddress(message, 20) ); } else { + require(message.length > 0, "Empty message"); (address targetToken, bytes memory recipient) = abi.decode( message, (address, bytes) ); + require(recipient.length > 0, "Empty recipient"); params.target = targetToken; params.to = recipient; }
Line range hint
84-89: Add slippage protection to prevent sandwich attacksThe swap operation uses a minimum output amount of 0, which makes it vulnerable to sandwich attacks.
Consider adding a minimum output parameter:
- 0 + SwapHelperLib.calculateMinimumOutputAmount(outputAmount)
Line range hint
91-97: Optimize token approvalsMultiple token approvals could be combined to save gas.
Consider consolidating the approvals:
- if (gasZRC20 == targetToken) { - IZRC20(gasZRC20).approve(address(gateway), outputAmount + gasFee); - } else { - IZRC20(gasZRC20).approve(address(gateway), gasFee); - IZRC20(targetToken).approve(address(gateway), outputAmount); - } + uint256 gasTokenAmount = gasZRC20 == targetToken ? outputAmount + gasFee : gasFee; + IZRC20(gasZRC20).approve(address(gateway), gasTokenAmount); + if (gasZRC20 != targetToken) { + IZRC20(targetToken).approve(address(gateway), outputAmount); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
examples/swap/contracts/Swap.sol(6 hunks)
🧰 Additional context used
🪛 GitHub Check: Slither
examples/swap/contracts/Swap.sol
[notice] 24-24: Missing zero address validation
Swap.constructor(address,address).uniswapRouterAddress (contracts/Swap.sol#24) lacks a zero-check on :
- uniswapRouter = uniswapRouterAddress (contracts/Swap.sol#25)
| @@ -12,7 +12,7 @@ | |||
| import {GatewayZEVM} from "@zetachain/protocol-contracts/contracts/zevm/GatewayZEVM.sol"; | |||
|
|
|||
| contract Swap is UniversalContract { | |||
| SystemContract public systemContract; | |||
| address public immutable uniswapRouter; | |||
There was a problem hiding this comment.
Add critical address validations in constructor
The constructor lacks zero-address validation for critical components. An invalid router or gateway address would render the contract unusable.
Apply this diff to add the necessary checks:
constructor(address payable gatewayAddress, address uniswapRouterAddress) {
+ require(gatewayAddress != address(0), "Invalid gateway address");
+ require(uniswapRouterAddress != address(0), "Invalid router address");
uniswapRouter = uniswapRouterAddress;
gateway = GatewayZEVM(gatewayAddress);
}Also applies to: 24-26
| RevertContext calldata revertContext | ||
| ) external override onlyGateway {} | ||
| ) external onlyGateway {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement revert handling logic
The empty onRevert implementation might miss critical error handling scenarios.
Consider implementing basic revert handling:
- ) external onlyGateway {}
+ ) external onlyGateway {
+ // Emit event for monitoring
+ emit RevertReceived(
+ revertContext.sender,
+ revertContext.targetZRC20,
+ revertContext.amount
+ );
+ // Additional revert handling logic
+ }
+
+ event RevertReceived(address indexed sender, address indexed token, uint256 amount);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
examples/nft/contracts/Connected.sol (1)
56-60: Consider implementing collision prevention in token ID generationWhile the current implementation provides good entropy, it doesn't prevent potential token ID collisions. Consider adding a collision check before minting.
uint256 tokenId = hash & 0x00FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF; +if (_exists(tokenId)) { + revert TokenIdCollision(tokenId); +} _safeMint(to, tokenId);Also applies to: 62-62
examples/nft/contracts/Universal.sol (1)
181-183: Enhance TokenTransferReverted event with failure context.Include the failed destination address in the event to help with debugging and monitoring.
-emit TokenTransferReverted(sender, tokenId, uri); +emit TokenTransferReverted(sender, tokenId, uri, context.destination);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
examples/nft/contracts/Connected.sol(4 hunks)examples/nft/contracts/Universal.sol(4 hunks)examples/swap/contracts/Swap.sol(5 hunks)examples/swap/contracts/SwapToAnyToken.sol(5 hunks)examples/token/contracts/Universal.sol(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/swap/contracts/Swap.sol
- examples/swap/contracts/SwapToAnyToken.sol
🧰 Additional context used
🪛 GitHub Check: Slither
examples/nft/contracts/Connected.sol
[notice] 119-119: Missing zero address validation
Connected.onCall(MessageContext,bytes).sender (contracts/Connected.sol#119) lacks a zero-check on :
- (success,None) = sender.call{value: gasAmount}() (contracts/Connected.sol#125)
🔇 Additional comments (11)
examples/token/contracts/Universal.sol (6)
16-16: LGTM: Immutable state variables properly declared
The use of immutable variables for uniswapRouter and gasLimit is optimal for gas efficiency and security.
Also applies to: 19-19
29-29: LGTM: Improved error handling with custom errors
The use of custom errors instead of require statements is more gas efficient and provides better error messaging.
38-49: LGTM: Constructor properly initialized with validations
The constructor correctly initializes all immutable variables with proper validation checks.
75-75: LGTM: Enhanced cross-chain message traceability
The addition of sender address in message encoding improves transaction traceability.
Also applies to: 83-83
148-150: LGTM: Consistent message decoding
The message decoding order matches the encoding pattern used in other functions.
121-128:
Add slippage protection to prevent sandwich attacks
Setting minAmountOut to 0 provides no slippage protection, making transactions vulnerable to sandwich attacks and price manipulation.
examples/nft/contracts/Connected.sol (4)
23-23: LGTM: Well-structured state variable and error declarations
The immutable gasLimit and new error types enhance the contract's safety and efficiency.
Also applies to: 27-28
45-46: LGTM: Robust constructor implementation with proper validation
The gas parameter validation and assignment to immutable gasLimit enhance contract security.
Also applies to: 50-51
78-83: LGTM: Well-structured cross-chain transfer implementation
The message encoding and gateway calls are properly implemented with appropriate RevertOptions configuration.
Also applies to: 86-90, 95-101
133-135: LGTM: Correct message decoding implementation
The message decoding properly handles the updated message format.
examples/nft/contracts/Universal.sol (1)
Line range hint 63-68: Update CounterpartySet event parameters for consistency.
The function parameter type has been changed from bytes to address, but the corresponding event in Events.sol might still be using bytes. Ensure the event definition is updated to maintain consistency.
| uint256 out = SwapHelperLib.swapExactTokensForTokens( | ||
| uniswapRouter, | ||
| zrc20, | ||
| amount, | ||
| destination, | ||
| 0 | ||
| ); | ||
| IZRC20(destination).approve(address(gateway), gasFee); | ||
| gateway.call( | ||
| counterparty[destination], | ||
| IZRC20(destination).approve(address(gateway), out); | ||
| gateway.withdrawAndCall( | ||
| abi.encodePacked(counterparty[destination]), | ||
| out - gasFee, | ||
| destination, | ||
| abi.encode(receiver, tokenAmount), | ||
| abi.encode(receiver, tokenAmount, out - gasFee, sender), | ||
| CallOptions(gasLimit, false), | ||
| RevertOptions(address(0), false, address(0), "", 0) | ||
| RevertOptions( | ||
| address(this), | ||
| true, | ||
| address(0), | ||
| abi.encode(tokenAmount, sender), | ||
| 0 | ||
| ) |
There was a problem hiding this comment.
💡 Codebase verification
Critical: Missing reentrancy protection in external calls
The contract lacks ReentrancyGuard implementation while making multiple external calls that could be vulnerable to reentrancy attacks:
- In
onCall(): Sequential external calls toswapExactTokensForTokens(),approve(), andwithdrawAndCall()with state changes - In
transferCrossChain(): Sequential external calls totransferFrom(),approve(), andcall()
Recommended fixes:
- Import OpenZeppelin's ReentrancyGuard
- Add inheritance:
contract Universal is ERC20, Ownable2Step, UniversalContract, Events, ReentrancyGuard - Add
nonReentrantmodifier toonCall()andtransferCrossChain()
🔗 Analysis chain
Verify reentrancy protection in external calls
The contract makes external calls to swap tokens and withdraw. Let's verify the reentrancy protection.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for reentrancy vulnerabilities in external calls
# Search for ReentrancyGuard usage
rg -l "ReentrancyGuard"
# Check for potential reentrancy vulnerabilities in external calls
ast-grep --pattern 'function $FNAME($$$) {
$$$
$EXTERNAL_CALL($$$);
$STATE_CHANGE($$$);
$$$
}'
Length of output: 128
Script:
#!/bin/bash
# Let's check the contract implementation more thoroughly
# First, check if ReentrancyGuard is imported
rg -A 2 "import.*ReentrancyGuard" examples/token/contracts/
# Check contract inheritance
rg -A 5 "contract\s+Universal\s+is\s+" examples/token/contracts/
# Look for nonReentrant modifier usage
rg "nonReentrant" examples/token/contracts/
# Check the full contract implementation for state changes after external calls
cat examples/token/contracts/Universal.sol
Length of output: 5870
| ( | ||
| address receiver, | ||
| uint256 tokenId, | ||
| string memory uri, | ||
| uint256 gasAmount, | ||
| address sender | ||
| ) = abi.decode(message, (address, uint256, string, uint256, address)); |
There was a problem hiding this comment.
Enhance security measures for gas token transfers
The current implementation needs additional safety checks:
- Add zero-address validation for sender
- Use OpenZeppelin's Address.sendValue for safer ETH transfers
+import "@openzeppelin/contracts/utils/Address.sol";
contract Connected is
ERC721,
ERC721Enumerable,
ERC721URIStorage,
Ownable2Step,
Events
{
+ using Address for address payable;
// ... in onCall function
) = abi.decode(message, (address, uint256, string, uint256, address));
+ if (sender == address(0)) revert InvalidAddress();
_safeMint(receiver, tokenId);
_setTokenURI(tokenId, uri);
if (gasAmount > 0) {
- (bool success, ) = sender.call{value: gasAmount}("");
- if (!success) revert GasTokenTransferFailed();
+ payable(sender).sendValue(gasAmount);
}Also applies to: 124-127
🧰 Tools
🪛 GitHub Check: Slither
[notice] 119-119: Missing zero address validation
Connected.onCall(MessageContext,bytes).sender (contracts/Connected.sol#119) lacks a zero-check on :
- (success,None) = sender.call{value: gasAmount}() (contracts/Connected.sol#125)
examples/nft/contracts/Universal.sol
Outdated
| gateway.withdrawAndCall( | ||
| abi.encodePacked(counterparty[destination]), | ||
| out - gasFee, | ||
| destination, | ||
| abi.encode(sender, tokenId, uri), | ||
| abi.encode(receiver, tokenId, uri, out - gasFee, sender), | ||
| CallOptions(gasLimit, false), | ||
| RevertOptions(address(0), false, address(0), "", 0) | ||
| RevertOptions( | ||
| address(this), | ||
| true, | ||
| address(0), | ||
| abi.encode(tokenId, uri, sender), | ||
| 0 | ||
| ) |
There was a problem hiding this comment.
Add slippage protection for cross-chain transfers.
The current implementation doesn't protect against price slippage during cross-chain transfers. Consider adding a minimum output amount parameter.
gateway.withdrawAndCall(
abi.encodePacked(counterparty[destination]),
out - gasFee,
destination,
- abi.encode(receiver, tokenId, uri, out - gasFee, sender),
+ abi.encode(receiver, tokenId, uri, out - gasFee, sender, block.timestamp + 1800), // 30 min deadline
CallOptions(gasLimit, false),
RevertOptions(
address(this),
true,
address(0),
- abi.encode(tokenId, uri, sender),
+ abi.encode(tokenId, uri, sender, block.timestamp + 1800),
0
)
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| gateway.withdrawAndCall( | |
| abi.encodePacked(counterparty[destination]), | |
| out - gasFee, | |
| destination, | |
| abi.encode(sender, tokenId, uri), | |
| abi.encode(receiver, tokenId, uri, out - gasFee, sender), | |
| CallOptions(gasLimit, false), | |
| RevertOptions(address(0), false, address(0), "", 0) | |
| RevertOptions( | |
| address(this), | |
| true, | |
| address(0), | |
| abi.encode(tokenId, uri, sender), | |
| 0 | |
| ) | |
| gateway.withdrawAndCall( | |
| abi.encodePacked(counterparty[destination]), | |
| out - gasFee, | |
| destination, | |
| abi.encode(receiver, tokenId, uri, out - gasFee, sender, block.timestamp + 1800), // 30 min deadline | |
| CallOptions(gasLimit, false), | |
| RevertOptions( | |
| address(this), | |
| true, | |
| address(0), | |
| abi.encode(tokenId, uri, sender, block.timestamp + 1800), | |
| 0 | |
| ) |
examples/nft/contracts/Universal.sol
Outdated
| if (context.sender != counterparty[zrc20]) revert Unauthorized(); | ||
|
|
||
| ( | ||
| address destination, | ||
| address sender, | ||
| address receiver, | ||
| uint256 tokenId, | ||
| string memory uri | ||
| ) = abi.decode(message, (address, address, uint256, string)); | ||
| string memory uri, | ||
| address sender | ||
| ) = abi.decode(message, (address, address, uint256, string, address)); |
There was a problem hiding this comment.
Add validation for decoded receiver address.
The decoded receiver address should be validated to prevent minting to zero address.
) = abi.decode(message, (address, address, uint256, string, address));
+if (receiver == address(0)) revert InvalidAddress();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (context.sender != counterparty[zrc20]) revert Unauthorized(); | |
| ( | |
| address destination, | |
| address sender, | |
| address receiver, | |
| uint256 tokenId, | |
| string memory uri | |
| ) = abi.decode(message, (address, address, uint256, string)); | |
| string memory uri, | |
| address sender | |
| ) = abi.decode(message, (address, address, uint256, string, address)); | |
| if (context.sender != counterparty[zrc20]) revert Unauthorized(); | |
| ( | |
| address destination, | |
| address receiver, | |
| uint256 tokenId, | |
| string memory uri, | |
| address sender | |
| ) = abi.decode(message, (address, address, uint256, string, address)); | |
| if (receiver == address(0)) revert InvalidAddress(); |
| uint256 gas, | ||
| address uniswapRouterAddress | ||
| ) ERC721(name, symbol) Ownable(owner) { | ||
| if (gatewayAddress == address(0) || owner == address(0)) | ||
| revert InvalidAddress(); | ||
| if ( | ||
| gatewayAddress == address(0) || | ||
| owner == address(0) || | ||
| uniswapRouterAddress == address(0) | ||
| ) revert InvalidAddress(); | ||
| if (gas == 0) revert InvalidGasLimit(); | ||
| gateway = GatewayZEVM(gatewayAddress); | ||
| uniswapRouter = uniswapRouterAddress; | ||
| gasLimit = gas; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add upper bound validation for gas limit.
While the constructor validates that gas limit is non-zero, it should also check for an upper bound to prevent excessive gas consumption in cross-chain calls.
if (gas == 0) revert InvalidGasLimit();
+if (gas > 1000000) revert InvalidGasLimit(); // adjust threshold as needed
gateway = GatewayZEVM(gatewayAddress);Committable suggestion skipped: line range outside the PR's diff.
|
@andresaiello please, review. |
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range and nitpick comments (9)
examples/nft/hardhat.config.ts (2)
Line range hint
1-2: Remove duplicate import statementThe
"./tasks/deploy"module is imported twice, which is redundant.Apply this diff to fix the duplicate import:
import "./tasks/deploy"; -import "./tasks/deploy"; import "./tasks/mint";
Line range hint
18-18: Fix invalid Solidity versionThe specified Solidity version "0.8.26" is invalid. The latest stable version is 0.8.24.
Apply this diff to use the correct version:
- solidity: "0.8.26", + solidity: "0.8.24",examples/nft/contracts/shared/Events.sol (1)
5-6: Consider adding NatSpec documentation for events.The events would benefit from NatSpec documentation explaining their purpose and parameters.
Consider adding documentation like this:
+ /// @notice Emitted when the Universal contract address is set + /// @param universalAddress The address of the Universal contract event SetUniversal(address indexed universalAddress); + /// @notice Emitted when a Connected contract is associated with a ZRC20 token + /// @param zrc20 The address of the ZRC20 token + /// @param contractAddress The address of the Connected contract event SetConnected(address indexed zrc20, address contractAddress);examples/token/tasks/connectedSetUniversal.ts (1)
35-35: Add parameter validation for contract addressesThe task should validate that the provided addresses are valid Ethereum addresses.
Consider adding validation before the main execution:
const validateAddress = (address: string): boolean => { return hre.ethers.utils.isAddress(address); }; task("connected-set-universal", "Sets the universal contract address", async (args, hre) => { if (!validateAddress(args.contract) || !validateAddress(args.universal)) { throw new Error("Invalid Ethereum address provided"); } await main(args, hre); })Also applies to: 37-37
examples/nft/tasks/connectedSetUniversal.ts (1)
Line range hint
31-33: Fix inconsistent variable reference in console logThe console log uses
args.universalContractbut the correct variable isargs.universal, which could cause undefined values in the output.Apply this diff to fix the inconsistency:
console.log(`🚀 Successfully set the universal contract. 📜 Contract address: ${args.contract} -🔗 Universal contract address: ${args.universalContract} +🔗 Universal contract address: ${args.universal} 🔗 Transaction hash: ${tx.hash}`);examples/nft/tasks/universalSetConnected.ts (1)
Line range hint
29-34: Fix inconsistent variable name in console log messageThe console log message uses
args.contractAddressbut the parameter is namedargs.connected.Apply this fix:
console.log(`🚀 Successfully set the connected contract. 📜 Contract address: ${args.contract} 🔗 ZRC20 address: ${args.zrc20} -🔗 Connected contract address: ${args.contractAddress} +🔗 Connected contract address: ${args.connected} 🔗 Transaction hash: ${tx.hash}`);examples/nft/scripts/localnet.sh (1)
4-5: Consider making debug mode conditionalWhile
set -xis valuable for debugging, it should be conditionally enabled to prevent exposing sensitive information in production logs.-set -x +if [ "${DEBUG:-false}" = "true" ]; then + set -x +fiexamples/nft/contracts/Connected.sol (1)
56-62: Consider using a more unique source of randomness for tokenId generationThe current implementation uses
block.numberwhich could lead to tokenId collisions if multiple mints occur in the same block. Consider using additional entropy sources or OpenZeppelin'sCountersutility.- uint256 hash = uint256( - keccak256( - abi.encodePacked(address(this), block.number, _nextTokenId++) - ) - ); + uint256 hash = uint256( + keccak256( + abi.encodePacked( + address(this), + block.number, + block.timestamp, + msg.sender, + _nextTokenId++ + ) + ) + );examples/nft/contracts/Universal.sol (1)
86-92: Add message format versioning for future compatibility.The message encoding format should include a version identifier to support future format changes.
bytes memory message = abi.encode( + uint8(1), // format version receiver, tokenId, uri, 0, msg.sender );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
examples/token/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
examples/nft/contracts/Connected.sol(4 hunks)examples/nft/contracts/Universal.sol(4 hunks)examples/nft/contracts/shared/Events.sol(1 hunks)examples/nft/hardhat.config.ts(1 hunks)examples/nft/scripts/localnet.sh(3 hunks)examples/nft/tasks/connectedSetUniversal.ts(2 hunks)examples/nft/tasks/universalSetConnected.ts(2 hunks)examples/token/contracts/Connected.sol(2 hunks)examples/token/contracts/Universal.sol(4 hunks)examples/token/contracts/shared/Events.sol(1 hunks)examples/token/hardhat.config.ts(1 hunks)examples/token/package.json(1 hunks)examples/token/scripts/localnet.sh(4 hunks)examples/token/tasks/connectedSetUniversal.ts(2 hunks)examples/token/tasks/universalSetConnected.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/token/hardhat.config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/token/contracts/shared/Events.sol
- examples/token/package.json
🧰 Additional context used
🪛 GitHub Check: Slither
examples/token/contracts/Connected.sol
[notice] 94-94: Missing zero address validation
Connected.onCall(MessageContext,bytes).sender (contracts/Connected.sol#94) lacks a zero-check on :
- (success,None) = sender.call{value: amount}() (contracts/Connected.sol#98)
🔇 Additional comments (26)
examples/nft/hardhat.config.ts (1)
5-6: LGTM! Task imports are well-structured
The renaming from "Counterparty" to "Connected/Universal" maintains a clear and consistent naming convention, improving code readability.
examples/nft/contracts/shared/Events.sol (1)
5-6: Event signature changes look good, verify consumers.
The new event signatures are well-structured with appropriate parameter indexing. The shift from counterparty to universal/connected terminology aligns with the architectural changes.
Let's verify the event consumers in the codebase:
✅ Verification successful
Let me try a different search strategy to verify the event consumers.
Let me try again without specifying the file type:
Event signature changes are properly implemented across the codebase
The verification confirms:
- Both events are consistently used in both NFT and token implementations
- No remnants of old event patterns (SetCounterparty/CounterpartySet)
- Events are emitted in appropriate contexts:
- SetUniversal in Connected.sol contracts
- SetConnected in Universal.sol contracts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all contracts that might be affected by the event changes
# Search for old event names to ensure complete migration
echo "Checking for any remaining old event references..."
rg -l "SetCounterparty|CounterpartySet"
# Search for new event implementations
echo "Checking new event implementations..."
ast-grep --pattern 'emit Set(Universal|Connected)($$$)'
Length of output: 295
Script:
#!/bin/bash
# Search for files that import or inherit from Events.sol
echo "Files importing or inheriting Events:"
rg -l "import.*Events" --type sol
# Search for event emissions with context
echo -e "\nEvent emission patterns:"
rg "emit Set(Universal|Connected)" -A 2 --type sol
# Search for event listeners/handlers
echo -e "\nPotential event listeners:"
rg "(SetUniversal|SetConnected).*event" --type sol
Length of output: 444
Script:
#!/bin/bash
# Search for files that import or inherit from Events
echo "Files importing or inheriting Events:"
rg -l "import.*Events"
# Search for event emissions with context
echo -e "\nEvent emission patterns:"
rg "emit Set(Universal|Connected)" -A 2
# Search for event listeners/handlers
echo -e "\nPotential event listeners:"
rg "(SetUniversal|SetConnected).*event"
# Check for any remaining old event patterns
echo -e "\nOld event patterns:"
rg "SetCounterparty|CounterpartySet"
Length of output: 1256
examples/token/tasks/connectedSetUniversal.ts (1)
Line range hint 18-37: Verify consistent renaming across the codebase
Let's ensure all references to "counterparty" have been updated to "universal" consistently.
✅ Verification successful
Rebranding verification completed: No remaining 'counterparty' references found
The codebase has been thoroughly checked and shows consistent usage of "universal" terminology with no lingering references to "counterparty". All relevant files and interfaces correctly use the updated terminology.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of "counterparty" in the codebase
echo "Searching for remaining 'counterparty' references..."
rg -i "counterparty" --type ts
# Search for any potential inconsistencies in naming patterns
echo "Checking for consistent use of 'universal'..."
rg -i "setUniversal|getUniversal|universal" --type ts
Length of output: 4971
examples/nft/tasks/connectedSetUniversal.ts (2)
37-39: LGTM: Task definition is clear and consistent
The task name and parameter descriptions accurately reflect the new terminology and purpose.
18-18: Verify the contract interface update
The code calls setUniversal method on the Connected contract. Let's verify this method exists in the contract interface.
✅ Verification successful
Contract interface verification successful
The setUniversal method is properly defined in the Connected contract. The method:
- Takes an
address contractAddressparameter - Has
externalvisibility - Is protected with
onlyOwnermodifier - Includes zero-address validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Connected contract interface includes the setUniversal method
# Expected: Find the setUniversal method definition in the Connected contract
# Search for the Connected contract and its setUniversal method
ast-grep --pattern 'contract Connected {
$$$
function setUniversal($$$) {
$$$
}
$$$
}'
Length of output: 100
Script:
#!/bin/bash
# Let's try to find the contract file first
fd -e sol Connected
# Then let's search for any file containing 'Connected' and 'setUniversal'
rg -l "Connected.*setUniversal|setUniversal.*Connected"
# As a backup, let's look for any contract interface or implementation with setUniversal
rg "function setUniversal" -A 2
Length of output: 812
examples/token/tasks/universalSetConnected.ts (4)
33-34: LGTM! Clear and consistent console output
The console output is well-formatted and maintains consistency with the new terminology.
18-18: Verify contract interface update
The method call has been updated from setCounterparty to setConnected. Let's ensure the contract interface is consistently updated.
✅ Verification successful
Contract interface is correctly updated
The contract interface in Universal.sol has been properly updated with the setConnected method, and there are no remaining references to setCounterparty. The changes are consistent across both the task file and the contract implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old method name
rg "setCounterparty" --type ts --type sol
# Verify the new method exists in the contract
ast-grep --pattern 'function setConnected($$$) {$$$}'
Length of output: 125
Script:
#!/bin/bash
# Search for contract files and interface definitions
fd -e sol .
# Search for any remaining references to setCounterparty
rg "setCounterparty"
# Search for setConnected method in TypeScript files
rg "setConnected" --type ts -A 2
Length of output: 947
Script:
#!/bin/bash
# Check the contract interface in Universal.sol for the token example
cat examples/token/contracts/Universal.sol
# Check if there are any remaining references to counterparty in the contract
rg "counterparty" examples/token/contracts/Universal.sol -i
Length of output: 5210
38-41: Verify task registration in Hardhat config
The task name has been updated. Let's ensure this change is reflected in the Hardhat configuration and any importing files.
✅ Verification successful
The search results show that:
- The old task name "universal-set-counterparty" is not referenced anywhere in the codebase
- The new task "universalSetConnected" is correctly imported in multiple Hardhat config files
Let me verify if there are any other potential references to the old name that might have been missed:
Task name update is properly reflected across the codebase
The task name change from 'universal-set-counterparty' to 'universal-set-connected' has been correctly implemented. The new task is properly imported in the Hardhat configuration files, and there are no lingering references to the old task name in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for old task name references
rg "universal-set-counterparty" --type ts --type js
# Check Hardhat config files
fd "hardhat.config.ts" --exec cat {}
Length of output: 2930
Script:
#!/bin/bash
# Search for variations of the old task name
rg -i "counterparty" --type ts
rg -i "set.*counterparty" --type ts
# Check the task file content
cat examples/token/tasks/universalSetConnected.ts
Length of output: 1544
25-27: Verify JSON output consumers
The JSON output field has been renamed from 'counterpartyContractAddress' to 'connectedContractAddress'. This change might affect any scripts or systems consuming this output.
examples/nft/tasks/universalSetConnected.ts (2)
38-41: LGTM! Task definition is clear and consistent
The task name and parameter descriptions have been updated to use the new "connected" terminology consistently. The changes improve clarity and maintain a coherent naming convention.
18-18: Verify the contract interface implementation
The method name has been changed from setCounterparty to setConnected. Let's verify this change in the contract interface.
✅ Verification successful
Let's analyze the contract files and interfaces to verify the method name change. The previous pattern might have been too specific, so we're casting a wider net to find relevant contract definitions and implementations.
Contract interface matches the task implementation
The setConnected method is correctly implemented in the Universal contract with matching signature and parameters. The contract contains:
- A
connectedmapping to store ZRC20-to-contract address relationships - The
setConnectedfunction with correct parameters (address zrc20, address contractAddress) - Proper access control via
onlyOwnermodifier - Event emission for tracking changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the contract interface matches the new method name
# Search for the Universal contract interface
ast-grep --pattern 'contract Universal {
$$$
function setConnected($$$) {
$$$
}
$$$
}'
# Search for any remaining references to the old method name
rg 'setCounterparty'
Length of output: 121
Script:
#!/bin/bash
# Let's try a broader search for contract interfaces and implementations
# Search for contract files
fd -e sol
# Search for setConnected or setCounterparty method definitions
rg -l "function set[C|c]onnected|function set[C|c]ounterparty"
# Search for interface definitions
rg -l "interface I.*Universal"
# Look for any test files that might contain contract usage
fd -e test.ts -e spec.ts
Length of output: 639
Script:
#!/bin/bash
# Let's examine the Universal contract implementations specifically for the NFT example
# Check the NFT Universal contract implementation
cat examples/nft/contracts/Universal.sol
# Check the Connected contract as well since it's related
cat examples/nft/contracts/Connected.sol
# Look for any usage patterns of setConnected/setCounterparty in tasks
rg -A 2 -B 2 "setConnected|setCounterparty" examples/nft/tasks/
Length of output: 12777
examples/token/scripts/localnet.sh (1)
5-5: Previous review comment is still applicable
The existing review comment about enhancing network management robustness remains valid.
Also applies to: 74-74
examples/token/contracts/Universal.sol (7)
16-16: LGTM: Appropriate use of immutable state variables
The immutable declarations for uniswapRouter and gasLimit are well-designed, providing gas optimization and preventing post-deployment modifications.
Also applies to: 19-19
26-26: LGTM: Clear and well-structured mapping declaration
The connected mapping appropriately tracks the relationship between ZRC20 tokens and their corresponding contract addresses.
29-29: LGTM: Gas-efficient authorization check
The modifier uses a custom error for unauthorized access, which is more gas-efficient than string error messages.
38-49: LGTM: Robust parameter validation in constructor
The constructor properly validates all critical parameters including zero address checks and gas limit validation.
52-57: LGTM: Well-structured administrative function with event emission
The setConnected function properly restricts access and emits events for off-chain tracking.
147-149: LGTM: Clear and consistent message decoding
The message decoding structure properly extracts the required parameters for handling reverted transactions.
120-127:
Add slippage protection to token swaps
The swap operation uses 0 as minAmountOut, which provides no protection against price manipulation.
Consider implementing slippage protection:
uint256 out = SwapHelperLib.swapExactTokensForTokens(
uniswapRouter,
zrc20,
amount,
destination,
- 0
+ calculateMinAmountOut(amount, maxSlippage)
);Likely invalid or redundant comment.
examples/nft/contracts/Connected.sol (5)
22-28: LGTM! Good improvements to state variables and error handling
The changes enhance code clarity and gas efficiency:
- Renaming
counterpartytouniversalbetter reflects its purpose - Making
gasLimitimmutable optimizes gas usage - New error types follow best practices for custom errors
30-33: LGTM! Well-structured setter with proper validation
The function includes all necessary components:
- Zero address validation
- State update
- Event emission
44-51: LGTM! Constructor properly validates and initializes new gas parameter
Good implementation of the gas limit feature with proper validation.
78-101: LGTM! Well-structured cross-chain transfer implementation
The changes properly implement the cross-chain transfer mechanism with appropriate message encoding and revert options.
133-135: LGTM! Message decoding matches the encoding structure
The decoding structure properly aligns with the encoding in transferCrossChain.
examples/nft/contracts/Universal.sol (2)
51-55: LGTM: Robust address validation in constructor.
The constructor properly validates all critical addresses to prevent zero address initialization.
62-67: LGTM: Proper access control and event emission.
The setConnected function maintains proper access control with onlyOwner modifier and emits appropriate events.
| @@ -15,13 +15,13 @@ const main = async (args: any, hre: HardhatRuntimeEnvironment) => { | |||
| args.contract | |||
| ); | |||
|
|
|||
| const tx = await contract.setCounterparty(args.counterparty); | |||
| const tx = await contract.setUniversal(args.universal); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for contract interaction
The contract call should be wrapped in a try-catch block to handle potential transaction failures gracefully.
Consider applying this improvement:
- const tx = await contract.setUniversal(args.universal);
+ let tx;
+ try {
+ tx = await contract.setUniversal(args.universal);
+ await tx.wait(); // Wait for transaction confirmation
+ } catch (error) {
+ throw new Error(`Failed to set universal contract: ${error.message}`);
+ }Also applies to: 24-24
| echo -e "\n🔗 Setting universal and connected contracts..." | ||
| npx hardhat connected-set-universal --network localhost --contract "$CONTRACT_ETHEREUM" --universal "$CONTRACT_ZETACHAIN" --json &>/dev/null | ||
| npx hardhat connected-set-universal --network localhost --contract "$CONTRACT_BNB" --universal "$CONTRACT_ZETACHAIN" --json &>/dev/null | ||
| npx hardhat universal-set-connected --network localhost --contract "$CONTRACT_ZETACHAIN" --connected "$CONTRACT_ETHEREUM" --zrc20 "$ZRC20_ETHEREUM" --json &>/dev/null | ||
| npx hardhat universal-set-connected --network localhost --contract "$CONTRACT_ZETACHAIN" --connected "$CONTRACT_BNB" --zrc20 "$ZRC20_BNB" --json &>/dev/null |
There was a problem hiding this comment.
Improve error handling for contract relationship setup
The current implementation suppresses all output and lacks error handling for failed operations.
echo -e "\n🔗 Setting universal and connected contracts..."
-npx hardhat connected-set-universal --network localhost --contract "$CONTRACT_ETHEREUM" --universal "$CONTRACT_ZETACHAIN" --json &>/dev/null
+
+function set_contract_relationship() {
+ local cmd=$1
+ local output
+ output=$(eval "$cmd" 2>&1)
+ if [ $? -ne 0 ]; then
+ echo "Error: Failed to set contract relationship"
+ echo "$output"
+ exit 1
+ fi
+}
+
+set_contract_relationship "npx hardhat connected-set-universal --network localhost --contract \"$CONTRACT_ETHEREUM\" --universal \"$CONTRACT_ZETACHAIN\" --json"Apply similar error handling to other relationship setup commands.
Committable suggestion skipped: line range outside the PR's diff.
| SENDER=0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266 | ||
|
|
||
|
|
||
| CONTRACT_ZETACHAIN=$(npx hardhat deploy --network localhost --gas-limit 700000 --json | jq -r '.contractAddress') | ||
| CONTRACT_ZETACHAIN=$(npx hardhat deploy --network localhost --gateway "$GATEWAY_ZETACHAIN" --uniswap-router "$UNISWAP_ROUTER" --json | jq -r '.contractAddress') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate gateway and router parameters before deployment
The deployment command should verify that the gateway and router parameters are valid before proceeding.
+# Validate deployment parameters
+if [[ -z "$GATEWAY_ZETACHAIN" || -z "$UNISWAP_ROUTER" ]]; then
+ echo "Error: Missing required parameters for deployment"
+ echo "GATEWAY_ZETACHAIN: $GATEWAY_ZETACHAIN"
+ echo "UNISWAP_ROUTER: $UNISWAP_ROUTER"
+ exit 1
+fi
+
CONTRACT_ZETACHAIN=$(npx hardhat deploy --network localhost --gateway "$GATEWAY_ZETACHAIN" --uniswap-router "$UNISWAP_ROUTER" --json | jq -r '.contractAddress')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CONTRACT_ZETACHAIN=$(npx hardhat deploy --network localhost --gateway "$GATEWAY_ZETACHAIN" --uniswap-router "$UNISWAP_ROUTER" --json | jq -r '.contractAddress') | |
| # Validate deployment parameters | |
| if [[ -z "$GATEWAY_ZETACHAIN" || -z "$UNISWAP_ROUTER" ]]; then | |
| echo "Error: Missing required parameters for deployment" | |
| echo "GATEWAY_ZETACHAIN: $GATEWAY_ZETACHAIN" | |
| echo "UNISWAP_ROUTER: $UNISWAP_ROUTER" | |
| exit 1 | |
| fi | |
| CONTRACT_ZETACHAIN=$(npx hardhat deploy --network localhost --gateway "$GATEWAY_ZETACHAIN" --uniswap-router "$UNISWAP_ROUTER" --json | jq -r '.contractAddress') |
| GATEWAY_ZETACHAIN=$(jq -r '.addresses[] | select(.type=="gatewayZEVM" and .chain=="zetachain") | .address' localnet.json) | ||
| GATEWAY_ETHEREUM=$(jq -r '.addresses[] | select(.type=="gatewayEVM" and .chain=="ethereum") | .address' localnet.json) | ||
| GATEWAY_BNB=$(jq -r '.addresses[] | select(.type=="gatewayEVM" and .chain=="bnb") | .address' localnet.json) | ||
| UNISWAP_ROUTER=$(jq -r '.addresses[] | select(.type=="uniswapRouterInstance" and .chain=="zetachain") | .address' localnet.json) | ||
| SENDER=0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance JSON parsing error handling
The jq commands should handle cases where the JSON file is malformed or missing required entries.
-GATEWAY_ZETACHAIN=$(jq -r '.addresses[] | select(.type=="gatewayZEVM" and .chain=="zetachain") | .address' localnet.json)
+GATEWAY_ZETACHAIN=$(jq -r '.addresses[] | select(.type=="gatewayZEVM" and .chain=="zetachain") | .address' localnet.json 2>/dev/null)
+if [ -z "$GATEWAY_ZETACHAIN" ]; then
+ echo "Error: Failed to extract gatewayZEVM address for zetachain from localnet.json"
+ exit 1
+fiApply similar error handling to other address extractions.
Committable suggestion skipped: line range outside the PR's diff.
| address public universal; | ||
| uint256 public immutable gasLimit; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making universal immutable for better gas efficiency and security.
The universal address is a critical contract reference that should not change frequently. Making it immutable would improve gas efficiency and prevent potential security risks from unauthorized changes.
- address public universal;
+ address public immutable universal;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| address public universal; | |
| uint256 public immutable gasLimit; | |
| address public immutable universal; | |
| uint256 public immutable gasLimit; |
| if (gasAmount > 0) { | ||
| (bool success, ) = sender.call{value: gasAmount}(""); | ||
| if (!success) revert GasTokenTransferFailed(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using OpenZeppelin's Address.sendValue for safer ETH transfers
Replace the low-level call with OpenZeppelin's Address.sendValue for better security:
+import "@openzeppelin/contracts/utils/Address.sol";
contract Connected is
ERC721,
ERC721Enumerable,
ERC721URIStorage,
Ownable2Step,
Events
{
+ using Address for address payable;
// ... in onCall function
if (gasAmount > 0) {
- (bool success, ) = sender.call{value: gasAmount}("");
- if (!success) revert GasTokenTransferFailed();
+ payable(sender).sendValue(gasAmount);
}Committable suggestion skipped: line range outside the PR's diff.
| address receiver, | ||
| uint256 tokenId, | ||
| string memory uri | ||
| ) = abi.decode(message, (address, address, uint256, string)); | ||
| string memory uri, | ||
| address sender | ||
| ) = abi.decode(message, (address, address, uint256, string, address)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for decoded receiver address.
The decoded receiver address should be validated to prevent minting to zero address.
address receiver,
uint256 tokenId,
string memory uri,
address sender
) = abi.decode(message, (address, address, uint256, string, address));
+if (receiver == address(0)) revert InvalidAddress();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| address receiver, | |
| uint256 tokenId, | |
| string memory uri | |
| ) = abi.decode(message, (address, address, uint256, string)); | |
| string memory uri, | |
| address sender | |
| ) = abi.decode(message, (address, address, uint256, string, address)); | |
| address receiver, | |
| uint256 tokenId, | |
| string memory uri, | |
| address sender | |
| ) = abi.decode(message, (address, address, uint256, string, address)); | |
| if (receiver == address(0)) revert InvalidAddress(); |
| address public immutable uniswapRouter; | ||
| uint256 private _nextTokenId; | ||
| bool public isUniversal = true; | ||
| uint256 public gasLimit; | ||
| uint256 public immutable gasLimit; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation for gas limit upper bound.
While the contract includes immutable variables for uniswapRouter and gasLimit, consider adding an upper bound validation for gasLimit to prevent excessive gas consumption in cross-chain calls.
uint256 public immutable gasLimit;
+uint256 public constant MAX_GAS_LIMIT = 1000000; // Adjust based on network specificsThen in the constructor:
if (gas == 0) revert InvalidGasLimit();
+if (gas > MAX_GAS_LIMIT) revert InvalidGasLimit();Committable suggestion skipped: line range outside the PR's diff.
| uint256 out = SwapHelperLib.swapExactTokensForTokens( | ||
| uniswapRouter, | ||
| zrc20, | ||
| amount, | ||
| destination, | ||
| 0 | ||
| ); | ||
|
|
||
| IZRC20(destination).approve(address(gateway), gasFee); | ||
| gateway.call( | ||
| counterparty[destination], | ||
| IZRC20(destination).approve(address(gateway), out); |
There was a problem hiding this comment.
Add swap output validation and slippage protection.
The swap implementation needs output amount validation and slippage protection.
uint256 out = SwapHelperLib.swapExactTokensForTokens(
uniswapRouter,
zrc20,
amount,
destination,
- 0
+ amount * 95 / 100 // 5% max slippage
);
+require(out > gasFee, "Insufficient swap output");
+require(out >= amount * 95 / 100, "Excessive slippage");
IZRC20(destination).approve(address(gateway), out);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint256 out = SwapHelperLib.swapExactTokensForTokens( | |
| uniswapRouter, | |
| zrc20, | |
| amount, | |
| destination, | |
| 0 | |
| ); | |
| IZRC20(destination).approve(address(gateway), gasFee); | |
| gateway.call( | |
| counterparty[destination], | |
| IZRC20(destination).approve(address(gateway), out); | |
| uint256 out = SwapHelperLib.swapExactTokensForTokens( | |
| uniswapRouter, | |
| zrc20, | |
| amount, | |
| destination, | |
| amount * 95 / 100 // 5% max slippage | |
| ); | |
| require(out > gasFee, "Insufficient swap output"); | |
| require(out >= amount * 95 / 100, "Excessive slippage"); | |
| IZRC20(destination).approve(address(gateway), out); |
| gateway.withdrawAndCall( | ||
| abi.encodePacked(connected[destination]), | ||
| out - gasFee, | ||
| destination, | ||
| abi.encode(sender, tokenId, uri), | ||
| abi.encode(receiver, tokenId, uri, out - gasFee, sender), | ||
| CallOptions(gasLimit, false), | ||
| RevertOptions(address(0), false, address(0), "", 0) | ||
| RevertOptions( | ||
| address(this), | ||
| true, | ||
| address(0), | ||
| abi.encode(tokenId, uri, sender), | ||
| 0 | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add deadline protection for cross-chain transfers.
Cross-chain transfers should include a deadline to prevent pending transactions from being executed after significant delays.
gateway.withdrawAndCall(
abi.encodePacked(connected[destination]),
out - gasFee,
destination,
- abi.encode(receiver, tokenId, uri, out - gasFee, sender),
+ abi.encode(receiver, tokenId, uri, out - gasFee, sender, block.timestamp + 1800), // 30 min deadline
CallOptions(gasLimit, false),
RevertOptions(
address(this),
true,
address(0),
- abi.encode(tokenId, uri, sender),
+ abi.encode(tokenId, uri, sender, block.timestamp + 1800),
0
)
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| gateway.withdrawAndCall( | |
| abi.encodePacked(connected[destination]), | |
| out - gasFee, | |
| destination, | |
| abi.encode(sender, tokenId, uri), | |
| abi.encode(receiver, tokenId, uri, out - gasFee, sender), | |
| CallOptions(gasLimit, false), | |
| RevertOptions(address(0), false, address(0), "", 0) | |
| RevertOptions( | |
| address(this), | |
| true, | |
| address(0), | |
| abi.encode(tokenId, uri, sender), | |
| 0 | |
| ) | |
| gateway.withdrawAndCall( | |
| abi.encodePacked(connected[destination]), | |
| out - gasFee, | |
| destination, | |
| abi.encode(receiver, tokenId, uri, out - gasFee, sender, block.timestamp + 1800), // 30 min deadline | |
| CallOptions(gasLimit, false), | |
| RevertOptions( | |
| address(this), | |
| true, | |
| address(0), | |
| abi.encode(tokenId, uri, sender, block.timestamp + 1800), | |
| 0 | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (7)
examples/call/contracts/Connected.sol (2)
1-16: Add NatSpec documentation for better code clarity.The contract structure is well-organized with appropriate use of custom errors and events. Consider adding NatSpec documentation to describe the contract's purpose, functions, events, and errors.
// SPDX-License-Identifier: MIT pragma solidity 0.8.26; import {RevertContext} from "@zetachain/protocol-contracts/contracts/Revert.sol"; import "@zetachain/protocol-contracts/contracts/evm/GatewayEVM.sol"; +/// @title Connected +/// @notice A contract that handles cross-chain communication via ZetaChain gateway +/// @dev This contract enables deposits and calls between different chains contract Connected { GatewayEVM public immutable gateway; event RevertEvent(string, RevertContext); event HelloEvent(string, string); + /// @notice Thrown when an ERC20 transfer fails error TransferFailed(); + /// @notice Thrown when an ERC20 approval fails error ApprovalFailed(); + /// @notice Thrown when a caller is not the gateway error Unauthorized();
102-104: Add events to track received funds.The receive and fallback functions should emit events to track received funds for better transparency and debugging.
+ event ReceivedETH(address indexed sender, uint256 amount); + event FallbackCalled(address indexed sender, uint256 amount, bytes data); - receive() external payable {} + receive() external payable { + emit ReceivedETH(msg.sender, msg.value); + } - fallback() external payable {} + fallback() external payable { + emit FallbackCalled(msg.sender, msg.value, msg.data); + }examples/call/contracts/Universal.sol (2)
18-21: Maintain consistent if-revert style across the codebase.For better readability and consistency, use braces with the if-revert statement.
- if (msg.sender != address(gateway)) revert Unauthorized(); + if (msg.sender != address(gateway)) { + revert Unauthorized(); + }
101-115: Consider enhancing event logging for better traceability.While the current implementation is secure, consider adding more context to events such as timestamps, amounts, and addresses for better transaction tracing and debugging.
- event HelloEvent(string, string); - event RevertEvent(string, RevertContext); + event HelloEvent(string action, string name, uint256 timestamp, address indexed sender); + event RevertEvent(string action, RevertContext context, uint256 timestamp, address indexed sender); function onCall(...) { string memory name = abi.decode(message, (string)); - emit HelloEvent("Hello on ZetaChain", name); + emit HelloEvent("Hello on ZetaChain", name, block.timestamp, context.sender); } function onRevert(...) { - emit RevertEvent("Revert on ZetaChain", revertContext); + emit RevertEvent("Revert on ZetaChain", revertContext, block.timestamp, revertContext.sender); }examples/hello/contracts/Universal.sol (3)
9-9: Name event parameters for improved clarity.Consider specifying parameter names in the
HelloEventto enhance readability and maintainability.Apply this diff:
-event HelloEvent(string, string); +event HelloEvent(string greeting, string name);
17-17: Useaddressinstead ofaddress payableforgatewayAddressif not handling Ether transfers.If the contract does not send Ether to
gatewayAddress,addresssuffices and avoids unnecessary use ofpayable.Apply this diff:
-constructor(address payable gatewayAddress) { +constructor(address gatewayAddress) {
6-30: Add NatSpec documentation to the contract and its functions.Including NatSpec comments enhances code understanding and aids in the generation of documentation.
Example for the contract:
+/// @title Universal Contract +/// @notice A contract that handles cross-chain calls via ZetaChain. contract Universal is UniversalContract {Example for the
onCallfunction:+ /** + * @notice Handles incoming cross-chain calls from the gateway. + * @param context The message context provided by the gateway. + * @param zrc20 The address of the ZRC20 token used. + * @param amount The amount of tokens sent. + * @param message The encoded message data sent. + */ function onCall(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
examples/call/contracts/Connected.sol(1 hunks)examples/call/contracts/Universal.sol(2 hunks)examples/hello/contracts/Universal.sol(1 hunks)examples/swap/contracts/Swap.sol(5 hunks)examples/swap/contracts/SwapToAnyToken.sol(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/swap/contracts/Swap.sol
- examples/swap/contracts/SwapToAnyToken.sol
🧰 Additional context used
🪛 GitHub Check: Slither
examples/call/contracts/Universal.sol
[warning] 43-65: Unused return
Universal.withdraw(bytes,uint256,address,RevertOptions) (contracts/Universal.sol#43-65) ignores return value by IZRC20(gasZRC20).approve(address(gateway),gasFee) (contracts/Universal.sol#62)
[warning] 43-65: Unused return
Universal.withdraw(bytes,uint256,address,RevertOptions) (contracts/Universal.sol#43-65) ignores return value by IZRC20(zrc20).approve(address(gateway),target) (contracts/Universal.sol#53)
🔇 Additional comments (2)
examples/call/contracts/Connected.sol (1)
17-20: LGTM! Access control is properly implemented.
The modifier correctly restricts access to the gateway address using a custom error.
examples/call/contracts/Universal.sol (1)
9-16: LGTM! Well-structured contract declaration with clear error definitions.
The contract name change to Universal better reflects its purpose, and the error types are appropriately defined for specific failure scenarios.
| constructor(address payable gatewayAddress) { | ||
| gateway = GatewayEVM(gatewayAddress); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add zero address validation in constructor.
The constructor should validate that the gateway address is not zero to prevent deployment with an invalid gateway.
constructor(address payable gatewayAddress) {
+ if (gatewayAddress == address(0)) revert("Zero address");
gateway = GatewayEVM(gatewayAddress);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor(address payable gatewayAddress) { | |
| gateway = GatewayEVM(gatewayAddress); | |
| } | |
| constructor(address payable gatewayAddress) { | |
| if (gatewayAddress == address(0)) revert("Zero address"); | |
| gateway = GatewayEVM(gatewayAddress); | |
| } |
| function deposit( | ||
| address receiver, | ||
| uint256 amount, | ||
| address asset, | ||
| RevertOptions memory revertOptions | ||
| ) external { | ||
| if (!IERC20(asset).transferFrom(msg.sender, address(this), amount)) { | ||
| revert TransferFailed(); | ||
| } | ||
| if (!IERC20(asset).approve(address(gateway), amount)) { | ||
| revert ApprovalFailed(); | ||
| } | ||
| gateway.deposit(receiver, amount, asset, revertOptions); | ||
| } | ||
|
|
||
| function depositAndCall( | ||
| address receiver, | ||
| uint256 amount, | ||
| address asset, | ||
| bytes calldata message, | ||
| RevertOptions memory revertOptions | ||
| ) external { | ||
| if (!IERC20(asset).transferFrom(msg.sender, address(this), amount)) { | ||
| revert TransferFailed(); | ||
| } | ||
| if (!IERC20(asset).approve(address(gateway), amount)) { | ||
| revert ApprovalFailed(); | ||
| } | ||
| gateway.depositAndCall(receiver, amount, asset, message, revertOptions); | ||
| } |
There was a problem hiding this comment.
Use SafeERC20 for token operations.
The current implementation uses unsafe ERC20 operations. Some tokens (like USDT) don't follow the standard return value pattern, which could cause the transfers to fail silently.
+import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
+import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
contract Connected {
+ using SafeERC20 for IERC20;
// ...
function deposit(
address receiver,
uint256 amount,
address asset,
RevertOptions memory revertOptions
) external {
- if (!IERC20(asset).transferFrom(msg.sender, address(this), amount)) {
- revert TransferFailed();
- }
- if (!IERC20(asset).approve(address(gateway), amount)) {
- revert ApprovalFailed();
- }
+ IERC20(asset).safeTransferFrom(msg.sender, address(this), amount);
+ IERC20(asset).safeApprove(address(gateway), amount);
gateway.deposit(receiver, amount, asset, revertOptions);
}Apply similar changes to the depositAndCall function.
Committable suggestion skipped: line range outside the PR's diff.
| function onCall( | ||
| MessageContext calldata context, | ||
| bytes calldata message | ||
| ) external payable onlyGateway returns (bytes4) { | ||
| emit HelloEvent("Hello on EVM from onCall()", "hey"); | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Return value should be IMessageDispatcher.onCall.selector
The empty bytes4 return value in the onCall function is incorrect. Based on the codebase analysis, this function is a callback from the gateway contract and should return the function selector for IMessageDispatcher.onCall to indicate successful processing.
🔗 Analysis chain
Verify the expected return value for onCall.
The function returns an empty bytes4 value. Verify if this is the expected return value by the gateway contract.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the expected return value in the gateway contract
# Search for onCall function definition in the gateway contract
ast-grep --pattern 'function onCall($$$) $$ returns (bytes4) { $$$ }'
Length of output: 71
Script:
#!/bin/bash
# Let's try a broader search for gateway interfaces or contracts
rg -l "gateway|Gateway" --type sol
# Then look for any interface or contract definitions related to gateway callbacks
ast-grep --pattern 'interface $_ {
$$$
function onCall($$$)
$$$
}'
# Also search for any documentation or comments about gateway callbacks
rg -i "oncall.*return" --type sol -A 2 -B 2
Length of output: 213
Script:
#!/bin/bash
# Search for any files with .sol extension
fd -e sol
# Then search for gateway-related content in all files
rg "gateway|Gateway" -A 5 -B 5
# Look for onCall implementations
rg "function onCall" -A 5 -B 5
Length of output: 82222
| function withdraw( | ||
| bytes memory receiver, | ||
| uint256 amount, | ||
| address zrc20, | ||
| RevertOptions memory revertOptions | ||
| ) external { | ||
| (address gasZRC20, uint256 gasFee) = IZRC20(zrc20).withdrawGasFee(); | ||
| uint256 target = zrc20 == gasZRC20 ? amount + gasFee : amount; | ||
| if (!IZRC20(zrc20).transferFrom(msg.sender, address(this), target)) | ||
| revert TransferFailed(); | ||
| IZRC20(zrc20).approve(address(gateway), target); | ||
| if (zrc20 != gasZRC20) { | ||
| if ( | ||
| !IZRC20(gasZRC20).transferFrom( | ||
| msg.sender, | ||
| address(this), | ||
| gasFee | ||
| ) | ||
| ) revert TransferFailed(); | ||
| IZRC20(gasZRC20).approve(address(gateway), gasFee); | ||
| } | ||
| gateway.withdraw(receiver, amount, zrc20, revertOptions); |
There was a problem hiding this comment.
Critical: Handle approve return values to prevent silent failures.
The approve calls' return values are not checked, which could lead to silent failures. This is particularly critical for cross-chain operations.
Apply this pattern to all approve calls in the contract:
- IZRC20(zrc20).approve(address(gateway), target);
+ if (!IZRC20(zrc20).approve(address(gateway), target)) {
+ revert TransferFailed();
+ }Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: Slither
[warning] 43-65: Unused return
Universal.withdraw(bytes,uint256,address,RevertOptions) (contracts/Universal.sol#43-65) ignores return value by IZRC20(gasZRC20).approve(address(gateway),gasFee) (contracts/Universal.sol#62)
[warning] 43-65: Unused return
Universal.withdraw(bytes,uint256,address,RevertOptions) (contracts/Universal.sol#43-65) ignores return value by IZRC20(zrc20).approve(address(gateway),target) (contracts/Universal.sol#53)
| // SPDX-License-Identifier: MIT | ||
| pragma solidity 0.8.26; | ||
|
|
||
| import "@zetachain/protocol-contracts/contracts/zevm/GatewayZEVM.sol"; |
There was a problem hiding this comment.
Import MessageContext for proper type definition.
The type MessageContext used in the onCall function is not imported or defined within this file.
Add the necessary import statement:
+import "@zetachain/protocol-contracts/contracts/zevm/MessageContext.sol";
import "@zetachain/protocol-contracts/contracts/zevm/GatewayZEVM.sol";Also applies to: 22-22
|
|
||
| import "@zetachain/protocol-contracts/contracts/zevm/GatewayZEVM.sol"; | ||
|
|
||
| contract Universal is UniversalContract { |
There was a problem hiding this comment.
Include the import statement for UniversalContract.
The contract Universal extends UniversalContract, but UniversalContract is neither imported nor defined in this file.
Apply this diff to include the import statement:
+import "@zetachain/zevm-example-contracts/contracts/UniversalContract.sol";
contract Universal is UniversalContract {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| contract Universal is UniversalContract { | |
| import "@zetachain/zevm-example-contracts/contracts/UniversalContract.sol"; | |
| contract Universal is UniversalContract { |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
examples/call/contracts/Connected.sol(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Slither
examples/call/contracts/Connected.sol
[warning] 42-51: Unused return
Connected.deposit(address,uint256,address,RevertOptions) (contracts/Connected.sol#42-51) ignores return value by IERC20(asset).approve(address(gateway),amount) (contracts/Connected.sol#49)
[warning] 53-63: Unused return
Connected.depositAndCall(address,uint256,address,bytes,RevertOptions) (contracts/Connected.sol#53-63) ignores return value by IERC20(asset).approve(address(gateway),amount) (contracts/Connected.sol#61)
🔇 Additional comments (3)
examples/call/contracts/Connected.sol (3)
1-9: LGTM! Appropriate imports and SafeERC20 usage.
The contract setup follows best practices with specific version pinning and necessary imports.
11-16: LGTM! Well-defined state variables and events.
The immutable gateway variable and events provide good visibility into contract operations.
65-98: LGTM! Solid implementation of payable functions and error handling.
The implementation includes appropriate error handling, modifiers, and fallback functions.
| function deposit( | ||
| address receiver, | ||
| uint256 amount, | ||
| address asset, | ||
| RevertOptions memory revertOptions | ||
| ) external { | ||
| IERC20(asset).safeTransferFrom(msg.sender, address(this), amount); | ||
| IERC20(asset).approve(address(gateway), amount); | ||
| gateway.deposit(receiver, amount, asset, revertOptions); | ||
| } | ||
|
|
||
| function depositAndCall( | ||
| address receiver, | ||
| uint256 amount, | ||
| address asset, | ||
| bytes calldata message, | ||
| RevertOptions memory revertOptions | ||
| ) external { | ||
| IERC20(asset).safeTransferFrom(msg.sender, address(this), amount); | ||
| IERC20(asset).approve(address(gateway), amount); | ||
| gateway.depositAndCall(receiver, amount, asset, message, revertOptions); | ||
| } |
There was a problem hiding this comment.
Use safeApprove instead of approve.
The contract uses SafeERC20 but doesn't utilize it for approve operations. Replace approve with safeApprove:
- IERC20(asset).approve(address(gateway), amount);
+ IERC20(asset).safeApprove(address(gateway), amount);This change should be applied to both deposit functions.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: Slither
[warning] 42-51: Unused return
Connected.deposit(address,uint256,address,RevertOptions) (contracts/Connected.sol#42-51) ignores return value by IERC20(asset).approve(address(gateway),amount) (contracts/Connected.sol#49)
[warning] 53-63: Unused return
Connected.depositAndCall(address,uint256,address,bytes,RevertOptions) (contracts/Connected.sol#53-63) ignores return value by IERC20(asset).approve(address(gateway),amount) (contracts/Connected.sol#61)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
examples/call/contracts/Universal.sol (3)
9-16: Consider adding descriptive error messagesWhile the error types are well-defined, consider adding descriptive messages to improve debugging:
- error TransferFailed(); - error Unauthorized(); + error TransferFailed(string message); + error Unauthorized(string message);
18-21: Standardize revert statement styleFor consistency with other parts of the contract and improved readability, use curly braces for the revert statement.
modifier onlyGateway() { - if (msg.sender != address(gateway)) revert Unauthorized(); + if (msg.sender != address(gateway)) { + revert Unauthorized(); + } _; }
Line range hint
71-105: Apply consistent error handling and approve checksThe function has similar issues as the withdraw function:
- Inconsistent revert statement style
- Missing approve return value checks
- Duplicate gas fee handling logic
Consider using the same helper function suggested for the withdraw function and apply consistent error handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
examples/call/contracts/Universal.sol(2 hunks)
🧰 Additional context used
🪛 GitHub Check: Slither
examples/call/contracts/Universal.sol
[warning] 44-69: Unused return
Universal.withdraw(bytes,uint256,address,RevertOptions) (contracts/Universal.sol#44-69) ignores return value by IZRC20(gasZRC20).approve(address(gateway),gasFee) (contracts/Universal.sol#66)
[warning] 44-69: Unused return
Universal.withdraw(bytes,uint256,address,RevertOptions) (contracts/Universal.sol#44-69) ignores return value by IZRC20(zrc20).approve(address(gateway),target) (contracts/Universal.sol#55)
🔇 Additional comments (1)
examples/call/contracts/Universal.sol (1)
107-121: LGTM!
The onCall and onRevert functions are well-implemented with proper access control and event emission.
| function withdraw( | ||
| bytes memory receiver, | ||
| uint256 amount, | ||
| address zrc20, | ||
| RevertOptions memory revertOptions | ||
| ) external { | ||
| (address gasZRC20, uint256 gasFee) = IZRC20(zrc20).withdrawGasFee(); | ||
| uint256 target = zrc20 == gasZRC20 ? amount + gasFee : amount; | ||
| if (!IZRC20(zrc20).transferFrom(msg.sender, address(this), target)) { | ||
| revert TransferFailed(); | ||
| } | ||
| IZRC20(zrc20).approve(address(gateway), target); | ||
| if (zrc20 != gasZRC20) { | ||
| if ( | ||
| !IZRC20(gasZRC20).transferFrom( | ||
| msg.sender, | ||
| address(this), | ||
| gasFee | ||
| ) | ||
| ) { | ||
| revert TransferFailed(); | ||
| } | ||
| IZRC20(gasZRC20).approve(address(gateway), gasFee); | ||
| } | ||
| gateway.withdraw(receiver, amount, zrc20, revertOptions); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor withdraw function for better maintainability
The function has multiple approve calls without return value checks and could benefit from helper methods.
- Add return value checks for approve calls:
- IZRC20(zrc20).approve(address(gateway), target);
+ if (!IZRC20(zrc20).approve(address(gateway), target)) {
+ revert TransferFailed();
+ }
- IZRC20(gasZRC20).approve(address(gateway), gasFee);
+ if (!IZRC20(gasZRC20).approve(address(gateway), gasFee)) {
+ revert TransferFailed();
+ }- Consider extracting the gas fee handling logic into a helper function to improve readability and reusability:
function handleGasFee(
address zrc20,
address gasZRC20,
uint256 gasFee
) private {
if (zrc20 != gasZRC20) {
if (!IZRC20(gasZRC20).transferFrom(msg.sender, address(this), gasFee)) {
revert TransferFailed();
}
if (!IZRC20(gasZRC20).approve(address(gateway), gasFee)) {
revert TransferFailed();
}
}
}🧰 Tools
🪛 GitHub Check: Slither
[warning] 44-69: Unused return
Universal.withdraw(bytes,uint256,address,RevertOptions) (contracts/Universal.sol#44-69) ignores return value by IZRC20(gasZRC20).approve(address(gateway),gasFee) (contracts/Universal.sol#66)
[warning] 44-69: Unused return
Universal.withdraw(bytes,uint256,address,RevertOptions) (contracts/Universal.sol#44-69) ignores return value by IZRC20(zrc20).approve(address(gateway),target) (contracts/Universal.sol#55)
| (, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit( | ||
| callOptions.gasLimit | ||
| ); | ||
| if (!IZRC20(zrc20).transferFrom(msg.sender, address(this), gasFee)) { | ||
| revert TransferFailed(); | ||
| } | ||
| IZRC20(zrc20).approve(address(gateway), gasFee); | ||
| gateway.call(receiver, zrc20, message, gasLimit, revertOptions); | ||
| gateway.call(receiver, zrc20, message, callOptions, revertOptions); |
There was a problem hiding this comment.
Handle approve return value
The approve call's return value should be checked to prevent silent failures.
- IZRC20(zrc20).approve(address(gateway), gasFee);
+ if (!IZRC20(zrc20).approve(address(gateway), gasFee)) {
+ revert TransferFailed();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit( | |
| callOptions.gasLimit | |
| ); | |
| if (!IZRC20(zrc20).transferFrom(msg.sender, address(this), gasFee)) { | |
| revert TransferFailed(); | |
| } | |
| IZRC20(zrc20).approve(address(gateway), gasFee); | |
| gateway.call(receiver, zrc20, message, gasLimit, revertOptions); | |
| gateway.call(receiver, zrc20, message, callOptions, revertOptions); | |
| (, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit( | |
| callOptions.gasLimit | |
| ); | |
| if (!IZRC20(zrc20).transferFrom(msg.sender, address(this), gasFee)) { | |
| revert TransferFailed(); | |
| } | |
| if (!IZRC20(zrc20).approve(address(gateway), gasFee)) { | |
| revert TransferFailed(); | |
| } | |
| gateway.call(receiver, zrc20, message, callOptions, revertOptions); |
|
@andresaiello please, review again. |
Call
Call from Base to ZetaChain
https://sepolia.basescan.org/tx/0x133cdf3a06195de0a6bb89dd4761ca98d1301534b3c4987f0ff93c95c3fff78c
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/0x133cdf3a06195de0a6bb89dd4761ca98d1301534b3c4987f0ff93c95c3fff78c
Call from ZetaChain to Base
https://zetachain-testnet.blockscout.com/tx/0x19d476fa2c3d29ba41467ae7f2742541fd11e0b67d6548fe7655a3d40274323e
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/0x19d476fa2c3d29ba41467ae7f2742541fd11e0b67d6548fe7655a3d40274323e
Withdraw And Call from ZetaChain to Base
https://zetachain-testnet.blockscout.com/tx/0x67099389ab6cb44ee03602d164320c615720b57762c5ddab042d65bdbe30c7a2
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/0x67099389ab6cb44ee03602d164320c615720b57762c5ddab042d65bdbe30c7a2
Swap
https://testnet.bscscan.com/tx/0x0d944bbfd5c9f3a9ffba14d342c30808a0e61e9cd6e7889ea6e557a266f3527d
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/0x0d944bbfd5c9f3a9ffba14d342c30808a0e61e9cd6e7889ea6e557a266f3527d
https://sepolia.basescan.org/tx/0x990d2f4c9dcbb96f0b61f6a9d06d6680e45cbdc6f4af9ab4fff2ba0207c9a199
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/0x990d2f4c9dcbb96f0b61f6a9d06d6680e45cbdc6f4af9ab4fff2ba0207c9a199
https://amoy.polygonscan.com/tx/0xb162f4a9a2414a7fd63ef3e7d34a113af14d325a97cab1cc8036d7d958eafae9
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/0xb162f4a9a2414a7fd63ef3e7d34a113af14d325a97cab1cc8036d7d958eafae9
NFT
ZetaChain -> Base NFT transfer
{"contractAddress":"0xCD58653a854817692079efd8ef0f6928aA66F628","transferTransactionHash":"0x554dec1282f3c367b4a465e391a0887f5b891f86ff2f32048c16c4932c278c4c","sender":"0x4955a3F38ff86ae92A914445099caa8eA2B9bA32","tokenId":"456003645905194763481523353718654221867725429194"}https://zetachain-testnet.blockscout.com/tx/0x554dec1282f3c367b4a465e391a0887f5b891f86ff2f32048c16c4932c278c4c
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/0x554dec1282f3c367b4a465e391a0887f5b891f86ff2f32048c16c4932c278c4c
Summary by CodeRabbit
Release Notes
New Features
connected-deposit,connected-deposit-and-call, anduniversal-withdraw.localnet.shscript for managing local blockchain interactions.ConnectedandUniversalcontracts with improved error handling and gas limit management.SwapandSwapToAnyTokencontracts for facilitating cross-chain token swaps.localnet.shscripts across various examples to improve deployment logic and parameter handling.Bug Fixes
Chores
package.jsonfiles to ensure compatibility and access to the latest features..gitignoreentries forlocalnet.jsonacross multiple directories to manage ignored files.