feat: add sender when calling connected chain contracts from zetachain #355
Conversation
WalkthroughWalkthroughThis pull request introduces significant modifications to 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 #355 +/- ##
==========================================
+ Coverage 91.39% 91.92% +0.52%
==========================================
Files 8 8
Lines 337 359 +22
Branches 110 117 +7
==========================================
+ Hits 308 330 +22
Misses 29 29 ☔ View full report in Codecov by Sentry. |
skosito
changed the title
skosito
requested review from
andresaiello,
brewmaster012,
lumtis,
CharlieMc0,
fadeev and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Outside diff range and nitpick comments (10)
go.mod (1)
Line range hint
1-26: Address the overall trend of dependency downgradesThere's a noticeable trend of downgrading multiple dependencies in this update. While each individual downgrade may have its reasons, this overall pattern raises some concerns:
- It may introduce technical debt by using older versions of libraries.
- The project might miss out on important bug fixes, security updates, and performance improvements.
- It could make future upgrades more challenging as the gap between the used versions and the latest versions widens.
Consider the following recommendations:
- Document the specific reasons for each downgrade to maintain institutional knowledge.
- Create a plan for gradually updating these dependencies in the future to reduce technical debt.
- Implement a dependency management strategy that balances stability with staying up-to-date.
- Regularly review and test against newer versions of these dependencies to ensure easier future upgrades.
v2/test/utils/ReceiverEVM.sol (1)
76-85: LGTM: NewonCallfunction added, with suggestions for improvement.The
onCallfunction is correctly implemented as anexternal payablefunction, matching the expected interface. However, consider the following suggestions:
- The function doesn't use its input parameters. Consider adding logic to process
messageContextandmessage.- There's no check or use of
msg.value. If this is intentional for testing, consider adding a comment explaining this.- The function returns
bytes memorybut doesn't return any data. Consider either removing the return type if not needed or returning some meaningful data.Example improvement:
function onCall( MessageContext calldata messageContext, bytes calldata message ) external payable returns (bytes memory) { // Process message context and message // For example, you could return the message as-is emit ReceivedOnCall(messageContext.sender, msg.value, message); return message; }v2/test/GatewayEVMZEVM.t.sol (2)
137-144: LGTM: Improved event emission with CallOptions structThe update to the emit statement enhances code clarity by using a CallOptions struct. This change likely reflects modifications in the contract's event structure and improves maintainability.
Consider extracting the CallOptions struct creation to a separate variable for improved readability:
+CallOptions memory callOptions = CallOptions({ gasLimit: 1, isArbitraryCall: true }); emit Called( address(ownerZEVM), address(zrc20), abi.encodePacked(receiverEVM), message, - CallOptions({ gasLimit: 1, isArbitraryCall: true }), + callOptions, revertOptions );This change would make the emit statement cleaner and allow for easier reuse of the CallOptions if needed elsewhere in the test.
204-204: LGTM: Consistent use of CallOptions struct in withdrawAndCallThe changes in this function are consistent with the previous modifications, using the CallOptions struct in both the event emission and the gatewayZEVM.withdrawAndCall function call. This ensures consistency with the updated contract interface.
For consistency with the previous suggestion and to improve readability, consider extracting the CallOptions struct creation to a separate variable:
+CallOptions memory callOptions = CallOptions({ gasLimit: 1, isArbitraryCall: true }); emit Withdrawn( ownerZEVM, 0, abi.encodePacked(receiverEVM), address(zrc20), 500_000, expectedGasFee, zrc20.PROTOCOL_FLAT_FEE(), message, - CallOptions({ gasLimit: 1, isArbitraryCall: true }), + callOptions, revertOptions ); vm.prank(ownerZEVM); gatewayZEVM.withdrawAndCall( abi.encodePacked(receiverEVM), 500_000, address(zrc20), message, - CallOptions({ gasLimit: 1, isArbitraryCall: true }), + callOptions, revertOptions );This change would improve consistency and make the code more readable and maintainable.
Also applies to: 208-215
v2/contracts/zevm/interfaces/IGatewayZEVM.sol (2)
15-15: Typo in 'arbitrary' in documentationThe word "arbirtrary" is misspelled in multiple comments. It should be corrected to "arbitrary".
Apply the following diff to fix the typos:
-/// @param callOptions Call options including gas limit and arbirtrary call flag. +/// @param callOptions Call options including gas limit and arbitrary call flag.Also applies to: 35-35, 142-142, 174-174, 286-286
288-290: Consider enhancing theCallOptionsstruct documentationTo improve clarity, consider providing more detailed descriptions for the
CallOptionsstruct fields.Update the comments as follows:
/// @notice CallOptions struct passed to call and withdrawAndCall functions. /// @param gasLimit Gas limit for the cross-chain call execution. -/// @param isArbitraryCall Indicates if call should be arbitrary or authenticated. +/// @param isArbitraryCall Indicates whether the call is arbitrary (true) or authenticated (false). struct CallOptions { uint256 gasLimit; bool isArbitraryCall; }v2/contracts/evm/GatewayEVM.sol (1)
126-128: Redundant variable initializationIn the first
executefunction, the variableresultis declared and then immediately assigned.You can simplify the code by combining declaration and assignment:
-bytes memory result; -result = _executeAuthenticatedCall(messageContext, destination, data); +bytes memory result = _executeAuthenticatedCall(messageContext, destination, data);v2/test/GatewayZEVM.t.sol (3)
76-76: Consider using a realisticgasLimitincallOptions.In the
setUpfunction,callOptionsis initialized withgasLimit: 1. For more accurate testing, consider settinggasLimitto a value that reflects actual expected gas usage in real scenarios.
Line range hint
277-277: Correct the function name for clarity.The function
testWithdrawAndCallZETAFailsIfAmountIsReceiverIsZeroAddresshas a naming issue. It should likely be renamed totestWithdrawAndCallZETAFailsIfReceiverIsZeroAddressto accurately describe the test case.
287-287: Rename function to reflect the test scenario accurately.The function
testWithdrawAndCallZETAWithCallOptsFailsIfAmountIsReceiverIsZeroAddressappears to have a typo. Consider renaming it totestWithdrawAndCallZETAWithCallOptsFailsIfReceiverIsZeroAddressfor clarity.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (56)
go.sumis excluded by!**/*.sumv2/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/igatewayevm.sol/callable.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/igatewayzevm.sol/igatewayzevm.gois excluded by!v2/pkg/**v2/pkg/igatewayzevm.sol/igatewayzevmevents.gois excluded by!v2/pkg/**v2/pkg/ireceiverevm.sol/ireceiverevmevents.gois excluded by!v2/pkg/**v2/pkg/receiverevm.sol/receiverevm.gois excluded by!v2/pkg/**v2/pkg/senderzevm.sol/senderzevm.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/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/IGatewayEVM.sol/Callable.tsis excluded by!v2/types/**v2/types/IGatewayEVM.sol/IGatewayEVM.tsis excluded by!v2/types/**v2/types/IGatewayEVM.sol/index.tsis excluded by!v2/types/**v2/types/IGatewayZEVM.sol/IGatewayZEVM.tsis excluded by!v2/types/**v2/types/IGatewayZEVM.sol/IGatewayZEVMEvents.tsis excluded by!v2/types/**v2/types/IReceiverEVM.sol/IReceiverEVMEvents.tsis excluded by!v2/types/**v2/types/ReceiverEVM.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/IGatewayEVM.sol/Callable__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayEVM.sol/IGatewayEVMErrors__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayEVM.sol/IGatewayEVM__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayEVM.sol/index.tsis excluded by!v2/types/**v2/types/factories/IGatewayZEVM.sol/IGatewayZEVMEvents__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/ReceiverEVM__factory.tsis excluded by!v2/types/**v2/types/factories/SenderZEVM__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/**v2/types/index.tsis excluded by!v2/types/**
Files selected for processing (11)
- go.mod (1 hunks)
- v2/contracts/evm/GatewayEVM.sol (5 hunks)
- v2/contracts/evm/interfaces/IGatewayEVM.sol (3 hunks)
- v2/contracts/zevm/GatewayZEVM.sol (6 hunks)
- v2/contracts/zevm/interfaces/IGatewayZEVM.sol (6 hunks)
- v2/test/GatewayEVM.t.sol (4 hunks)
- v2/test/GatewayEVMZEVM.t.sol (3 hunks)
- v2/test/GatewayZEVM.t.sol (9 hunks)
- v2/test/utils/GatewayEVMUpgradeTest.sol (4 hunks)
- v2/test/utils/IReceiverEVM.sol (1 hunks)
- v2/test/utils/ReceiverEVM.sol (2 hunks)
Additional comments not posted (26)
go.mod (6)
12-12: Confirm rationale for go-ole/go-ole downgradeThe github.com/go-ole/go-ole package has been downgraded from v1.3.0 to v1.2.5. While this is a minor version downgrade, it may remove bug fixes or small features introduced in later 1.2.x or 1.3.0 versions.
Please confirm:
- The reason for this specific downgrade.
- That any bug fixes or features introduced in versions 1.2.6 to 1.3.0 are not critical for your use case.
- This change doesn't introduce any compatibility issues with other dependencies or parts of the system.
16-16: Confirm rationale for holiman/uint256 downgradeThe github.com/holiman/uint256 package has been downgraded from v1.2.4 to v1.2.3. While this is a patch version downgrade and unlikely to introduce breaking changes, it may remove bug fixes or optimizations introduced in v1.2.4.
Please confirm:
- The specific reason for downgrading to v1.2.3.
- That any bug fixes or optimizations introduced in v1.2.4 are not critical for your use case.
- This change aligns with the project's dependency management strategy.
23-23: Verify rationale for golang.org/x/sys downgradeThe golang.org/x/sys package has been downgraded from v0.16.0 to v0.15.0. While this is a minor version downgrade, it may remove features or optimizations introduced in v0.16.0.
Please confirm:
- The specific reason for downgrading to v0.15.0.
- That this downgrade is compatible with the Go version used in the project.
- That any features or optimizations introduced in v0.16.0 are not critical for your use case.
- This change doesn't introduce any compatibility issues with other dependencies or parts of the system.
10-10: Verify compatibility with downgraded golang-set versionThe github.com/deckarep/golang-set package has been downgraded from v2.1.0 to v1.8.0. This is a major version change that could potentially break existing code relying on v2 features.
Please ensure that:
- The codebase has been updated to use the v1 API of golang-set.
- There are no remaining references to v2-specific features or syntax.
- This downgrade doesn't introduce any compatibility issues with other parts of the system.
To help verify the impact, you can run the following script:
#!/bin/bash # Description: Check for usage of golang-set v2 features # Test: Search for potential usage of v2 APIs rg --type go "mapset\.New\w*Set"
17-17: Clarify the purpose of adding rjeczalik/notifyA new dependency, github.com/rjeczalik/notify v0.9.1, has been added to the project.
Please provide information on:
- The specific use case or functionality that requires this new dependency.
- Any potential impact on the project's performance or complexity.
- Whether this addition aligns with the project's overall architecture and design principles.
To help understand the usage, you can run the following script:
#!/bin/bash # Description: Check for usage of rjeczalik/notify in the codebase # Test: Search for import and usage of notify package rg --type go "github.com/rjeczalik/notify"
5-5: Verify the rationale for downgrading ethereum/go-ethereumThe ethereum/go-ethereum package has been significantly downgraded from v1.13.15 to v1.10.26. This is a major version change that could potentially introduce compatibility issues or remove features that the project might be relying on.
Please provide the rationale for this downgrade and confirm that:
- The project doesn't rely on any features introduced in versions 1.11-1.13.
- This change doesn't introduce any compatibility issues with other dependencies or parts of the system.
- The team is aware of any security fixes or important updates that might be missed by using this older version.
To help verify the impact, you can run the following script:
v2/test/utils/IReceiverEVM.sol (1)
39-40: New eventReceivedOnCall()added: Consider adding documentationThe addition of the
ReceivedOnCall()event is noted. This event appears to signal when a call is received, which aligns with the purpose of the interface. However, to maintain consistency with other events in this interface, consider adding a documentation comment explaining its purpose and when it's emitted.Consider adding a documentation comment for the new event, similar to the existing ones:
+ /// @notice Emitted when a call is received. event ReceivedOnCall();Also, verify if any implementations of this interface need to be updated to emit this new event where appropriate.
Verification successful
Verified:
ReceivedOnCall()event added with appropriate documentationThe
ReceivedOnCall()event has been successfully added without affecting existing implementations, as no contracts currently implementIReceiverEVMEvents. Ensure that future implementations of this interface emit theReceivedOnCall()event as appropriate.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for implementations of IReceiverEVMEvents and verify if they need updates # Search for files implementing IReceiverEVMEvents echo "Files potentially implementing IReceiverEVMEvents:" rg --type solidity -l "contract.*implements.*IReceiverEVMEvents" # Search for existing emit statements of ReceivedOnCall echo "\nExisting emit statements of ReceivedOnCall:" rg --type solidity "emit ReceivedOnCall()"Length of output: 389
Script:
#!/bin/bash # Description: Corrected script to check for implementations of IReceiverEVMEvents and verify if they need updates # Search for files implementing IReceiverEVMEvents echo "Files potentially implementing IReceiverEVMEvents:" rg "contract.*implements.*IReceiverEVMEvents" --glob "*.sol" -l # Search for existing emit statements of ReceivedOnCall echo "\nExisting emit statements of ReceivedOnCall:" rg "emit ReceivedOnCall()" --glob "*.sol"Length of output: 438
v2/test/utils/ReceiverEVM.sol (1)
5-5: LGTM: New import statement is correctly added.The import of
MessageContextis necessary for the newonCallfunction and is correctly placed.v2/test/GatewayEVMZEVM.t.sol (2)
16-16: LGTM: Import statements improvedThe changes to the import statements enhance code organization and readability:
- Using named import for SystemContractMock (line 16) clarifies which specific component is being imported.
- Consolidating related imports (line 20) reduces the number of import statements and groups related components together.
These modifications align with best practices for managing imports in Solidity.
Also applies to: 20-20
Line range hint
1-268: Overall assessment: Consistent and well-implemented changesThe modifications in this test file are consistent and well-implemented throughout. The main changes include:
- Improved import statements for better code organization.
- Introduction of the CallOptions struct in relevant function calls and event emissions.
These changes likely reflect updates in the contract interfaces and improve the overall readability and maintainability of the test code. The uniform application of the CallOptions struct ensures consistency across different test functions.
To further enhance the code:
- Consider implementing the suggested extractions of CallOptions struct creation to separate variables for improved readability.
- Ensure that the changes in this test file accurately reflect the modifications made in the actual contract implementations.
- Verify that all test cases still pass with these modifications and consider adding new test cases if the contract behavior has been extended.
v2/contracts/evm/interfaces/IGatewayEVM.sol (4)
85-85: Approved: Added new errorNotAllowedToCallOnCall()The new error definition is appropriate and follows the existing naming conventions in
IGatewayEVMErrors.
117-130: Approved: Added overloadedexecutefunction withMessageContextThe addition of the overloaded
executefunction enhances functionality by allowing calls with sender context. The function signature and modifiers are appropriate.
193-197: Approved: AddedMessageContextstructThe incorporation of the
MessageContextstruct improves the ability to pass sender information through function calls. The struct is properly defined.
200-202: Approved: AddedCallableinterface withonCallfunctionThe introduction of the
Callableinterface standardizes theonCallfunction implementation for contracts receiving authenticated calls. The function signature is clear and adheres to best practices.v2/contracts/zevm/interfaces/IGatewayZEVM.sol (2)
137-152: Ensure function overloading works as intendedThe addition of overloaded
withdrawAndCallfunctions introduces new function signatures. Ensure that overloading does not cause ambiguity and that all calls resolve to the correct function.Confirm that:
- The Solidity compiler version used supports function overloading as intended.
- The function signatures are unique and do not conflict with existing functions.
- Documentation is updated to clearly differentiate between the overloaded functions.
Also applies to: 169-184
186-199: Verify the newcallfunction integrationThe new
callfunction allows calling contracts on external chains without asset transfer. Ensure that:Run the following script to verify that there are no naming conflicts and that the function is properly declared:
Verification successful
✅ Verified: No naming conflicts found with the
callfunction integration.
- The
callfunction is properly declared inIGatewayZEVM.solwithout conflicting with othercallfunctions in different contracts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any function naming conflicts with `call`. # Test: Search for `function call(` declarations. Expect: No conflicts. ast-grep --lang solidity --pattern $'function call($_, $_, $_, $_, $_)$_' --ignore "v2/contracts/zevm/interfaces/IGatewayZEVM.sol"Length of output: 246
Script:
#!/bin/bash # Description: Check for any function naming conflicts with `call` in Solidity files, excluding IGatewayZEVM.sol. # Search for `function call(` declarations in Solidity files. rg 'function\s+call\s*\(' --type solidity --glob '!v2/contracts/zevm/interfaces/IGatewayZEVM.sol'Length of output: 132
Script:
#!/bin/bash # Description: Check for any function naming conflicts with `call` in Solidity files, excluding IGatewayZEVM.sol. # Search for `function call(` declarations in Solidity files, excluding the specified interface file. rg 'function\s+call\s*\(' -g '*.sol' -g '!v2/contracts/zevm/interfaces/IGatewayZEVM.sol'Length of output: 458
v2/contracts/evm/GatewayEVM.sol (4)
Line range hint
107-132: Addition ofexecutefunction withMessageContextparameterThe introduction of the new
executefunction that accepts aMessageContextand calls_executeAuthenticatedCallenhances the contract's ability to handle authenticated calls. The use ofnonReentrant,onlyRole(TSS_ROLE), andwhenNotPausedmodifiers ensures security and proper access control.
134-153: Overloading ofexecutefunction for arbitrary callsThe added overload of the
executefunction allows for arbitrary calls without authentication. It correctly calls_executeArbitraryCalland emits theExecutedevent upon success.
149-153: Verify access control for the overloadedexecutefunctionEnsure that the
onlyRole(TSS_ROLE)modifier sufficiently restricts access to theexecutefunction to prevent unauthorized usage.Please confirm that only the intended entities have the
TSS_ROLEassigned.Verification successful
Adjusting the verification script to correctly target Solidity files.
Access control for the
executefunction is properly restricted by theonlyRole(TSS_ROLE)modifier.
- The
TSS_ROLEis assigned exclusively totssAddress_in the following files:
v2/test/utils/GatewayEVMUpgradeTest.solv2/contracts/evm/GatewayEVM.solv2/contracts/evm/ZetaConnectorBase.solScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify entities with TSS_ROLE. # Test: List all assignments of TSS_ROLE. rg --type solidity -A 3 $'_grantRole(TSS_ROLE, $_);'Length of output: 86
Script:
#!/bin/bash # Description: Verify entities with TSS_ROLE by searching .sol files. # Test: List all assignments of TSS_ROLE in Solidity files. rg '_grantRole\(TSS_ROLE,\s*[^)]+\);' --glob '*.sol'Length of output: 294
408-414: Ensure safe execution in_executeArbitraryCallWhile
_executeArbitraryCalleffectively calls the destination with provided data, it's important to consider the impact of allowing arbitrary external calls. Ensure that there are no security vulnerabilities introduced by this function, such as reentrancy attacks or unauthorized access.Please run the following script to check for potential reentrancy vulnerabilities:
v2/contracts/zevm/GatewayZEVM.sol (1)
4-4: Approved: Import statement correctly includes CallOptions and IGatewayZEVMThe import statement correctly brings in
CallOptionsandIGatewayZEVMfrom the specified file, which are necessary for the new functionality.v2/test/GatewayEVM.t.sol (3)
223-229: Confirm zero address handling inexecutewithMessageContextThe test
testExecuteWithMsgContextFailsIfDestinationIsZeroAddresscorrectly verifies that callinggateway.executewith a zero destination address and aMessageContextreverts with aZeroAddresserror. Ensure that the contract'sexecutefunction implements this check appropriately.You can verify the presence of the zero address check with the following script:
140-147: Verify the correctness of the sender address inMessageContextIn this test function
testForwardCallToReceiveOnCallUsingAuthCall, you are invokinggateway.executewith aMessageContextwhere thesenderis set toaddress(0x123). Please verify thataddress(0x123)is the intended sender for this test case and that it aligns with the expected behavior of theexecutefunction when using authenticated calls.You can confirm the intended usage with the following script:
Verification successful
Verified: Correctness of sender address in
MessageContextThe sender address
address(0x123)in theMessageContextis correctly set and aligns with the intended behavior of theexecutefunction for this test case.
- File:
v2/test/GatewayEVM.t.sol- Function:
testForwardCallToReceiveOnCallUsingAuthCallScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure that the sender address in MessageContext is correctly used in gateway.execute calls. # Search for all instances where gateway.execute is called with MessageContext rg 'gateway\.execute\(\s*MessageContext\(\{\s*sender\s*:\s*address\(0x[0-9a-fA-F]+\)\s*\}\),' # Expected Result: Verify that the sender address used here matches the intended test scenarios.Length of output: 287
Script:
#!/bin/bash # Description: Search for gateway.execute calls with MessageContext and extract sender addresses. # Search for all instances where gateway.execute is called with MessageContext and extract the sender addresses rg 'gateway\.execute\(\s*MessageContext\s*\(\s*\{\s*sender\s*:\s*address\(0x[0-9a-fA-F]+\)\s*\}\s*\)\s*,\s*address\([^)]+\)\s*,\s*bytes\([^)]+\)\s*\)'Length of output: 277
162-173: Ensure consistent error handling for unauthorized accessThe test
testForwardCallToReceiveNonPayableWithMsgContextFailsIfSenderIsNotTSSchecks thatgateway.executereverts when called by an unauthorized sender withMessageContext. This is correctly implemented. Ensure that the revert messageAccessControlUnauthorizedAccountmatches the contract's access control mechanisms.You can verify the access control enforcement with:
v2/test/GatewayZEVM.t.sol (2)
310-312: Verify the use ofgasLimit: 0inCallOptions.In the
emit Withdrawnstatement oftestWithdrawZETA, theCallOptionsis set withgasLimit: 0. Ensure that settinggasLimitto zero is intentional and does not lead to unexpected behavior during execution.
369-378: ConfirmgasLimit: 0inCallOptionsfortestWithdrawZETAWithMessage.Again,
CallOptionsis configured withgasLimit: 0. Please verify that a zero gas limit is appropriate for this test and won't affect the functionality negatively.
| @@ -111,6 +114,21 @@ interface IGatewayEVM is IGatewayEVMErrors, IGatewayEVMEvents { | |||
| /// @return The result of the contract call. | |||
| function execute(address destination, bytes calldata data) external payable returns (bytes memory); | |||
|
|
|||
| /// @notice Executes a call to a destination address without ERC20 tokens. | |||
| /// @dev This function can only be called by the TSS address and it is payable. | |||
| /// @param messageContext Message context containing sender and arbitrary call flag. | |||
There was a problem hiding this comment.
Inconsistent documentation for messageContext parameter
The documentation for the messageContext parameter states: "Message context containing sender and arbitrary call flag." However, the MessageContext struct only contains the sender field without an arbitrary call flag.
To resolve this inconsistency, please update the documentation or include the arbitraryCall flag in the MessageContext struct.
Option 1: Update the documentation
- /// @param messageContext Message context containing sender and arbitrary call flag.
+ /// @param messageContext Message context containing sender information.Option 2: Add arbitraryCall flag to MessageContext struct
struct MessageContext {
address sender;
+ bool arbitraryCall;
}Please choose the appropriate option based on the intended functionality.
Committable suggestion was skipped due to low confidence.
| CallOptions callOptions, | ||
| RevertOptions revertOptions | ||
| ); |
There was a problem hiding this comment.
Changing event parameters affects event signatures
Replacing the uint256 gasLimit parameter with CallOptions callOptions in the Called and Withdrawn events changes their event signatures. This may impact external contracts or systems that rely on these events.
Consider the implications of altering event signatures:
- Compatibility: External listeners or dApps that decode these events may fail or produce incorrect results due to the changed parameters.
- Migration Plan: If backward compatibility is required, consider emitting new events with different names or maintaining the old events alongside the new ones.
Also applies to: 46-48
| @@ -163,7 +178,7 @@ contract GatewayEVM is | |||
| if (!resetApproval(token, to)) revert ApprovalFailed(); | |||
| if (!IERC20(token).approve(to, amount)) revert ApprovalFailed(); | |||
| // Execute the call on the target contract | |||
| _execute(to, data); | |||
| _executeArbitraryCall(to, data); | |||
There was a problem hiding this comment.
Handle return values in executeWithERC20
In the executeWithERC20 function, the result of the _executeArbitraryCall is not captured or handled.
Consider capturing the result for logging or further processing:
// Existing code
_executeArbitraryCall(to, data);
// Suggested change
bytes memory result = _executeArbitraryCall(to, data);This allows you to use the result if needed.
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.
| _executeArbitraryCall(to, data); | |
| bytes memory result = _executeArbitraryCall(to, data); |
| return Callable(destination).onCall{ value: msg.value }(messageContext, data); | ||
| } |
There was a problem hiding this comment.
Handle potential failures in _executeAuthenticatedCall
The function _executeAuthenticatedCall calls onCall on the destination contract but does not handle the scenario where the call fails.
Consider adding a check to ensure the call was successful:
-function _executeAuthenticatedCall(
- MessageContext calldata messageContext,
- address destination,
- bytes calldata data
-) private returns (bytes memory) {
- return Callable(destination).onCall{ value: msg.value }(messageContext, data);
-}
+function _executeAuthenticatedCall(
+ MessageContext calldata messageContext,
+ address destination,
+ bytes calldata data
+) private returns (bytes memory) {
+ bytes memory result = Callable(destination).onCall{ value: msg.value }(messageContext, data);
+ // Handle failure if necessary
+ return result;
+}Alternatively, you can use a low-level call to capture the success status:
function _executeAuthenticatedCall(
MessageContext calldata messageContext,
address destination,
bytes calldata data
) private returns (bytes memory) {
(bool success, bytes memory result) = address(destination).call{ value: msg.value }(
abi.encodeWithSelector(Callable.onCall.selector, messageContext, data)
);
if (!success) revert ExecutionFailed();
return result;
}This ensures that any failure during the call is appropriately handled.
Committable suggestion was skipped due to low confidence.
| if (destination == address(0)) revert ZeroAddress(); | ||
| bytes memory result = _executeArbitraryCall(destination, data); | ||
|
|
||
| emit Executed(destination, msg.value, data); | ||
|
|
There was a problem hiding this comment.
Missing nonReentrant modifier in overloaded execute function
The overloaded execute function for arbitrary calls does not have the nonReentrant modifier, which could expose the contract to reentrancy attacks.
Add the nonReentrant modifier to the function:
function execute(
address destination,
bytes calldata data
)
external
payable
onlyRole(TSS_ROLE)
whenNotPaused
+ nonReentrant
returns (bytes memory)
{
// Function body
}This adds an extra layer of security against reentrancy.
Committable suggestion was skipped due to low confidence.
| /// @param amount The amount of tokens to withdraw. | ||
| /// @param zrc20 The address of the ZRC20 token. | ||
| /// @param message The calldata to pass to the contract call. | ||
| /// @param callOptions Call options including gas limit and arbirtrary call flag. |
There was a problem hiding this comment.
Correct misspelling in documentation comments
The word "arbirtrary" is misspelled in the documentation comments. It should be "arbitrary".
Apply the following diffs to fix the typo:
At line 201:
- /// @param callOptions Call options including gas limit and arbirtrary call flag.
+ /// @param callOptions Call options including gas limit and arbitrary call flag.At line 305:
- /// @param callOptions Call options including gas limit and arbirtrary call flag.
+ /// @param callOptions Call options including gas limit and arbitrary call flag.At line 332:
- /// @param callOptions Call options including gas limit and arbirtrary call flag.
+ /// @param callOptions Call options including gas limit and arbitrary call flag.Also applies to: 305-305, 332-332
| /// @notice Withdraw ZRC20 tokens and call a smart contract on an external chain. | ||
| /// @param receiver The receiver address on the external chain. | ||
| /// @param amount The amount of tokens to withdraw. | ||
| /// @param zrc20 The address of the ZRC20 token. | ||
| /// @param message The calldata to pass to the contract call. | ||
| /// @param callOptions Call options including gas limit and arbirtrary call flag. | ||
| /// @param revertOptions Revert options. | ||
| function withdrawAndCall( | ||
| bytes memory receiver, | ||
| uint256 amount, | ||
| address zrc20, | ||
| bytes calldata message, | ||
| CallOptions calldata callOptions, | ||
| RevertOptions calldata revertOptions | ||
| ) | ||
| external | ||
| nonReentrant | ||
| whenNotPaused | ||
| { | ||
| if (receiver.length == 0) revert ZeroAddress(); | ||
| if (amount == 0) revert InsufficientZRC20Amount(); | ||
|
|
||
| uint256 gasFee = _withdrawZRC20WithGasLimit(amount, zrc20, callOptions.gasLimit); | ||
| emit Withdrawn( | ||
| msg.sender, | ||
| 0, | ||
| receiver, | ||
| zrc20, | ||
| amount, | ||
| gasFee, | ||
| IZRC20(zrc20).PROTOCOL_FLAT_FEE(), | ||
| message, | ||
| callOptions, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to reduce code duplication
Multiple functions such as withdrawAndCall and call have overloaded versions with similar code structures. Consider refactoring these functions to reduce code duplication, improve maintainability, and adhere to the DRY (Don't Repeat Yourself) principle.
One possible approach is to have the functions that accept a gasLimit parameter internally create a CallOptions object and then call the function that accepts CallOptions, thereby centralizing the logic.
Also applies to: 308-326, 334-345, 364-366
Add unit tests for the new functions to improve code coverage
The newly added functions withdrawAndCall and call are significant and should be thoroughly tested. Please add unit tests covering various scenarios, including edge cases, to ensure their correctness and reliability.
Also applies to: 308-326, 334-345
| function _call( | ||
| bytes memory receiver, | ||
| address zrc20, | ||
| bytes calldata message, | ||
| CallOptions memory callOptions, | ||
| RevertOptions memory revertOptions | ||
| ) | ||
| internal | ||
| { | ||
| if (receiver.length == 0) revert ZeroAddress(); | ||
| if (message.length == 0) revert EmptyMessage(); | ||
|
|
||
| (address gasZRC20, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit(gasLimit); | ||
| (address gasZRC20, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit(callOptions.gasLimit); | ||
| if (!IZRC20(gasZRC20).transferFrom(msg.sender, PROTOCOL_ADDRESS, gasFee)) { | ||
| revert GasFeeTransferFailed(); | ||
| } | ||
|
|
||
| emit Called(msg.sender, zrc20, receiver, message, gasLimit, revertOptions); | ||
| emit Called(msg.sender, zrc20, receiver, message, callOptions, revertOptions); |
There was a problem hiding this comment.
Add validation for 'zrc20' address in '_call' function
The _call function does not validate the zrc20 address. If zrc20 is the zero address, it could lead to unintended behavior or errors. Add a check to ensure that zrc20 is not the zero address.
Apply the following diff to add the validation:
function _call(
bytes memory receiver,
address zrc20,
bytes calldata message,
CallOptions memory callOptions,
RevertOptions memory revertOptions
)
internal
{
+ if (zrc20 == address(0)) revert ZeroAddress();
if (receiver.length == 0) revert ZeroAddress();
if (message.length == 0) revert EmptyMessage();
// rest of the code
}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 _call( | |
| bytes memory receiver, | |
| address zrc20, | |
| bytes calldata message, | |
| CallOptions memory callOptions, | |
| RevertOptions memory revertOptions | |
| ) | |
| internal | |
| { | |
| if (receiver.length == 0) revert ZeroAddress(); | |
| if (message.length == 0) revert EmptyMessage(); | |
| (address gasZRC20, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit(gasLimit); | |
| (address gasZRC20, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit(callOptions.gasLimit); | |
| if (!IZRC20(gasZRC20).transferFrom(msg.sender, PROTOCOL_ADDRESS, gasFee)) { | |
| revert GasFeeTransferFailed(); | |
| } | |
| emit Called(msg.sender, zrc20, receiver, message, gasLimit, revertOptions); | |
| emit Called(msg.sender, zrc20, receiver, message, callOptions, revertOptions); | |
| function _call( | |
| bytes memory receiver, | |
| address zrc20, | |
| bytes calldata message, | |
| CallOptions memory callOptions, | |
| RevertOptions memory revertOptions | |
| ) | |
| internal | |
| { | |
| if (zrc20 == address(0)) revert ZeroAddress(); | |
| if (receiver.length == 0) revert ZeroAddress(); | |
| if (message.length == 0) revert EmptyMessage(); | |
| (address gasZRC20, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit(callOptions.gasLimit); | |
| if (!IZRC20(gasZRC20).transferFrom(msg.sender, PROTOCOL_ADDRESS, gasFee)) { | |
| revert GasFeeTransferFailed(); | |
| } | |
| emit Called(msg.sender, zrc20, receiver, message, callOptions, revertOptions); |
| function testForwardCallToReceiveOnCallFails() public { | ||
| bytes memory data = abi.encodeWithSignature("onCall((address),bytes)", address(123), bytes("")); | ||
|
|
||
| vm.prank(tssAddress); | ||
| vm.expectRevert(NotAllowedToCallOnCall.selector); | ||
| gateway.execute(address(receiver), data); | ||
| } |
There was a problem hiding this comment.
Correct the abi.encodeWithSignature usage for tuple parameters
In line 208, you're encoding a function call with a tuple parameter using abi.encodeWithSignature. The syntax for encoding tuples requires careful formatting. The current code may not encode the parameters as intended.
Consider updating the code to properly encode the tuple parameter:
-bytes memory data = abi.encodeWithSignature("onCall((address),bytes)", address(123), bytes(""));
+bytes memory data = abi.encodeWithSignature("onCall((address,bytes))", (address(123), bytes("")));This change ensures that the tuple (address, bytes) is correctly encoded as a single parameter.
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 testForwardCallToReceiveOnCallFails() public { | |
| bytes memory data = abi.encodeWithSignature("onCall((address),bytes)", address(123), bytes("")); | |
| vm.prank(tssAddress); | |
| vm.expectRevert(NotAllowedToCallOnCall.selector); | |
| gateway.execute(address(receiver), data); | |
| } | |
| function testForwardCallToReceiveOnCallFails() public { | |
| bytes memory data = abi.encodeWithSignature("onCall((address,bytes))", (address(123), bytes(""))); | |
| vm.prank(tssAddress); | |
| vm.expectRevert(NotAllowedToCallOnCall.selector); | |
| gateway.execute(address(receiver), data); | |
| } |
| emit Called(owner, address(zrc20), abi.encodePacked(addr1), message, callOptions, revertOptions); | ||
| gateway.call(abi.encodePacked(addr1), address(zrc20), message, 1, revertOptions); | ||
| } |
There was a problem hiding this comment.
Inconsistent usage of callOptions in testCall function.
In the testCall function, the emit Called event uses callOptions, but the subsequent gateway.call invocation passes 1 instead of callOptions.
emit Called(owner, address(zrc20), abi.encodePacked(addr1), message, callOptions, revertOptions);
gateway.call(abi.encodePacked(addr1), address(zrc20), message, 1, revertOptions);This inconsistency may lead to unintended behavior in the test. Ensure both the event emission and the function call use callOptions for consistency.
Apply this diff to fix the inconsistency:
- gateway.call(abi.encodePacked(addr1), address(zrc20), message, 1, revertOptions);
+ gateway.call(abi.encodePacked(addr1), address(zrc20), message, callOptions, revertOptions);
| @@ -163,7 +178,7 @@ contract GatewayEVM is | |||
| if (!resetApproval(token, to)) revert ApprovalFailed(); | |||
| if (!IERC20(token).approve(to, amount)) revert ApprovalFailed(); | |||
| // Execute the call on the target contract | |||
| _execute(to, data); | |||
| _executeArbitraryCall(to, data); | |||
There was a problem hiding this comment.
Why do we only allow for arbitrary and not for authenticated calls when executing with ERC20?
| 0, | ||
| 0, | ||
| message, | ||
| CallOptions({ gasLimit: 0, isArbitraryCall: true }), |
There was a problem hiding this comment.
Won't the call always fail on the other side as we hardcode a gasLimit of 0? (Same for other occurrences)
There was a problem hiding this comment.
i think it is 0 because it is not provided in arguments and it wont fail since it can be handled on protocol side
| /// @param amount The amount of tokens to withdraw. | ||
| /// @param chainId Chain id of the external chain. | ||
| /// @param message The calldata to pass to the contract call. | ||
| /// @param callOptions Call options including gas limit and arbirtrary call flag. |
| uint256 amount, | ||
| address zrc20, | ||
| bytes calldata message, | ||
| CallOptions calldata callOptions, |
There was a problem hiding this comment.
This would, by my understanding allow for an authenticated call with an erc20 transfer on the other side. However, this is not possible as executeWithERC20() only implements the arbitrary call
There was a problem hiding this comment.
i think we can have authenticated calls for executeWithERC20, but i think initially at least we just wanted it for execute, @lumtis was this intentional or we need authenticated calls for erc20 as well?
There was a problem hiding this comment.
When you look at executeWithERC20 you can see that it only allows for arbitrary calls.
function executeWithERC20(
address token,
address to,
uint256 amount,
bytes calldata data
)
public
onlyRole(ASSET_HANDLER_ROLE)
whenNotPaused
nonReentrant
{
if (amount == 0) revert InsufficientERC20Amount();
if (to == address(0)) revert ZeroAddress();
// Approve the target contract to spend the tokens
if (!resetApproval(token, to)) revert ApprovalFailed();
if (!IERC20(token).approve(to, amount)) revert ApprovalFailed();
// Execute the call on the target contract
_executeArbitraryCall(to, data);
// Reset approval
if (!resetApproval(token, to)) revert ApprovalFailed();
// Transfer any remaining tokens back to the custody/connector contract
uint256 remainingBalance = IERC20(token).balanceOf(address(this));
if (remainingBalance > 0) {
transferToAssetHandler(token, remainingBalance);
}
emit ExecutedWithERC20(token, to, amount, data);
}But in the GatewayZEVM.sol someone could set the authenticated call to true in the call options.
There was a problem hiding this comment.
yes, i now remember this was also a question from me in PR description in original PR (2nd question in questions part) that was closed, but we didnt discuss it: #350, i am also leaning towards that its needed
same as: #350 just correct base branch and onCall function payable
Summary by CodeRabbit
Release Notes
New Features
GatewayEVMandGatewayZEVMcontracts.CallOptionsstructure for standardized call parameters, improving clarity in function calls.onCallfunction inReceiverEVMcontract to handle incoming calls with context.ReceivedOnCalladded for better event handling inIReceiverEVMEvents.Bug Fixes
Tests
GatewayEVMandGatewayZEVMwith new scenarios and validation checks.CallOptionsstructure for consistency.