fix: prevent using 0 gas limit for call and withdrawAndCall #370
Conversation
skosito
requested review from
andresaiello,
brewmaster012,
lumtis,
CharlieMc0,
fadeev and
fbac
as code owners
📝 WalkthroughWalkthroughThe changes introduced in the Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
skosito
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #370 +/- ##
==========================================
+ Coverage 83.07% 83.29% +0.21%
==========================================
Files 8 8
Lines 384 389 +5
Branches 122 127 +5
==========================================
+ Hits 319 324 +5
Misses 65 65 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
v2/contracts/zevm/interfaces/IGatewayZEVM.sol (1)
89-91: LGTM! Consider adding a comment for clarity.The addition of the
InsufficientGasLimiterror aligns well with the PR objective to prevent passing 0 gas limit. This error will allow for more specific handling of gas limit issues in the contract implementation.Consider adding a brief comment to explain when this error is thrown, for example:
/// @notice Error indicating an insufficient gas limit. /// @dev Thrown when the provided gas limit is too low or zero. error InsufficientGasLimit();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (48)
v2/docs/src/contracts/Revert.sol/interface.Revertable.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/struct.RevertContext.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/struct.RevertOptions.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ERC20Custody.sol/contract.ERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/GatewayEVM.sol/contract.GatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorBase.sol/abstract.ZetaConnectorBase.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorNative.sol/contract.ZetaConnectorNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorNonNative.sol/contract.ZetaConnectorNonNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20CustodyErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20CustodyEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.Callable.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/struct.MessageContext.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IZetaConnector.sol/interface.IZetaConnectorEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IZetaNonEthNew.sol/interface.IZetaNonEthNew.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/GatewayZEVM.sol/contract.GatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/SystemContract.sol/contract.SystemContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/SystemContract.sol/interface.SystemContractErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/ZRC20.sol/contract.ZRC20.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/ZRC20.sol/interface.ZRC20Errors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/struct.CallOptions.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/ISystem.sol/interface.ISystem.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IWZETA.sol/interface.IWETH9.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/enum.CoinType.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.IZRC20.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.IZRC20Metadata.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.ZRC20Events.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.UniversalContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.zContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/struct.zContext.mdis excluded by!v2/docs/**v2/pkg/gatewayevmzevm.t.sol/gatewayevmzevmtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.sol/gatewayzevm.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.t.sol/gatewayzevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.t.sol/gatewayzevmoutboundtest.gois excluded by!v2/pkg/**v2/pkg/igatewayzevm.sol/igatewayzevm.gois excluded by!v2/pkg/**v2/pkg/igatewayzevm.sol/igatewayzevmerrors.gois excluded by!v2/pkg/**v2/pkg/senderzevm.sol/senderzevm.gois excluded by!v2/pkg/**v2/pkg/zrc20.t.sol/zrc20test.gois excluded by!v2/pkg/**v2/types/factories/GatewayZEVM__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayZEVM.sol/IGatewayZEVMErrors__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayZEVM.sol/IGatewayZEVM__factory.tsis excluded by!v2/types/**v2/types/factories/SenderZEVM__factory.tsis excluded by!v2/types/**
📒 Files selected for processing (3)
- v2/contracts/zevm/GatewayZEVM.sol (5 hunks)
- v2/contracts/zevm/interfaces/IGatewayZEVM.sol (1 hunks)
- v2/test/GatewayZEVM.t.sol (4 hunks)
🔇 Additional comments (6)
v2/contracts/zevm/interfaces/IGatewayZEVM.sol (2)
Line range hint
1-324: Summary: New error added to enhance gas limit handlingThe addition of the
InsufficientGasLimiterror to theIGatewayZEVMErrorsinterface is a focused change that enhances the contract's ability to handle gas limit issues. This change:
- Aligns with the PR objective to prevent passing 0 gas limit.
- Maintains consistency with the existing error structure in the interface.
- Provides a specific error for gas limit-related issues, improving error handling and debugging.
The impact of this change is minimal on the interface level but potentially significant for the contract implementation. Ensure that the new error is utilized appropriately in the contract code, particularly in functions dealing with gas limits.
89-91: Verify usage of InsufficientGasLimit error in contract implementationThe new
InsufficientGasLimiterror has been added to the interface, which is good. To ensure it's being used correctly:
- Verify that this error is thrown in the contract implementation when a zero or insufficient gas limit is provided.
- Check functions like
withdrawAndCallandcallthat acceptgasLimitorCallOptionsto ensure they use this error appropriately.To help verify the implementation, you can run the following script:
This script will help identify where the new error is being used and where gas limit checks are performed in the codebase.
✅ Verification successful
InsufficientGasLimit error is correctly implemented and utilized
The
InsufficientGasLimiterror is properly used in the contract implementation. Multiple functions, includingwithdrawAndCallandcall, revert withInsufficientGasLimitwhen thegasLimitis zero. Additionally, corresponding tests ensure that this error handling is validated.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the usage of InsufficientGasLimit error in the contract implementation echo "Searching for InsufficientGasLimit usage:" rg --type solidity "InsufficientGasLimit" -C 5 echo "\nSearching for gas limit checks:" rg --type solidity "gasLimit\s*==?\s*0" -C 5 rg --type solidity "gasLimit\s*[<>]=?\s*" -C 5Length of output: 411
Script:
#!/bin/bash # Search for the usage of InsufficientGasLimit error in Solidity files echo "Searching for InsufficientGasLimit usage in .sol files:" rg "InsufficientGasLimit" --glob "*.sol" -C 5 echo "\nSearching for gas limit checks in .sol files:" rg "gasLimit\s*==?\s*0" --glob "*.sol" -C 5 rg "gasLimit\s*[<>]=?\s*" --glob "*.sol" -C 5Length of output: 14831
v2/test/GatewayZEVM.t.sol (4)
155-159: Test correctly handles zero gas limit forwithdrawAndCallZRC20The test function
testWithdrawAndCallZRC20FailsIfGasLimitIsZeroproperly verifies that callingwithdrawAndCallwith a gas limit of zero reverts withInsufficientGasLimit.selector.
216-221: Test correctly handles zero gas limit incallOptionsforwithdrawAndCallZRC20The test function
testWithdrawAndCallZRC20WithCallOptsFailsIfGasLimitIsZerocorrectly setscallOptions.gasLimitto zero and expects a revert withInsufficientGasLimit.selectorwhen callingwithdrawAndCall.
511-515: Test correctly handles zero gas limit forcallThe test function
testCallFailsIfGasLimitIsZeroaccurately expects a revert withInsufficientGasLimit.selectorwhengateway.callis invoked with a gas limit of zero.
531-536: Test correctly handles zero gas limit incallOptionsforcallThe test function
testCallWithCallOptsFailsIfGasLimitIsZeroproperly setscallOptions.gasLimitto zero and verifies thatgateway.callreverts withInsufficientGasLimit.selector.
| @@ -177,6 +177,7 @@ contract GatewayZEVM is | |||
| { | |||
| if (receiver.length == 0) revert ZeroAddress(); | |||
| if (amount == 0) revert InsufficientZRC20Amount(); | |||
| if (gasLimit == 0) revert InsufficientGasLimit(); | |||
There was a problem hiding this comment.
Define InsufficientGasLimit error
The error InsufficientGasLimit() is used but not defined in the contract. Please define the InsufficientGasLimit error to maintain consistency with other error handling.
Suggested addition:
/// @notice Error indicating that the provided gas limit is zero.
error InsufficientGasLimit();Place this definition alongside other error definitions in the contract.
Also applies to: 218-218, 323-323, 348-348, 370-370
Add unit tests for gas limit validations
To ensure the new InsufficientGasLimit checks function as intended, please add unit tests that verify the contract correctly reverts when a zero gas limit is provided.
Also applies to: 218-218, 323-323, 348-348, 370-370
| function testWithdrawZETAWithCallOptsWithMessageFailsIfGasLimitIsZero() public { | ||
| uint256 amount = 1; | ||
| uint256 ownerBalanceBefore = zetaToken.balanceOf(owner); | ||
| uint256 gatewayBalanceBefore = zetaToken.balanceOf(address(gateway)); | ||
| uint256 protocolAddressBalanceBefore = protocolAddress.balance; | ||
| bytes memory message = abi.encodeWithSignature("hello(address)", addr1); | ||
| uint256 chainId = 1; | ||
|
|
||
| // Remove allowance for gateway | ||
| vm.prank(owner); | ||
| zetaToken.approve(address(gateway), 0); | ||
|
|
||
| callOptions.gasLimit = 0; | ||
| vm.expectRevert(InsufficientGasLimit.selector); | ||
| gateway.withdrawAndCall(abi.encodePacked(addr1), amount, chainId, message, callOptions, revertOptions); | ||
| } |
There was a problem hiding this comment.
Test may not correctly validate gas limit zero due to zero allowance
In the test function testWithdrawZETAWithCallOptsWithMessageFailsIfGasLimitIsZero, the allowance for the gateway is removed by setting zetaToken.approve(address(gateway), 0); before setting callOptions.gasLimit = 0. This causes the withdrawAndCall call to revert due to insufficient allowance, potentially causing the test to fail for the wrong reason.
Apply this diff to fix the test:
- // Remove allowance for gateway
- vm.prank(owner);
- zetaToken.approve(address(gateway), 0);
callOptions.gasLimit = 0;
vm.expectRevert(InsufficientGasLimit.selector);
gateway.withdrawAndCall(abi.encodePacked(addr1), amount, chainId, message, callOptions, revertOptions);By removing the code that resets the allowance, the test will correctly check for a revert due to an InsufficientGasLimit, ensuring the test validates the intended behavior.
📝 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.
| function testWithdrawZETAWithCallOptsWithMessageFailsIfGasLimitIsZero() public { | |
| uint256 amount = 1; | |
| uint256 ownerBalanceBefore = zetaToken.balanceOf(owner); | |
| uint256 gatewayBalanceBefore = zetaToken.balanceOf(address(gateway)); | |
| uint256 protocolAddressBalanceBefore = protocolAddress.balance; | |
| bytes memory message = abi.encodeWithSignature("hello(address)", addr1); | |
| uint256 chainId = 1; | |
| // Remove allowance for gateway | |
| vm.prank(owner); | |
| zetaToken.approve(address(gateway), 0); | |
| callOptions.gasLimit = 0; | |
| vm.expectRevert(InsufficientGasLimit.selector); | |
| gateway.withdrawAndCall(abi.encodePacked(addr1), amount, chainId, message, callOptions, revertOptions); | |
| } | |
| function testWithdrawZETAWithCallOptsWithMessageFailsIfGasLimitIsZero() public { | |
| uint256 amount = 1; | |
| uint256 ownerBalanceBefore = zetaToken.balanceOf(owner); | |
| uint256 gatewayBalanceBefore = zetaToken.balanceOf(address(gateway)); | |
| uint256 protocolAddressBalanceBefore = protocolAddress.balance; | |
| bytes memory message = abi.encodeWithSignature("hello(address)", addr1); | |
| uint256 chainId = 1; | |
| callOptions.gasLimit = 0; | |
| vm.expectRevert(InsufficientGasLimit.selector); | |
| gateway.withdrawAndCall(abi.encodePacked(addr1), amount, chainId, message, callOptions, revertOptions); | |
| } |
Summary by CodeRabbit
New Features
withdrawAndCallandcallfunctions.withdrawAndCallandcallfunctions to accept flexible gas limit specifications.Bug Fixes
Tests