feat: improve revert handling #361
Conversation
skosito
requested review from
andresaiello,
brewmaster012,
lumtis,
CharlieMc0,
fadeev and
fbac
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve multiple modifications across several Solidity files, primarily focusing on enhancing the Changes
Possibly related issues
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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #361 +/- ##
==========================================
+ Coverage 83.00% 83.10% +0.09%
==========================================
Files 8 8
Lines 359 361 +2
Branches 117 118 +1
==========================================
+ Hits 298 300 +2
Misses 61 61 ☔ 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: 1
🧹 Outside diff range and nitpick comments (6)
v2/test/utils/TestUniversalContract.sol (1)
Line range hint
1-70: Overall impact: Revert handling capability addedThe changes to this contract are minimal and focused on adding revert-handling capabilities through the implementation of the Revertable interface. The existing onRevert function seems to align with this new interface, but this should be verified. As no other changes were made to the contract's functionality, it appears that existing behavior should remain intact.
To ensure the contract's integrity:
- Verify that the onRevert function fully complies with the Revertable interface requirements.
- Conduct thorough testing of all existing functionality to confirm that the addition of Revertable hasn't introduced any unintended side effects.
- Add new test cases specifically for the revert-handling capabilities introduced by implementing Revertable.
v2/contracts/evm/interfaces/IGatewayEVM.sol (1)
87-88: LGTM! Consider adding a comment for clarity.The new error
NotAllowedToCallOnRevertis a logical addition to the interface, complementing the existingNotAllowedToCallOnCallerror. It aligns well with the PR objective of preventing the invocation of theonRevertfunction during arbitrary calls in the GatewayEVM.Consider adding a brief comment explaining when this error is thrown, similar to other errors in the interface. For example:
/// @notice Error when trying to call onRevert method using arbitrary call. error NotAllowedToCallOnRevert();v2/test/ZetaConnectorNative.t.sol (1)
Line range hint
1-365: Enhance testWithdrawAndRevert() to verify the 'sender' fieldWhile the change in the
setUp()function doesn't introduce any breaking changes, it provides an opportunity to enhance thetestWithdrawAndRevert()function. Consider adding an assertion to verify that the 'sender' field in theRevertContextis correctly set and used during the test.Add the following assertion to the
testWithdrawAndRevert()function:assertEq(revertContext.sender, owner, "RevertContext sender should be set to owner");This will ensure that the 'sender' field is correctly initialized and maintained throughout the test execution.
v2/contracts/zevm/GatewayZEVM.sol (1)
Line range hint
1-502: Consider updating other functions for consistencyWhile the changes made align with the PR objectives, it's worth considering if other functions that still use UniversalContract (e.g., execute and depositAndCall) should also be updated for consistency. This depends on whether these functions are intended to handle revert operations or if they serve different purposes.
v2/contracts/evm/GatewayEVM.sol (2)
Line range hint
432-446: Refactor to remove inline assembly for safer codeThe
revertIfOnCallOrOnRevertfunction uses inline assembly to extract the function selector fromdata. Using Solidity's built-in functions can improve readability and safety by eliminating low-level code.Apply this diff to refactor the function without inline assembly:
function revertIfOnCallOrOnRevert(bytes calldata data) private pure { if (data.length >= 4) { - bytes4 functionSelector; - assembly { - functionSelector := calldataload(data.offset) - } + bytes4 functionSelector = bytes4(data[:4]); if (functionSelector == Callable.onCall.selector) { revert NotAllowedToCallOnCall(); } - if (functionSelector == Revertable.onRevert.selector) { revert NotAllowedToCallOnRevert(); } } }This change enhances code safety and maintainability by utilizing high-level Solidity features.
Line range hint
432-446: Optimize conditional statements to avoid redundant checksCurrently, both
ifconditions are checked independently, even though only one can be true at a time. Combining them usingelse ifcan slightly improve efficiency.Apply this diff to optimize the conditional logic:
function revertIfOnCallOrOnRevert(bytes calldata data) private pure { if (data.length >= 4) { bytes4 functionSelector = bytes4(data[:4]); if (functionSelector == Callable.onCall.selector) { revert NotAllowedToCallOnCall(); - } - - if (functionSelector == Revertable.onRevert.selector) { + } else if (functionSelector == Revertable.onRevert.selector) { revert NotAllowedToCallOnRevert(); } } }This adjustment prevents the second condition from being evaluated if the first condition is true.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (119)
v2/docs/src/SUMMARY.mdis excluded by!v2/docs/**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/evm/interfaces/README.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/GatewayZEVM.sol/contract.GatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/README.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/README.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/docs/src/index.mdis excluded by!v2/docs/**v2/pkg/erc20custody.sol/erc20custody.gois excluded by!v2/pkg/**v2/pkg/erc20custody.t.sol/erc20custodytest.gois excluded by!v2/pkg/**v2/pkg/erc20custodyechidnatest.sol/erc20custodyechidnatest.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.sol/gatewayevm.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevmtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmechidnatest.sol/gatewayevmechidnatest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmupgrade.t.sol/gatewayevmuupsupgradetest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmupgradetest.sol/gatewayevmupgradetest.gois excluded by!v2/pkg/**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/ierc20custody.sol/ierc20custody.gois excluded by!v2/pkg/**v2/pkg/ierc20custody.sol/ierc20custodyevents.gois excluded by!v2/pkg/**v2/pkg/igatewayevm.sol/igatewayevm.gois excluded by!v2/pkg/**v2/pkg/igatewayevm.sol/igatewayevmerrors.gois excluded by!v2/pkg/**v2/pkg/igatewayevm.sol/igatewayevmevents.gois excluded by!v2/pkg/**v2/pkg/igatewayzevm.sol/igatewayzevm.gois excluded by!v2/pkg/**v2/pkg/ireceiverevm.sol/ireceiverevmevents.gois excluded by!v2/pkg/**v2/pkg/izetaconnector.sol/izetaconnectorevents.gois excluded by!v2/pkg/**v2/pkg/receiverevm.sol/receiverevm.gois excluded by!v2/pkg/**v2/pkg/revert.sol/revertable.gois excluded by!v2/pkg/**v2/pkg/senderzevm.sol/senderzevm.gois excluded by!v2/pkg/**v2/pkg/systemcontract.sol/systemcontract.gois excluded by!v2/pkg/**v2/pkg/systemcontractmock.sol/systemcontractmock.gois excluded by!v2/pkg/**v2/pkg/testuniversalcontract.sol/testuniversalcontract.gois excluded by!v2/pkg/**v2/pkg/universalcontract.sol/universalcontract.gois excluded by!v2/pkg/**v2/pkg/zetaconnectorbase.sol/zetaconnectorbase.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornative.sol/zetaconnectornative.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornative.t.sol/zetaconnectornativetest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.sol/zetaconnectornonnative.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.t.sol/zetaconnectornonnativetest.gois excluded by!v2/pkg/**v2/pkg/zrc20.t.sol/zrc20test.gois excluded by!v2/pkg/**v2/types/ERC20Custody.tsis excluded by!v2/types/**v2/types/ERC20CustodyEchidnaTest.tsis excluded by!v2/types/**v2/types/GatewayEVM.tsis excluded by!v2/types/**v2/types/GatewayEVMEchidnaTest.tsis excluded by!v2/types/**v2/types/GatewayEVMUpgradeTest.tsis excluded by!v2/types/**v2/types/GatewayZEVM.tsis excluded by!v2/types/**v2/types/IERC20Custody.sol/IERC20Custody.tsis excluded by!v2/types/**v2/types/IERC20Custody.sol/IERC20CustodyEvents.tsis excluded by!v2/types/**v2/types/IGatewayEVM.sol/IGatewayEVM.tsis excluded by!v2/types/**v2/types/IGatewayEVM.sol/IGatewayEVMEvents.tsis excluded by!v2/types/**v2/types/IGatewayZEVM.sol/IGatewayZEVM.tsis excluded by!v2/types/**v2/types/IReceiverEVM.sol/IReceiverEVMEvents.tsis excluded by!v2/types/**v2/types/IZetaConnector.sol/IZetaConnectorEvents.tsis excluded by!v2/types/**v2/types/ReceiverEVM.tsis excluded by!v2/types/**v2/types/Revert.sol/Revertable.tsis excluded by!v2/types/**v2/types/TestUniversalContract.tsis excluded by!v2/types/**v2/types/UniversalContract.sol/UniversalContract.tsis excluded by!v2/types/**v2/types/ZetaConnectorBase.tsis excluded by!v2/types/**v2/types/ZetaConnectorNative.tsis excluded by!v2/types/**v2/types/ZetaConnectorNonNative.tsis excluded by!v2/types/**v2/types/factories/ERC20CustodyEchidnaTest__factory.tsis excluded by!v2/types/**v2/types/factories/ERC20Custody__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVMEchidnaTest__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVMUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVM__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayZEVM__factory.tsis excluded by!v2/types/**v2/types/factories/IERC20Custody.sol/IERC20CustodyEvents__factory.tsis excluded by!v2/types/**v2/types/factories/IERC20Custody.sol/IERC20Custody__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayEVM.sol/IGatewayEVMErrors__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayEVM.sol/IGatewayEVMEvents__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayEVM.sol/IGatewayEVM__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayZEVM.sol/IGatewayZEVM__factory.tsis excluded by!v2/types/**v2/types/factories/IReceiverEVM.sol/IReceiverEVMEvents__factory.tsis excluded by!v2/types/**v2/types/factories/IZetaConnector.sol/IZetaConnectorEvents__factory.tsis excluded by!v2/types/**v2/types/factories/ReceiverEVM__factory.tsis excluded by!v2/types/**v2/types/factories/Revert.sol/Revertable__factory.tsis excluded by!v2/types/**v2/types/factories/SenderZEVM__factory.tsis excluded by!v2/types/**v2/types/factories/SystemContract.sol/SystemContract__factory.tsis excluded by!v2/types/**v2/types/factories/SystemContractMock.sol/SystemContractMock__factory.tsis excluded by!v2/types/**v2/types/factories/TestUniversalContract__factory.tsis excluded by!v2/types/**v2/types/factories/UniversalContract.sol/UniversalContract__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorBase__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNative__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNonNative__factory.tsis excluded by!v2/types/**
📒 Files selected for processing (11)
- v2/contracts/Revert.sol (1 hunks)
- v2/contracts/evm/GatewayEVM.sol (3 hunks)
- v2/contracts/evm/interfaces/IGatewayEVM.sol (1 hunks)
- v2/contracts/zevm/GatewayZEVM.sol (3 hunks)
- v2/contracts/zevm/interfaces/UniversalContract.sol (0 hunks)
- v2/test/ERC20Custody.t.sol (1 hunks)
- v2/test/GatewayEVM.t.sol (2 hunks)
- v2/test/GatewayZEVM.t.sol (1 hunks)
- v2/test/ZetaConnectorNative.t.sol (1 hunks)
- v2/test/ZetaConnectorNonNative.t.sol (1 hunks)
- v2/test/utils/TestUniversalContract.sol (1 hunks)
💤 Files not reviewed due to no reviewable changes (1)
- v2/contracts/zevm/interfaces/UniversalContract.sol
🔇 Additional comments (16)
v2/contracts/Revert.sol (2)
Line range hint
1-38: The overall structure and documentation of the file are well-maintained.The file is well-organized with appropriate use of comments, clear struct definitions, and a concise interface declaration. The use of a recent Solidity version (0.8.26) is commendable. The MIT license is correctly specified at the beginning of the file.
19-24: LGTM! The addition of thesenderfield enhances the context provided during revert operations.The new
senderfield in theRevertContextstruct is a valuable addition. It provides more context during revert scenarios by capturing the address of the account that initiated the smart contract call. This change is well-documented with a clear and concise comment.To ensure this change is consistently applied throughout the codebase, let's verify its usage:
✅ Verification successful
Retrying search without specifying file type for Solidity files.
To accurately verify the usage of
RevertContextand the implementation ofonRevertfunctions, let's search for.solfiles directly and inspect their contents:
The addition of the
senderfield toRevertContexthas been successfully and consistently implemented across the codebase.The verification process shows that the
RevertContextstruct now includes thesenderfield in all relevant parts of the project, including contracts, interfaces, and generated bindings. TheonRevertfunction implementations are correctly using the updatedRevertContextstructure.No inconsistencies or uses of the old
RevertContextstructure were found during this verification. The change appears to have been applied thoroughly and correctly throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for RevertContext usage and ensure the sender field is properly handled # Search for RevertContext struct usage echo "Searching for RevertContext usage:" rg --type solidity -A 5 'RevertContext' # Search for onRevert function implementations echo "Searching for onRevert function implementations:" rg --type solidity -A 10 'function onRevert\('Length of output: 340
Script:
#!/bin/bash # Description: Search for RevertContext usage and onRevert function implementations in Solidity files # Find all Solidity files and search for RevertContext echo "Searching for RevertContext usage in .sol files:" fd --extension sol | xargs rg 'RevertContext' # Find all Solidity files and search for onRevert function implementations echo "Searching for onRevert function implementations in .sol files:" fd --extension sol | xargs rg 'function onRevert\('Length of output: 372892
v2/test/utils/TestUniversalContract.sol (2)
6-6: LGTM: Import statement updated correctlyThe import statement has been properly updated to include both
RevertContextandRevertablefrom the Revert.sol file. This change is consistent with the modifications in the contract declaration and suggests the implementation of revert-handling functionality.
12-12: LGTM: Contract inheritance updated correctlyThe contract declaration has been properly updated to include inheritance from both
UniversalContractandRevertable. This change is consistent with the updated import statement and suggests the implementation of additional revert-handling functionality.To ensure full compliance with the Revertable interface, please run the following script to verify the interface implementation:
✅ Verification successful
Verified: Revertable interface implemented correctly
The
TestUniversalContractcorrectly inherits from bothUniversalContractandRevertable, and implements theonRevertfunction with the appropriate signature as required by theRevertableinterface.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the Revertable interface implementation in TestUniversalContract # Test 1: Check if TestUniversalContract implements all methods from Revertable echo "Checking Revertable interface implementation:" ast-grep --lang solidity --pattern 'interface Revertable { $$$ }' echo "TestUniversalContract implementation:" ast-grep --lang solidity --pattern 'contract TestUniversalContract is UniversalContract, Revertable { $$$ }' # Test 2: Verify onRevert function signature echo "Verifying onRevert function signature:" ast-grep --lang solidity --pattern 'function onRevert(RevertContext calldata revertContext) external'Length of output: 906
Script:
#!/bin/bash # Description: Verify the Revertable interface implementation in TestUniversalContract using ripgrep # Test 1: Check if Revertable interface exists echo "Searching for Revertable interface definition:" rg 'interface\s+Revertable\s*\{' # Test 2: Check if TestUniversalContract inherits from Revertable echo "Checking TestUniversalContract inheritance:" rg 'contract\s+TestUniversalContract\s+is\s+UniversalContract,\s+Revertable\s*\{' # Test 3: Verify onRevert function signature echo "Verifying onRevert function signature in TestUniversalContract:" rg 'function\s+onRevert\s*\(\s*RevertContext\s+calldata\s+revertContext\s*\)\s+external'Length of output: 1137
v2/contracts/evm/interfaces/IGatewayEVM.sol (2)
Line range hint
1-88: Summary: Minimal, focused change improving revert handling.The addition of the
NotAllowedToCallOnReverterror to theIGatewayEVMErrorsinterface is a targeted improvement that aligns well with the PR objectives. This change:
- Enhances error handling capabilities for the GatewayEVM.
- Maintains consistency with existing error definitions.
- Doesn't introduce breaking changes to the interface.
The modification is minimal yet impactful, providing a clear mechanism to prevent arbitrary calls from invoking the
onRevertfunction. This change contributes to a more robust and secure implementation of the GatewayEVM.
87-88: Verify usage of the new error in the implementation.The new
NotAllowedToCallOnReverterror has been added to theIGatewayEVMErrorsinterface. To ensure its effectiveness, we should verify that it's being used appropriately in the GatewayEVM implementation.Let's check the implementation file for the usage of this new error:
This script will help us confirm that the new error is being used in the implementation, ensuring that the interface change is fully utilized.
✅ Verification successful
Further verification needed to locate
NotAllowedToCallOnRevertusage correctly.
Further verification needed to locate
NotAllowedToCallOnRevertusage correctly.
Usage of
NotAllowedToCallOnRevertverified in the implementation.
- The error is declared in
IGatewayEVM.sol.- It is properly used in
GatewayEVM.solwith arevertstatement.- Tests in
GatewayEVM.t.solcorrectly expect this specific revert.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for the usage of NotAllowedToCallOnRevert error in the implementation file. # Find the GatewayEVM implementation file implementation_file=$(fd -e sol -t f GatewayEVM) if [ -z "$implementation_file" ]; then echo "GatewayEVM implementation file not found." exit 1 fi # Search for the usage of NotAllowedToCallOnRevert echo "Searching for NotAllowedToCallOnRevert usage in $implementation_file:" rg "NotAllowedToCallOnRevert" "$implementation_file" # If not found, it might be using a require statement instead if [ $? -ne 0 ]; then echo "NotAllowedToCallOnRevert not found. Searching for related require statements:" rg "require\(.*onRevert.*\)" "$implementation_file" fiLength of output: 3318
Script:
#!/bin/bash # Description: Search for the usage of NotAllowedToCallOnRevert error across all Solidity files. # Search for the usage of NotAllowedToCallOnRevert in all .sol files echo "Searching for NotAllowedToCallOnRevert usage in the codebase:" rg "NotAllowedToCallOnRevert" --type sol # If not found, search for related require statements handling onRevert if [ $? -ne 0 ]; then echo "NotAllowedToCallOnRevert not found. Searching for related require statements:" rg "require\(.*onRevert.*\)" --type sol fiLength of output: 454
Script:
#!/bin/bash # Description: Search for the usage of NotAllowedToCallOnRevert error across all Solidity files. # Search for the usage of NotAllowedToCallOnRevert in all .sol files echo "Searching for NotAllowedToCallOnRevert usage in the codebase:" rg "NotAllowedToCallOnRevert" -g "*.sol" # If not found, search for related require statements handling onRevert if [ $? -ne 0 ]; then echo "NotAllowedToCallOnRevert not found. Searching for related require statements:" rg "require\(.*onRevert.*\)" -g "*.sol" fiLength of output: 441
v2/test/ZetaConnectorNonNative.t.sol (2)
Line range hint
1-424: Overall assessment: Change aligns with PR objectivesThe modification to add the 'sender' field to the RevertContext in the setUp() function aligns well with the PR objective of enhancing revert handling. The change is minimal and focused, which is a positive aspect of this update.
However, to ensure the robustness of this change:
- Verify that all existing tests still pass with this modification.
- Consider adding or updating tests that specifically validate the new 'sender' field in RevertContext, particularly in the testWithdrawAndRevert function.
- Ensure that this change is consistently applied across all relevant test files if there are multiple test files for related components.
79-79: LGTM: RevertContext updated with sender informationThe addition of the 'sender' field to the RevertContext is consistent with the PR objectives and enhances the context provided during revert scenarios. This change appropriately sets the sender to the 'owner' address in the test setup.
To ensure this change doesn't inadvertently affect existing tests, please run the following verification:
This script will run the tests, check for any failures, and highlight any changes in RevertContext usage, which might indicate unintended side effects of this change.
v2/contracts/zevm/GatewayZEVM.sol (3)
6-6: LGTM: Import statement updated to include Revertable interfaceThis change aligns with the PR objective of improving revert handling by introducing the Revertable interface. It's a good practice to import only the necessary components.
477-477: LGTM: Updated executeRevert to use Revertable interfaceThis change is consistent with the PR objective of removing revert functionality from the UniversalContract interface. Using the Revertable interface improves the separation of concerns and makes the contract more modular.
500-500: LGTM: Updated depositAndRevert to use Revertable interfaceThis change is consistent with the executeRevert function modification and aligns with the PR objectives. It maintains consistency across the contract in handling revert operations using the Revertable interface.
v2/test/ERC20Custody.t.sol (1)
80-80: LGTM. Consider verifying the impact of this change.The addition of the
senderfield to theRevertContextstructure is a good improvement, providing more context during revert scenarios. This change aligns with the PR objectives.To ensure this change doesn't introduce any issues, please run the following script to check for any other usages of
RevertContextthat might need updating:v2/test/GatewayZEVM.t.sol (1)
560-560: LGTM. Verify impact on related tests.The addition of the
senderfield to therevertContextinitialization is consistent with the reported changes. This update enhances the context available during revert scenarios, which is a positive improvement.To ensure this change doesn't negatively impact existing tests, please run the following script:
v2/contracts/evm/GatewayEVM.sol (1)
409-409: Ensure security checks are effective in_executeArbitraryCallThe addition of
revertIfOnCallOrOnRevert(data);strengthens security by preventing unauthorized calls toonCallandonRevert. Confirm that this function adequately protects against malicious attempts to invoke restricted functions through arbitrary calls.To validate the effectiveness of the security check, run the following script to search for all invocations of
_executeArbitraryCalland ensure that the parameters and contexts are appropriate:✅ Verification successful
Security checks in
_executeArbitraryCallare properly implemented.
- Verified that
revertIfOnCallOrOnRevert(data);is invoked within_executeArbitraryCall.- No unauthorized calls detected in usage contexts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `_executeArbitraryCall` across the codebase. # Test: Search for all calls to `_executeArbitraryCall`. Expect: Proper usage with necessary security checks. rg --type solidity --word-regexp '_executeArbitraryCall'Length of output: 89
Script:
#!/bin/bash # Description: Verify the usage of `_executeArbitraryCall` across Solidity files in the codebase. rg --word-regexp '_executeArbitraryCall' --glob '*.sol'Length of output: 718
Script:
#!/bin/bash # Description: Retrieve context around each `_executeArbitraryCall` invocation in GatewayEVM.sol to verify security checks. rg '_executeArbitraryCall' v2/contracts/evm/GatewayEVM.sol -C 3Length of output: 1036
v2/test/GatewayEVM.t.sol (2)
79-79: Initialization ofrevertContextwith newsenderfieldThe
revertContextis now initialized with the newsenderfield set toowner. This change aligns with the updatedRevertContextstructure, providing additional context during revert scenarios.
215-221: Addition of testtestForwardCallToReceiveOnRevertFailsThe new test function correctly verifies that unauthorized calls to
onRevertrevert withNotAllowedToCallOnRevert.selector, ensuring that theonRevertfunction cannot be called arbitrarily. This is consistent with the PR objective to enhance revert handling and security.
| @@ -474,7 +474,7 @@ contract GatewayZEVM is | |||
| function executeRevert(address target, RevertContext calldata revertContext) external onlyProtocol whenNotPaused { | |||
| if (target == address(0)) revert ZeroAddress(); | |||
|
|
|||
| UniversalContract(target).onRevert(revertContext); | |||
| Revertable(target).onRevert(revertContext); | |||
There was a problem hiding this comment.
Using onRevert() in the GatewayEVm seems to be missing here, but looking at the state of the main branch it seems to have been merged in another PR.
base is auth call branch, please dont merge until that one is merged
Summary by CodeRabbit
New Features
onRevertmethod.Bug Fixes
Tests