onlyGateway modifier #213
onlyGateway modifier #213
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a new modifier, Changes
Possibly related PRs
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 (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
examples/hello/contracts/Hello.sol (1)
35-37: LGTM: Consistent security model applied.The
onlyGatewaymodifier is correctly applied to theonRevertfunction, maintaining a consistent security model withonCall. This ensures that revert handling can only be triggered by the trusted gateway.Consider documenting these security constraints in the contract's NatSpec comments to make the security model explicit for integrators.
examples/nft/scripts/test.sh (2)
Line range hint
9-19: Consider enhancing the nft_balance function with error handling and optimizations.The function is well-structured with good visual formatting, but could be improved for production use.
Consider this enhanced implementation:
function nft_balance() { + # Cache contract addresses to avoid repeated string interpolation + local contracts=("$CONTRACT_ZETACHAIN" "$CONTRACT_ETHEREUM" "$CONTRACT_BNB") + local chains=("ZetaChain" "EVM Chain" "BNB Chain") + local emojis=("🟢" "🔵" "🟡") + echo -e "\n🖼️ NFT Balance" echo "---------------------------------------------" - local ZETACHAIN=$(cast call "$CONTRACT_ZETACHAIN" "balanceOf(address)(uint256)" "$SENDER") - local ETHEREUM=$(cast call "$CONTRACT_ETHEREUM" "balanceOf(address)(uint256)" "$SENDER") - local BNB=$(cast call "$CONTRACT_BNB" "balanceOf(address)(uint256)" "$SENDER") - echo "🟢 ZetaChain: $ZETACHAIN" - echo "🔵 EVM Chain: $ETHEREUM" - echo "🟡 BNB Chain: $BNB" + for i in {0..2}; do + if ! balance=$(cast call "${contracts[$i]}" "balanceOf(address)(uint256)" "$SENDER" 2>/dev/null); then + balance="Error: Failed to fetch balance" + fi + echo "${emojis[$i]} ${chains[$i]}: $balance" + done echo "---------------------------------------------" }
Line range hint
1-75: Consider enhancing script robustness and maintainability.While the script is functional, consider these production-ready improvements:
- Add version checks for required tools:
for cmd in npx hardhat jq cast; do if ! command -v $cmd &> /dev/null; then echo "Error: $cmd is required but not installed." exit 1 fi done
- Externalize configuration:
# config.sh export ZRC20_ETHEREUM="0x2ca7d64A7EFE2D62A725E2B35Cf7230D6677FfEe" export ZRC20_BNB="0x65a45c57636f9BcCeD4fe193A602008578BcA90b" # ... other configurations
- Add cleanup trap:
trap 'npx hardhat localnet-stop' EXITThese changes would make the script more maintainable and reliable in production environments.
examples/swap/contracts/Swap.sol (1)
113-115: Document the empty implementation intention.While the access control is correctly implemented, consider adding a comment explaining why this function intentionally has no implementation. This helps future maintainers understand the design decision.
+ /// @notice Implements the required onRevert interface method + /// @dev Intentionally empty as no revert handling is needed function onRevert( RevertContext calldata revertContext ) external override onlyGateway {}examples/swap/contracts/SwapToAnyToken.sol (2)
41-41: Consider adding events for important state changes.The
onlyGatewaymodifier addition is correct. However, consider emitting events for important operations to improve transparency and facilitate off-chain tracking.Add an event for cross-chain calls:
+ event CrossChainCallReceived( + uint256 indexed sourceChainId, + address indexed zrc20, + uint256 amount, + address targetToken + );And emit it in the onCall function:
) external virtual override onlyGateway { + emit CrossChainCallReceived( + context.chainID, + zrc20, + amount, + BytesHelperLib.bytesToAddress(message, 0) + );
155-157: Document the empty implementation and consider adding revert events.While the
onlyGatewaymodifier addition is correct, the empty implementation should be documented. Additionally, consider emitting events for revert operations.Add documentation and events:
+ /// @notice Handles revert operations from the gateway + /// @dev Currently implemented as a no-op as reverts are handled elsewhere function onRevert( RevertContext calldata revertContext ) external override onlyGateway { + emit CrossChainRevertReceived( + revertContext.sourceChainId, + revertContext.targetZRC20 + ); } + event CrossChainRevertReceived( + uint256 indexed sourceChainId, + address indexed targetZRC20 + );examples/nft/contracts/Universal.sol (1)
36-39: LGTM! Secure access control implementation.The modifier correctly restricts access to the gateway address. The immutable gateway address ensures that the access control cannot be tampered with post-deployment.
Consider adding an event emission when access is denied to aid in monitoring and debugging unauthorized access attempts.
examples/nft/contracts/Connected.sol (1)
Line range hint
82-90: Incorrect Return Type inonCallFunctionThe
onCallfunction is declared to return abytes4type, but it currently returns an empty string"". In Solidity, an empty string is of typestring, which is incompatible withbytes4. This will result in a type mismatch error during compilation.Apply the following change to return the correct
bytes4value:function onCall( MessageContext calldata messageContext, bytes calldata message ) external payable onlyGateway returns (bytes4) { if (messageContext.sender != counterparty) revert("Unauthorized"); (uint256 tokenId, address receiver, string memory uri) = abi.decode( message, (uint256, address, string) ); _safeMint(receiver, tokenId); _setTokenURI(tokenId, uri); - return ""; + return this.onCall.selector; }Returning
this.onCall.selectorprovides the function selector of theonCallfunction, which is abytes4value. This ensures compatibility with the expected return type and aligns with protocol conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
examples/hello/contracts/Echo.sol(2 hunks)examples/hello/contracts/Hello.sol(2 hunks)examples/nft/contracts/Connected.sol(3 hunks)examples/nft/contracts/Universal.sol(3 hunks)examples/nft/scripts/test.sh(1 hunks)examples/swap/contracts/Swap.sol(3 hunks)examples/swap/contracts/SwapToAnyToken.sol(3 hunks)
🔇 Additional comments (9)
examples/hello/contracts/Echo.sol (3)
13-16: Well-implemented access control modifier.
The onlyGateway modifier effectively restricts function access to only the gateway contract, with a clear error message and proper use of the immutable gateway reference.
26-28: Correctly secured onRevert function.
The onlyGateway modifier has been properly applied to the onRevert function, preventing unauthorized calls from malicious third-party contracts.
Line range hint 35-41: Consider securing the call() function.
The call() function allows interaction with the gateway but lacks access control. Consider adding the onlyGateway modifier or implementing additional access controls to prevent unauthorized calls to the gateway.
examples/hello/contracts/Hello.sol (2)
30-30: LGTM: Properly secured cross-chain message handler.
The onlyGateway modifier is correctly applied to the onCall function, ensuring that only the trusted gateway contract can invoke this cross-chain message handler.
16-19: LGTM: Secure modifier implementation.
The onlyGateway modifier correctly restricts access using the immutable gateway reference. The implementation follows security best practices with a clear error message.
Let's verify that this modifier is consistently applied across all critical functions:
✅ Verification successful
The previous ast-grep pattern might not have worked well with Solidity syntax. Let's try a different approach to find the external functions and verify the modifier usage.
Let's examine the Hello.sol contract to verify the consistent application of the onlyGateway modifier on critical functions.
Verified: onlyGateway modifier is correctly implemented and applied to critical functions
The onlyGateway modifier is properly implemented and correctly applied to the security-critical functions:
onCall: Protected withonlyGatewaymodifieronRevert: Protected withonlyGatewaymodifier
The remaining external functions (call and withdrawAndCall) are intentionally left without the modifier as they are user-facing entry points that interact with the gateway contract, rather than callback functions that should only be called by the gateway.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any external/public functions that might need the onlyGateway modifier
ast-grep --pattern 'function $FNAME($$$) external $$$'
Length of output: 56
Script:
#!/bin/bash
# Let's try a more specific search for Solidity files first
fd -e sol -x cat {}
# Then search for external functions in the output
rg "function.*external" -A 2
Length of output: 29288
Script:
#!/bin/bash
# Let's specifically look at the Hello.sol contract to verify the modifier usage
cat examples/hello/contracts/Hello.sol
Length of output: 2939
examples/swap/contracts/Swap.sol (1)
19-22: LGTM! Robust access control implementation.
The modifier correctly implements the access control check using the immutable gateway address set during construction. The error message is clear and descriptive.
examples/swap/contracts/SwapToAnyToken.sol (1)
20-23: LGTM! Modifier implementation is secure and follows best practices.
The onlyGateway modifier correctly restricts access to the gateway address with a clear error message.
Let's verify that the gateway address is properly initialized:
✅ Verification successful
Gateway initialization and modifier implementation verified as secure
The onlyGateway modifier is correctly implemented with proper initialization of the immutable gateway variable in the constructor, where it's set using the gatewayAddress parameter. The implementation follows security best practices:
- Gateway address is immutable, preventing post-deployment modifications
- Constructor properly initializes the gateway address
- Modifier performs a strict equality check against the stored gateway address
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify gateway initialization across the codebase
ast-grep --pattern 'constructor($params) {
$$$
gateway = $gateway;
$$$
}'
Length of output: 81
Script:
#!/bin/bash
# Let's try a broader pattern to catch constructor and gateway initialization
ast-grep --pattern 'constructor($$$) {
$$$
}'
# Also search for gateway variable declaration
ast-grep --pattern 'IZetaConnector public immutable gateway;'
# Backup search using ripgrep
rg -A 5 "constructor.*\(" "examples/swap/contracts/SwapToAnyToken.sol"
Length of output: 403
examples/nft/contracts/Connected.sol (2)
21-24: 'onlyGateway' Modifier Appropriately Implemented
The onlyGateway modifier is correctly defined to restrict access so that only the gateway contract can invoke functions adorned with this modifier. This enhances the security of the contract by preventing unauthorized access to critical functions.
94-94: onRevert Function Properly Secured with onlyGateway Modifier
The onRevert function is appropriately updated with the onlyGateway modifier, ensuring that only the gateway contract can invoke it. This addition strengthens the contract's security by preventing unauthorized entities from triggering revert logic.
| npx hardhat localnet-check | ||
| nft_balance |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance visibility of NFT state transitions.
While the current placement of nft_balance calls provides good visibility of post-operation states, consider adding checks before operations to clearly show state transitions.
Consider this pattern for operations:
+# Before minting
+echo -e "\nInitial balances:"
+nft_balance
+
NFT_ID=$(npx hardhat mint --network localhost --json --contract "$CONTRACT_ZETACHAIN" --token-uri https://example.com/nft/metadata/1 | jq -r '.tokenId')
echo -e "\nMinted NFT with ID: $NFT_ID on ZetaChain."
npx hardhat localnet-check
+echo -e "\nBalances after minting:"
nft_balanceApply this pattern before and after each major operation (transfers) to provide clear visibility of state changes.
Also applies to: 55-55, 61-61, 67-67, 73-73
| @@ -31,7 +36,7 @@ contract Swap is UniversalContract { | |||
| address zrc20, | |||
| uint256 amount, | |||
| bytes calldata message | |||
| ) external override { | |||
| ) external override onlyGateway { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding reentrancy protection.
While the onlyGateway modifier provides access control, the function performs multiple external calls during token operations. Consider adding a reentrancy guard to prevent potential attack vectors.
- ) external override onlyGateway {
+ ) external override onlyGateway nonReentrant {Add the ReentrancyGuard import and inheritance:
import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
contract Swap is UniversalContract, ReentrancyGuard {| @@ -140,7 +145,7 @@ contract Universal is | |||
| } | |||
| } | |||
|
|
|||
| function onRevert(RevertContext calldata context) external { | |||
| function onRevert(RevertContext calldata context) external onlyGateway { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for onRevert handler.
While the onlyGateway modifier provides access control, consider adding validation for the decoded parameters to prevent potential issues with malformed input data.
Add input validation:
function onRevert(RevertContext calldata context) external onlyGateway {
(uint256 tokenId, address sender, string memory uri) = abi.decode(
context.revertMessage,
(uint256, address, string)
);
+ require(sender != address(0), "Invalid sender address");
+ require(bytes(uri).length > 0, "Empty URI");
_safeMint(sender, tokenId);
_setTokenURI(tokenId, uri);
}| @@ -102,7 +107,7 @@ | |||
| address zrc20, | |||
| uint256 amount, | |||
| bytes calldata message | |||
| ) external override { | |||
| ) external override onlyGateway { | |||
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Hardcoded gas limit needs to be replaced with state variable
The review comment is partially correct. After analyzing the contract:
- The contract has a state variable
gasLimitinitialized to 700000, but theonCallfunction uses the hardcoded value instead of this configurable variable. - However, regarding reentrancy concerns:
- The function follows CEI (Checks-Effects-Interactions) pattern
- State changes (
_safeMintand_setTokenURI) occur before external calls - The
onlyGatewaymodifier provides additional security by restricting access
Suggested improvements:
- Replace hardcoded
700000withgasLimitstate variable inonCallfunction:
IZRC20(destination).withdrawGasFeeWithGasLimit(gasLimit)
...
CallOptions(gasLimit, false)🔗 Analysis chain
Consider gas limit configuration and reentrancy protection.
While the onlyGateway modifier addition enhances security, there are two considerations:
- The function uses a hardcoded gas limit (700000) which differs from the contract's
gasLimitstate variable. - The function performs external calls after state changes, which could be vulnerable to reentrancy.
Consider these improvements:
- gateway.call(
- counterparty[destination],
- destination,
- abi.encode(tokenId, sender, uri),
- CallOptions(700000, false),
- RevertOptions(address(0), false, address(0), "", 0)
- );
+ // Use contract's configurable gas limit
+ gateway.call(
+ counterparty[destination],
+ destination,
+ abi.encode(tokenId, sender, uri),
+ CallOptions(gasLimit, false),
+ RevertOptions(address(0), false, address(0), "", 0)
+ );Let's verify the external call patterns:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential reentrancy patterns in the codebase
ast-grep --pattern 'function $FNAME($_) {
$$$
_safeMint($_, $_);
$$$
gateway.call($$$);
$$$
}'
Length of output: 106
Script:
#!/bin/bash
# Let's search for the onCall function implementation to analyze state changes and external calls
ast-grep --pattern 'function onCall($_) {
$$$
}'
# Also search for gateway.call patterns to understand the gas limit usage
rg "gateway\.call" -A 3
Length of output: 1445
Script:
#!/bin/bash
# Let's examine the full Universal.sol contract to understand the context better
cat examples/nft/contracts/Universal.sol
# Also check for gasLimit state variable usage
rg "gasLimit" -A 2 examples/nft/contracts/Universal.sol
Length of output: 6279
To prevent malicious third-party contract from calling
onCallandonRevert.Summary by CodeRabbit
New Features
onlyGateway, to enhance security across multiple contracts, ensuring that critical functions can only be executed by the designated gateway.onCall,onRevert) to include theonlyGatewaymodifier, reinforcing access restrictions.Bug Fixes
Universalcontract to revert with aTransferFailederror if gas fee transfers fail.Chores
test.shscript to include calls to display NFT balances across different blockchain networks, enhancing visibility during execution.