feat: erc20 auth calls #401
Conversation
skosito
requested review from
andresaiello,
brewmaster012,
lumtis,
CharlieMc0,
fadeev and
fbac
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant updates across multiple contracts and their corresponding tests, primarily focusing on the addition of a 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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #401 +/- ##
=======================================
Coverage 84.23% 84.23%
=======================================
Files 8 8
Lines 387 387
Branches 121 122 +1
=======================================
Hits 326 326
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: 18
🧹 Outside diff range and nitpick comments (15)
v2/test/utils/IReceiverEVM.sol (1)
40-43: LGTM! Consider enhancing themessageparameter description.The updated
ReceivedOnCallevent now includessenderandmessageparameters, which significantly improves the context provided when the event is emitted. This change aligns well with the PR objectives of enhancing authentication for calls.Consider slightly expanding the description for the
messageparameter to provide more context about its contents. For example:/// @notice Emitted when onCall function is called with authenticated call. /// @param sender Message context sender. -/// @param message Message received. +/// @param message Encoded message data received in the authenticated call. event ReceivedOnCall(address sender, bytes message);This minor change would provide more clarity about the nature of the
messageparameter.v2/test/utils/ReceiverEVM.sol (1)
Line range hint
76-85: Approve event emission change and suggest improvementsThe updated event emission in the
onCallfunction is a good improvement, providing more context by including the sender and message data. This change is approved.However, there are a couple of points to consider for further improvement:
The function is marked as
payable, butmsg.valueis not used anywhere in the function. If there's no intention to use the sent Ether, consider removing thepayablemodifier to prevent accidental Ether transfers.The function declares a return type
bytes memory, but it doesn't actually return anything. Either add a return statement if a return value is intended, or remove the return type from the function signature if no return value is needed.Consider applying these changes:
function onCall( MessageContext calldata messageContext, bytes calldata message ) external - payable - returns (bytes memory) { emit ReceivedOnCall(messageContext.sender, message); + return ""; // Or remove if no return value is needed }v2/contracts/evm/interfaces/IERC20Custody.sol (1)
Line range hint
5-88: Overall impact: Focused enhancement with potential ripple effectsThe changes to the
IERC20Custodyinterface are focused and consistent with the described modifications. The addition of theMessageContextparameter towithdrawAndCallsuggests an improvement in contextual information for operations. While the impact is limited to this specific function, all implementations and calls towithdrawAndCallwill need to be updated, which could have a ripple effect through the codebase.To ensure the changes are properly integrated:
- Update all implementations of
IERC20Custodyto include the newMessageContextparameter inwithdrawAndCall.- Modify all calls to
withdrawAndCallthroughout the codebase to provide the requiredMessageContext.- Conduct thorough testing, including integration tests, to verify that the changes don't introduce any regressions or unexpected behavior.
- Update relevant documentation to reflect the new function signature and its usage.
v2/contracts/evm/ZetaConnectorNonNative.sol (2)
Line range hint
64-89: LGTM! Consider updating the function documentation.The changes to the
withdrawAndCallfunction look good. The addition of theMessageContextparameter enhances the context for the withdrawal operation, which is then correctly passed to thegateway.executeWithERC20function.Consider updating the function documentation to include a description of the new
messageContextparameter:/// @notice Withdraw tokens and call a contract through Gateway. +/// @param messageContext Message context containing sender information. /// @param to The address to withdraw tokens to. /// @param amount The amount of tokens to withdraw. /// @param data The calldata to pass to the contract call. /// @param internalSendHash A hash used for internal tracking of the transaction. /// @dev This function can only be called by the TSS address, and mints if supply is not reached.
Line range hint
1-143: Consider similar updates to other withdrawal functions for consistency.The
withdrawAndCallfunction has been updated to include aMessageContextparameter, which enhances the context for the withdrawal operation. For consistency and potential future-proofing, you might want to consider similar updates to thewithdrawandwithdrawAndRevertfunctions.If these functions don't require the additional context, it's fine to leave them as is. However, if there's a possibility they might benefit from this context in the future, consider updating them preemptively:
function withdraw( MessageContext calldata messageContext, address to, uint256 amount, bytes32 internalSendHash ) external override nonReentrant onlyRole(WITHDRAWER_ROLE) whenNotPaused { // ... (existing implementation) } function withdrawAndRevert( MessageContext calldata messageContext, address to, uint256 amount, bytes calldata data, bytes32 internalSendHash, RevertContext calldata revertContext ) external override nonReentrant onlyRole(WITHDRAWER_ROLE) whenNotPaused { // ... (existing implementation) }This suggestion is optional and depends on your specific use case and future plans for these functions.
v2/contracts/evm/ZetaConnectorBase.sol (1)
Line range hint
1-159: Overall assessment: Changes look good, but verify integration.The modifications to the
ZetaConnectorBasecontract, including the updated import statement and thewithdrawAndCallfunction signature, are consistent and well-implemented. These changes appear to be part of a larger update to include message context in various operations across the system.However, it's crucial to ensure that these changes are properly integrated throughout the codebase. Please verify that:
- All derived contracts have been updated to match the new
withdrawAndCallfunction signature.- All calling code has been modified to provide the required
MessageContextparameter.- The
MessageContexttype is correctly defined in theIGatewayEVMinterface.- The changes don't introduce any breaking changes in the contract's behavior or interactions with other parts of the system.
Consider updating the contract's documentation to reflect the new
MessageContextparameter and its significance in thewithdrawAndCalloperation. This will help maintain clear and up-to-date documentation for developers working with this contract.v2/test/GatewayEVMZEVM.t.sol (2)
53-53: Consider using a more meaningful default address forarbitraryCallMessageContext.The newly added
arbitraryCallMessageContextvariable usesaddress(0)as the default sender. While this might work for testing purposes, it's generally not recommended to use the zero address as it can lead to unexpected behavior or make debugging more difficult.Consider using a dedicated test address or one of the existing addresses in the test setup (e.g.,
ownerEVMortssAddress) as the default sender. Additionally, it would be helpful to add a comment explaining the purpose of this variable and why it's being introduced.
Line range hint
53-278: Consider refining the use ofarbitraryCallMessageContextacross test cases.The introduction of
arbitraryCallMessageContextand its use in theexecutefunction calls is consistent across all modified test functions. This change aligns with the updated function signature and is necessary for the tests to pass.However, to enhance the robustness and accuracy of these tests, consider the following improvements:
Instead of using a single
arbitraryCallMessageContextwithaddress(0)as the sender, create different contexts for each test case that accurately represent the scenario being tested (e.g., direct calls, calls from ZEVM, withdrawals, etc.).Add comments explaining the purpose and expected behavior of
arbitraryCallMessageContextin each test case.Consider adding additional test cases that specifically verify the behavior of the
executefunction with different message contexts.These refinements would make the tests more comprehensive and better aligned with the actual use cases of the contract.
v2/contracts/evm/interfaces/IGatewayEVM.sol (1)
Line range hint
191-193: Include 'arbitrary call flag' inMessageContextStruct or Update DocumentationThe
MessageContextstruct currently only includes thesenderfield, but the documentation suggests it should also contain an "arbitrary call flag." If this flag is necessary for your implementation, consider adding it to the struct. Otherwise, update the documentation to match the struct's actual fields.To include the "arbitrary call flag," apply this diff:
struct MessageContext { address sender; + bool arbitraryCallFlag; }v2/test/utils/upgrades/ERC20CustodyUpgradeTest.sol (1)
Validation Missing for
MessageContextinwithdrawAndCallThe
withdrawAndCallfunction invokesgateway.executeWithERC20withmessageContextwithout performing any validation checks. Norequireorassertstatements were found to validatemessageContext, which could expose the contract to malicious inputs or reentrancy attacks.
- File:
v2/test/utils/upgrades/ERC20CustodyUpgradeTest.sol- Lines: 144-156
🔗 Analysis chain
Line range hint
147-170: ValidateMessageContextinwithdrawAndCallThe
withdrawAndCallfunction now acceptsMessageContext calldata messageContext:function withdrawAndCall( MessageContext calldata messageContext, address to, address token, uint256 amount, bytes calldata data ) public nonReentrant onlyRole(WITHDRAWER_ROLE) whenNotPaused { // ... gateway.executeWithERC20(messageContext, token, to, amount, data); // ... }Ensure that
messageContextis properly validated before being used. Lack of validation could expose the contract to malicious inputs or reentrancy attacks.Consider adding validation checks for
messageContextto ensure it contains expected and safe values.Run the following script to check for validation of
messageContext:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for validation logic involving MessageContext in the contract. # Test: Look for `require` or `assert` statements validating `messageContext` rg --type solidity 'require.*messageContext' -A 5Length of output: 156
Script:
#!/bin/bash # Description: Search for validation logic involving MessageContext in Solidity contracts. # Test: Look for `require` or `assert` statements validating `messageContext` in `.sol` files rg 'require.*messageContext|assert.*messageContext' --type-add 'solidity:*.sol' --type solidity -A 5Length of output: 102
Script:
#!/bin/bash # Description: Explore usage and validation of MessageContext in Solidity contracts. # Search for the definition of MessageContext rg 'struct\s+MessageContext' --type-add 'solidity:*.sol' --type solidity # Search for any functions or modifiers that include 'validate' and 'messageContext' rg 'function\s+\w*\s*\(.*messageContext.*\)' --type-add 'solidity:*.sol' --type solidity -A 5 rg 'modifier\s+\w*\s*\(.*messageContext.*\)' --type-add 'solidity:*.sol' --type solidity -A 5 # Search for any inheritance that might include validation logic rg 'is\s+\w+' --type-add 'solidity:*.sol' --type solidity # Search for imported contracts that might validate MessageContext rg 'import\s+"[^"]*"' --type-add 'solidity:*.sol' --type solidity -A 2Length of output: 67969
v2/contracts/evm/GatewayEVM.sol (1)
182-186: Add comments to clarify the conditional logic based onmessageContext.senderSimilar to the
executefunction, theexecuteWithERC20function uses the conditionmessageContext.sender == address(0)to differentiate between arbitrary and authenticated calls. Adding explanatory comments can enhance code readability and maintainability.Apply this diff to add explanatory comments:
if (messageContext.sender == address(0)) { + // If sender is the zero address, perform an arbitrary call _executeArbitraryCall(to, data); } else { + // Otherwise, perform an authenticated call with messageContext _executeAuthenticatedCall(messageContext, to, data); }v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol (2)
246-246: Consider makingMAX_PAYLOAD_SIZEconfigurableThe checks for
payload.length + revertOptions.revertMessage.lengthagainstMAX_PAYLOAD_SIZEuse a hardcoded limit of1024. While this adds a safeguard, consider makingMAX_PAYLOAD_SIZEa configurable parameter to allow flexibility for different use cases.You can implement this by adding a setter function with appropriate access control:
- uint256 public constant MAX_PAYLOAD_SIZE = 1024; + uint256 public MAX_PAYLOAD_SIZE; + /// @notice Sets the maximum payload size. + /// @param maxSize The new maximum size. + function setMaxPayloadSize(uint256 maxSize) external onlyRole(DEFAULT_ADMIN_ROLE) { + MAX_PAYLOAD_SIZE = maxSize; + }This allows administrators to adjust the limit as needed.
Also applies to: 272-272, 295-295, 323-323, 344-344
25-25: Update event names consistently after version changeThe event
Executedhas been renamed toExecutedV2to indicate a version change. Ensure that all related events and their usages are consistently updated to reflect the new versioning scheme. Additionally, consider versioning other events or providing deprecation notices for old events to maintain consistency.You might also want to update other related events for consistency:
- event Reverted(address indexed destination, address token, uint256 value, bytes data, RevertContext revertContext); + event RevertedV2(address indexed destination, address token, uint256 value, bytes data, RevertContext revertContext);This helps in maintaining a clear event history with versioning.
v2/test/ZetaConnectorNonNative.t.sol (1)
44-44: Consider initializingarbitraryCallMessageContextwithin each test functionCurrently,
arbitraryCallMessageContextis initialized at the contract level withsender: address(0). To enhance test clarity and isolation, consider initializingMessageContextwithin each test function with context-specificsenderaddresses. This approach ensures that each test accurately reflects its scenario without relying on a shared state.v2/test/ZetaConnectorNative.t.sol (1)
44-44: Consider declaringarbitraryCallMessageContextas a constant.Since
arbitraryCallMessageContextis initialized once and not modified throughout the tests, declaring it as aconstantcan improve code clarity and indicate that it should not be changed.Apply this change:
- MessageContext arbitraryCallMessageContext = MessageContext({ sender: address(0) }); + MessageContext constant arbitraryCallMessageContext = MessageContext({ sender: address(0) });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (52)
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/IGatewayEVM.sol/interface.IGatewayEVM.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/erc20custodyupgradetest.sol/erc20custodyupgradetest.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/gatewayevmupgradetest.sol/gatewayevmupgradetest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmzevm.t.sol/gatewayevmzevmtest.gois excluded by!v2/pkg/**v2/pkg/ierc20custody.sol/ierc20custody.gois excluded by!v2/pkg/**v2/pkg/igatewayevm.sol/igatewayevm.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/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/zetaconnectornativeupgradetest.sol/zetaconnectornativeupgradetest.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/zetaconnectornonnativeupgradetest.sol/zetaconnectornonnativeupgradetest.gois excluded by!v2/pkg/**v2/types/ERC20Custody.tsis excluded by!v2/types/**v2/types/ERC20CustodyUpgradeTest.tsis excluded by!v2/types/**v2/types/GatewayEVM.tsis excluded by!v2/types/**v2/types/GatewayEVMUpgradeTest.tsis excluded by!v2/types/**v2/types/IERC20Custody.sol/IERC20Custody.tsis excluded by!v2/types/**v2/types/IGatewayEVM.sol/IGatewayEVM.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/ZetaConnectorBase.tsis excluded by!v2/types/**v2/types/ZetaConnectorNative.tsis excluded by!v2/types/**v2/types/ZetaConnectorNativeUpgradeTest.tsis excluded by!v2/types/**v2/types/ZetaConnectorNonNative.tsis excluded by!v2/types/**v2/types/ZetaConnectorNonNativeUpgradeTest.tsis excluded by!v2/types/**v2/types/factories/ERC20CustodyUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/ERC20Custody__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/IERC20Custody.sol/IERC20Custody__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayEVM.sol/IGatewayEVM__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/ZetaConnectorBase__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNativeUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNative__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNonNativeUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNonNative__factory.tsis excluded by!v2/types/**
📒 Files selected for processing (18)
- v2/contracts/evm/ERC20Custody.sol (3 hunks)
- v2/contracts/evm/GatewayEVM.sol (3 hunks)
- v2/contracts/evm/ZetaConnectorBase.sol (2 hunks)
- v2/contracts/evm/ZetaConnectorNative.sol (2 hunks)
- v2/contracts/evm/ZetaConnectorNonNative.sol (2 hunks)
- v2/contracts/evm/interfaces/IERC20Custody.sol (2 hunks)
- v2/contracts/evm/interfaces/IGatewayEVM.sol (1 hunks)
- v2/test/ERC20Custody.t.sol (12 hunks)
- v2/test/GatewayEVM.t.sol (9 hunks)
- v2/test/GatewayEVMZEVM.t.sol (5 hunks)
- v2/test/ZetaConnectorNative.t.sol (6 hunks)
- v2/test/ZetaConnectorNonNative.t.sol (7 hunks)
- v2/test/utils/IReceiverEVM.sol (1 hunks)
- v2/test/utils/ReceiverEVM.sol (1 hunks)
- v2/test/utils/upgrades/ERC20CustodyUpgradeTest.sol (5 hunks)
- v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol (19 hunks)
- v2/test/utils/upgrades/ZetaConnectorNativeUpgradeTest.sol (2 hunks)
- v2/test/utils/upgrades/ZetaConnectorNonNativeUpgradeTest.sol (3 hunks)
🧰 Additional context used
🔇 Additional comments (49)
v2/contracts/evm/ZetaConnectorNative.sol (1)
Line range hint
48-71: LGTM! Consider verifying impacts on calling code.The changes to the
withdrawAndCallfunction look good. The addition of theMessageContextparameter enhances the context information passed to the function, and the updates to both the function comment and thegateway.executeWithERC20call are consistent with this change.To ensure these changes don't introduce any issues, please verify:
- All calls to
withdrawAndCallin other contracts have been updated to include the newMessageContextparameter.- Any tests involving
withdrawAndCallhave been updated to reflect the new function signature.You can use the following script to check for potential places that might need updates:
This script will help identify areas of the codebase that might need attention due to the signature change.
v2/test/utils/upgrades/ZetaConnectorNativeUpgradeTest.sol (3)
Line range hint
20-20: LGTM: New eventWithdrawnV2added for upgrade testing.The new event
WithdrawnV2is correctly implemented with relevant parameters for tracking withdrawals. The "V2" suffix in the name clearly indicates it's an upgraded version of a previous event.
Line range hint
1-118: Summary: Changes inZetaConnectorNativeUpgradeTest.sollook good overall.The modifications to this contract, including the new
WithdrawnV2event and the updatedwithdrawAndCallfunction, are well-implemented and align with the PR objectives. The changes enhance the context for withdrawal operations by introducing theMessageContextparameter.To ensure the consistency of these changes across the codebase:
- Verify that all calls to
withdrawAndCallin other contracts are updated to include the newMessageContextparameter.- Confirm that the
Gatewaycontract'sexecuteWithERC20function has been updated to accept theMessageContextparameter.These verifications will help maintain the integrity of the system after the upgrade.
75-75: LGTM: Updated gateway call with newmessageContextparameter.The
gateway.executeWithERC20call has been correctly updated to include the newmessageContextparameter.To ensure consistency, please verify that the
Gatewaycontract'sexecuteWithERC20function has been updated to accept theMessageContextparameter:#!/bin/bash # Description: Check Gateway contract for updated executeWithERC20 function # Search for the executeWithERC20 function definition in the Gateway contract rg --type solidity 'function\s+executeWithERC20.*?MessageContext' -C 3v2/contracts/evm/interfaces/IERC20Custody.sol (2)
6-6: LGTM: New import for MessageContextThe addition of this import statement is necessary to support the new
MessageContextparameter in thewithdrawAndCallfunction. This change is consistent with the modifications described in the summary.
81-88: LGTM: Updated withdrawAndCall signature with MessageContextThe addition of the
MessageContextparameter to thewithdrawAndCallfunction is consistent with the described changes and likely provides valuable context for the operation. This modification enhances the function's capabilities but will require updates in all implementations and calls to this function.To ensure all implementations and calls have been updated accordingly, please run the following script:
v2/contracts/evm/ZetaConnectorBase.sol (2)
Line range hint
120-132: LGTM! Verify derived contracts and calling code.The
withdrawAndCallfunction signature has been correctly updated to include theMessageContextparameter. The use ofcalldatafor this parameter is appropriate and gas-efficient.To ensure that all derived contracts and calling code have been updated accordingly, please run the following script:
#!/bin/bash # Description: Verify updates in derived contracts and calling code # Test 1: Check for any remaining occurrences of the old function signature rg --type solidity 'function\s+withdrawAndCall\s*\(\s*address\s+to\s*,\s*uint256\s+amount\s*,\s*bytes\s+calldata\s+data\s*,\s*bytes32\s+internalSendHash\s*\)' # Test 2: Check for occurrences of the new function signature in derived contracts rg --type solidity 'function\s+withdrawAndCall\s*\(\s*MessageContext\s+calldata\s+messageContext\s*,\s*address\s+to\s*,\s*uint256\s+amount\s*,\s*bytes\s+calldata\s+data\s*,\s*bytes32\s+internalSendHash\s*\)' # Test 3: Check for function calls to withdrawAndCall rg --type solidity 'withdrawAndCall\s*\('Please review the results to ensure all necessary updates have been made.
13-18: LGTM! Verify the IGatewayEVM interface.The import statement has been correctly updated to include
MessageContext. This change is consistent with its usage in thewithdrawAndCallfunction.To ensure the
MessageContexttype is correctly defined, please run the following script:v2/test/GatewayEVMZEVM.t.sol (4)
170-170: Verify if the newarbitraryCallMessageContextcorrectly tests the intended behavior.The modification to include
arbitraryCallMessageContextin theexecutefunction call aligns with the updated function signature. This change is correct and necessary.However, since
arbitraryCallMessageContextis initialized with a default sender ofaddress(0), ensure that this adequately tests the intended behavior of theexecutefunction. Consider adding test cases with different sender addresses to comprehensively test the function's behavior with various message contexts.
200-200: Verify if thearbitraryCallMessageContextcorrectly represents the sender behavior.The modification to include
arbitraryCallMessageContextin theexecutefunction call is correct and consistent with the updated function signature.However, since this test is specifically for calls from a sender on ZEVM, consider modifying the
arbitraryCallMessageContextto use a sender address that represents the ZEVM sender (e.g.,address(senderZEVM)). This would more accurately simulate the expected behavior and provide a more robust test case.
246-246: EnsurearbitraryCallMessageContextcorrectly represents the withdrawal scenario.The modification to include
arbitraryCallMessageContextin theexecutefunction call is correct and consistent with the updated function signature.Given that this test combines withdrawal and execution, it's important to verify if the current
arbitraryCallMessageContext(with a sender ofaddress(0)) accurately represents the expected behavior in a withdrawal scenario. Consider modifying the context to use a sender address that reflects the withdrawal initiator (e.g.,ownerZEVM) to more accurately test this combined functionality.
278-278: Verify ifarbitraryCallMessageContextaccurately represents the withdrawal from sender ZEVM scenario.The modification to include
arbitraryCallMessageContextin theexecutefunction call is correct and consistent with the updated function signature.This test case combines withdrawal, sender behavior on ZEVM, and execution, which is a complex scenario. It's crucial to ensure that the current
arbitraryCallMessageContext(with a sender ofaddress(0)) accurately represents this multi-step process. Consider modifying the context to use a sender address that reflects both the withdrawal initiator and the ZEVM sender (e.g.,address(senderZEVM)). This would provide a more accurate and comprehensive test of the intended functionality.v2/contracts/evm/ERC20Custody.sol (4)
5-5: Import updated to includeMessageContextThe inclusion of
MessageContextin the import statement is appropriate and aligns with the updates to the function signatures.
143-143: Function documentation updated withmessageContextparameterThe function comment correctly documents the new
messageContextparameter, ensuring clarity in the function's usage.
Line range hint
149-153: FunctionwithdrawAndCallsignature updated appropriatelyThe addition of
MessageContext calldata messageContextto thewithdrawAndCallfunction enhances the contextual information passed during execution.
166-166: PassingmessageContexttogateway.executeWithERC20The updated call to
gateway.executeWithERC20now includesmessageContext, ensuring that the execution carries the necessary context.v2/test/utils/upgrades/ERC20CustodyUpgradeTest.sol (3)
98-99: Review the Sequence of UpdatingtssAddressThe
tssAddressis updated after revoking roles from the old address and before granting roles to the new address:tssAddress = newTSSAddress;Ensure that updating
tssAddressat this point is correct and that there are no dependencies ontssAddressbetween the role revocation and this assignment.
170-170: Confirm Parameter Order ingateway.executeWithERC20CallThe call to
gateway.executeWithERC20includesmessageContextas the first argument:gateway.executeWithERC20(messageContext, token, to, amount, data);Verify that the parameter order matches the function definition in
IGatewayEVMto prevent any mismatches that could lead to errors.Run the following script to retrieve the function definition and confirm the parameter order:
#!/bin/bash # Description: Retrieve the definition of `executeWithERC20` in `IGatewayEVM` # Test: Confirm parameter order matches usage rg --type solidity 'function executeWithERC20' -A 5
Line range hint
193-208: ValidateRevertContextinwithdrawAndRevertThe
withdrawAndRevertfunction now includesRevertContext calldata revertContext:function withdrawAndRevert( address to, address token, uint256 amount, bytes calldata data, RevertContext calldata revertContext ) public nonReentrant onlyRole(WITHDRAWER_ROLE) whenNotPaused { // ... gateway.revertWithERC20(token, to, amount, data, revertContext); // ... }Ensure that
revertContextis properly validated to prevent misuse. Improper handling might lead to unintended contract behavior.Consider adding necessary validation or sanitization for
revertContext.Run the following script to check for validation of
revertContext:#!/bin/bash # Description: Search for validation logic involving RevertContext in the contract. # Test: Look for `require` or `assert` statements validating `revertContext` rg --type solidity 'require.*revertContext' -A 5v2/contracts/evm/GatewayEVM.sol (3)
159-159: Documentation updated correctly with the new parameterThe function documentation now includes the
messageContextparameter, accurately reflecting the updated function signature inexecuteWithERC20. This ensures that the purpose and usage of the parameter are clear to other developers.
182-186: Verify that all calls toexecuteWithERC20have been updated with the new signatureThe
executeWithERC20function now requires aMessageContextparameter. Ensure that all existing calls to this function are updated accordingly to include this new parameter.Run the following script to identify any calls that may be missing the
MessageContextparameter:#!/bin/bash # Description: Find all calls to `executeWithERC20` that might not include the new `MessageContext` parameter. # Search for function calls to `executeWithERC20` without `MessageContext` rg --type solidity 'executeWithERC20\(\s*address\b' --files-with-matchesThis script looks for calls to
executeWithERC20where the first argument is anaddress, suggesting that theMessageContextparameter is not included.
145-149: Verify that all calls toexecutehave been updated with the new signatureThe
executefunction signature now includesMessageContext. It's important to ensure that all calls to this function throughout the codebase have been updated to match the new signature to prevent compilation errors or unintended behavior.Run the following script to locate any outdated calls to
execute:This script searches for calls to
executewhere the first argument is anaddress, indicating that theMessageContextparameter might be missing.✅ Verification successful
All
executefunction calls are updated with the newMessageContextparameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to `execute` that might not include the new `MessageContext` parameter. # Search for function calls to `execute` without `MessageContext` rg --type solidity 'execute\(\s*address\b' --files-with-matchesLength of output: 107
Script:
#!/bin/bash # Description: Find all calls to `execute` that might not include the new `MessageContext` parameter. # Search for function calls to `execute` without `MessageContext` in .sol files rg 'execute\(\s*address\b' --glob '*.sol' --files-with-matchesLength of output: 64
v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol (3)
386-397: Confirm the handling of ZETA token depositsThe code related to handling ZETA token deposits is commented out, and a
ZETANotSupportederror is reverted instead. Ensure that this is the intended behavior for the current release. Additionally, consider adding tests to cover this scenario and plan for re-enabling the code when support for ZETA deposits is implemented.Would you like assistance in creating unit tests for the
ZETANotSupportedscenario or in planning the reintroduction of ZETA token support?
149-153: Validate the condition for executing arbitrary or authenticated callsThe condition
if (messageContext.sender == address(0))is used to determine whether to execute an arbitrary call or an authenticated call. Ensure thatmessageContext.senderbeing zero accurately reflects an unauthenticated context.Run the following script to confirm that
messageContext.senderis set appropriately throughout the codebase:#!/bin/bash # Description: Verify that messageContext.sender is correctly initialized and used. # Test: Search for initializations and usages of MessageContext. # Expected result: Consistent handling of messageContext.sender across functions. # Search for MessageContext initializations rg --type solidity 'MessageContext\s+memory\s+\w+\s*=\s*MessageContext\({' # Search for usages of messageContext.sender rg --type solidity 'messageContext\.sender'This will help ensure that the logic based on
messageContext.senderis sound and that there are no scenarios where it might unintentionally be zero.Also applies to: 186-190
374-374: Verify that making functions private does not affect external callsThe functions
_resetApproval,_transferFromToAssetHandler, and_transferToAssetHandlerhave been changed toprivate. Ensure that these functions are not called from derived contracts or external sources to prevent breaking functionality.Run the following script to check for any external calls to these functions:
This script searches for any occurrences of these function calls outside their defining context. Ensure that no matches are found outside the
GatewayEVMUpgradeTestcontract.Also applies to: 384-384, 410-410
✅ Verification successful
Verification Successful: Private Functions Are Not Exposed Externally
- No external calls to
_resetApproval,_transferFromToAssetHandler, or_transferToAssetHandlerwere found outside their defining contracts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the functions made private are not called externally. # Test: Search for external calls to the private functions. # Expected result: No external calls found. # Search for external calls to _resetApproval rg --type solidity '_resetApproval\(' # Search for external calls to _transferFromToAssetHandler rg --type solidity '_transferFromToAssetHandler\(' # Search for external calls to _transferToAssetHandler rg --type solidity '_transferToAssetHandler\('Length of output: 444
Script:
#!/bin/bash # Description: Verify that the functions made private are not called externally. # Test: Search for external calls to the private functions. # Expected result: No external calls found. # Search for external calls to _resetApproval rg '_resetApproval\(' --glob '*.sol' # Search for external calls to _transferFromToAssetHandler rg '_transferFromToAssetHandler\(' --glob '*.sol' # Search for external calls to _transferToAssetHandler\(' --glob '*.sol'Length of output: 1444
v2/test/ZetaConnectorNonNative.t.sol (5)
174-174: Verify the appropriateness ofarbitraryCallMessageContextin this testIn
testWithdrawAndCallReceiveERC20, you're passingarbitraryCallMessageContextwithsender: address(0)towithdrawAndCall. Ensure that usingaddress(0)as the sender is suitable for this test case. If the test aims to simulate a specific sender, consider creating aMessageContextwith a meaningfulsenderaddress to reflect realistic scenarios.
193-226: Great addition of comprehensive test forwithdrawAndCallwithMessageContextThe
testWithdrawAndCallReceiveOnCallfunction effectively tests thewithdrawAndCallmethod with the newMessageContextparameter. It validates that the correct events are emitted and that token balances remain unchanged as expected when handlingonCall. This enhances the test coverage for the updated functionality.
228-238: Ensure correct error handling for unauthorized calls toonCallIn
testWithdrawAndCallReceiveOnCallTNotAllowedWithArbitraryCall, the test expects a revert withNotAllowedToCallOnCall.selector. Verify that theNotAllowedToCallOnCallerror is properly defined and used consistently in the contract. Additionally, confirm that the test accurately simulates an unauthorized call scenario toonCall, ensuring the contract's security measures are effectively tested.
268-268: Verify theMessageContextused intestWithdrawAndCallReceiveNoParamsIn this test, you're using
arbitraryCallMessageContextwithsender: address(0). Ensure that this context is appropriate for the test scenario. If the test requires a specific sender or aims to simulate a particular context, consider initializing aMessageContextwith the relevantsenderaddress within the test function.
306-306: EnsureMessageContextaccurately represents the test scenarioIn
testWithdrawAndCallReceiveERC20Partial, thearbitraryCallMessageContextis used with a default sender. To enhance the accuracy of the test, verify that theMessageContextreflects the intended sender for this scenario. Initializing a specificMessageContextwithin the test can improve clarity and prevent unintended interactions.v2/test/ZetaConnectorNative.t.sol (6)
232-265: Review the new testtestWithdrawAndCallReceiveOnCall()for completeness.The new test function appears to correctly verify the
withdrawAndCallfunctionality when receiving anonCallmessage. Ensure that all relevant assertions are included, and consider adding edge cases to thoroughly test the behavior.
305-305: Review the usage ofarbitraryCallMessageContextintestWithdrawAndCallReceiveNoParams().Check whether using
arbitraryCallMessageContextwithsenderset toaddress(0)is suitable for this test, or if a specificMessageContextshould be provided to match the test's intent and improve clarity.Run the following script to ensure consistency in
MessageContextusage across tests:#!/bin/bash # Description: List all `withdrawAndCall` calls and the `MessageContext` passed. # Search for `withdrawAndCall` invocations and extract the `MessageContext` argument rg --type solidity 'withdrawAndCall\(' -A 1 | rg 'MessageContext\([^)]+\)'Expected Result: Verify that the
MessageContextpassed towithdrawAndCallmatches each test case's requirements and that usingarbitraryCallMessageContextis intentional and appropriate.
341-341: Check the correctness ofMessageContextintestWithdrawAndCallReceiveERC20Partial().Ensure that
arbitraryCallMessageContextis appropriate for this test, and consider whether a specificMessageContextshould be used to accurately simulate the test conditions and improve test clarity.Run the following script to confirm
MessageContextusage in this test:#!/bin/bash # Description: Verify `MessageContext` in `testWithdrawAndCallReceiveERC20Partial`. # Extract the `MessageContext` parameter used in the test function rg --type solidity -A 10 'function testWithdrawAndCallReceiveERC20Partial\(\)' | rg 'withdrawAndCall\(' -A 1Expected Result: Review the extracted
MessageContextand ensure it is appropriate for the test scenario, confirming that the context aligns with the intended behavior being tested.
213-213: Verify the appropriateness of usingarbitraryCallMessageContextintestWithdrawAndCallReceiveERC20().In this test,
arbitraryCallMessageContexthassenderset toaddress(0). Ensure that this context aligns with the expected behavior of the test, and that usingaddress(0)does not affect the functionality being tested.Run the following script to check all instances where
arbitraryCallMessageContextis used and verify thesendervalue:Expected Result: Confirm that
arbitraryCallMessageContextis appropriate in each test case and that thesenderfield is correctly set for the intended test scenarios.✅ Verification successful
Verification Successful:
arbitraryCallMessageContextUsage is AppropriateThe use of
arbitraryCallMessageContextwithsenderset toaddress(0)aligns with the intended test scenarios and does not negatively impact the functionality being tested.
- Verified in multiple test files:
v2/test/ZetaConnectorNative.t.solv2/test/ZetaConnectorNonNative.t.solv2/test/GatewayEVMZEVM.t.solv2/test/GatewayEVM.t.solv2/test/ERC20Custody.t.sol🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of `arbitraryCallMessageContext` and display the context where it is used. # Search for `arbitraryCallMessageContext` assignments and usages in Solidity files rg --type solidity -A 2 'arbitraryCallMessageContext'Length of output: 86
Script:
#!/bin/bash # Description: Find usages of `arbitraryCallMessageContext` and display the context where it is used. # Search for `arbitraryCallMessageContext` in Solidity files with .sol extension rg --glob '*.sol' -A 2 'arbitraryCallMessageContext'Length of output: 9944
267-277: Verify error handling intestWithdrawAndCallReceiveOnCallTNotAllowedWithArbitraryCall().Confirm that the
vm.expectRevertis correctly catching theNotAllowedToCallOnCallerror and that the test accurately reflects the scenario where an arbitrary call toonCallis disallowed.Run the following script to find all definitions and usages of
NotAllowedToCallOnCall:Expected Result: Verify that
NotAllowedToCallOnCallis correctly defined as an error and that it is appropriately used in both the contract and the test to ensure the test behaves as expected.✅ Verification successful
Error Handling Verified Successfully. The
vm.expectRevert(NotAllowedToCallOnCall.selector)correctly catches theNotAllowedToCallOnCallerror, ensuring that arbitrary calls toonCallare properly disallowed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find occurrences of `NotAllowedToCallOnCall` in the codebase. # Search for `NotAllowedToCallOnCall` definitions and usages in Solidity files rg --type solidity 'NotAllowedToCallOnCall'Length of output: 143
Script:
#!/bin/bash # Description: Find occurrences of `NotAllowedToCallOnCall` in Solidity (.sol) files. # Search for `NotAllowedToCallOnCall` in files with .sol extension rg 'NotAllowedToCallOnCall' --glob '*.sol'Length of output: 656
287-287: EnsurearbitraryCallMessageContextis appropriate intestWithdrawAndCallReceiveERC20FailsIfSenderIsNotWithdrawer().In this test, verify that using
arbitraryCallMessageContextis suitable for testing the failure case where the sender lacks theWITHDRAWER_ROLE. Ensure that the context does not unintentionally affect the test outcome.Run the following script to review the context of
withdrawAndCallcalls in failure tests:Expected Result: Confirm that in failure tests, the
MessageContextparameters are appropriate for the scenarios being tested and do not interfere with the focus of the test.v2/test/GatewayEVM.t.sol (1)
189-190: Correct usage ofMessageContext.senderintestForwardCallToReceiveOnCallUsingAuthCallThe test properly initializes
senderwith a specific address and includes it in theMessageContext. This accurately simulates the scenario where the sender is a particular address, ensuring the test reflects the expected behavior.Also applies to: 194-194
v2/test/ERC20Custody.t.sol (10)
38-38: Verify initialization ofarbitraryCallMessageContext.senderInitializing
arbitraryCallMessageContext.senderwithaddress(0)may cause unexpected behavior if downstream logic expects a valid non-zero address. Please confirm that usingaddress(0)is intentional and that all contract functions handle this case appropriately.
286-286: Test correctly expects revert for unauthorized accountThe test properly checks that a revert occurs when an unauthorized account (
owner) attempts to callwithdrawAndCall, ensuring access control is enforced.
296-296: Test correctly handles zero amount caseThe test accurately verifies that calling
withdrawAndCallwith an amount of zero reverts withInsufficientERC20Amount, confirming proper input validation.
306-306: Test correctly handles zero address receiverThe test ensures that invoking
withdrawAndCallwith a zero address as the receiver reverts withZeroAddress, which is essential for preventing token loss.
343-373: Review oftestForwardCallToReceiveOnCallThroughCustodyThe test correctly initializes
MessageContextwith a specificsenderand verifies thatwithdrawAndCallfunctions as expected. All assertions and emitted events are properly validated.
375-384: Test correctly verifiesNotAllowedToCallOnCallrevertThe test accurately checks that attempting to call
onCallviawithdrawAndCallwitharbitraryCallMessageContextreverts withNotAllowedToCallOnCall, ensuring restricted functions cannot be called arbitrarily.
386-393: Test correctly verifiesNotAllowedToCallOnRevertrevertThis test ensures that calling
onRevertthroughwithdrawAndCallwitharbitraryCallMessageContextappropriately reverts withNotAllowedToCallOnRevert, maintaining contract security.
402-402: Test correctly expects revert for unauthorized accountSimilar to earlier, the test validates that an unauthorized account cannot invoke
withdrawAndCall, reinforcing access control mechanisms.
412-412: Test correctly handles zero amount caseThe test confirms that providing an amount of zero to
withdrawAndCallresults in a revert withInsufficientERC20Amount, highlighting proper input checks.
462-462: Test correctly verifiesNotWhitelistedrevertThe test ensures that calling
withdrawAndCallwith a token that has been unwhitelisted reverts withNotWhitelisted, enforcing token whitelist constraints.v2/test/utils/upgrades/ZetaConnectorNonNativeUpgradeTest.sol (2)
91-91:⚠️ Potential issueConfirm correct usage of
gateway.executeWithERC20withmessageContextIn the call to
gateway.executeWithERC20(messageContext, address(zetaToken), to, amount, data);, ensure that thegatewaycontract'sexecuteWithERC20function acceptsMessageContextas the first parameter. If thegatewaycontract or its interface has not been updated accordingly, this could result in a mismatch and potential runtime errors.Run the following script to check the definition of
executeWithERC20in thegatewaycontract:#!/bin/bash # Description: Verify that `gateway.executeWithERC20` accepts `MessageContext` as the first parameter. # Search for the definition of `executeWithERC20` in Solidity files # Expect: Function should have `MessageContext` as the first parameter rg --type solidity -A3 -B3 'function executeWithERC20\('
132-132: Assess the impact of changing_mintTovisibility frominternaltoprivateThe
_mintTofunction's visibility has been changed frominternaltoprivate. This restricts its access exclusively to theZetaConnectorNonNativeUpgradeTestcontract. Ensure that no derived contracts or external functions rely on_mintTo. Changing visibility could break functionality in contracts that inherit from this contract.Run the following script to identify any external usages of
_mintTo:#!/bin/bash # Description: Check for any calls to `_mintTo` outside of the current contract. # Search for calls to `_mintTo` in Solidity files excluding the current file # Expect: No occurrences of `_mintTo` being used outside this contract rg --type solidity '_mintTo\(' --glob '!v2/test/utils/upgrades/ZetaConnectorNonNativeUpgradeTest.sol'
| /// @param messageContext Message context containing sender. | ||
| /// @param to The address to withdraw tokens to. | ||
| /// @param amount The amount of tokens to withdraw. | ||
| /// @param data The calldata to pass to the contract call. | ||
| /// @param internalSendHash A hash used for internal tracking of the transaction. | ||
| /// @dev This function can only be called by the TSS address. | ||
| function withdrawAndCall( | ||
| MessageContext calldata messageContext, |
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Update All withdrawAndCall Calls
Multiple instances of withdrawAndCall were found across the codebase. Please ensure that each call includes the new MessageContext parameter to maintain consistency and functionality.
- Review and update all identified calls in the output to match the updated function signature.
- Ensure that the
MessageContextis correctly constructed and passed in each case.
🔗 Analysis chain
LGTM: Updated withdrawAndCall function signature and comment.
The function signature has been correctly updated to include the new MessageContext parameter, and the function comment has been appropriately updated to describe this new parameter.
To ensure consistency across the codebase, please run the following script to check for any calls to withdrawAndCall that might need updating:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for calls to withdrawAndCall that might need updating
# Search for calls to withdrawAndCall
rg --type solidity 'withdrawAndCall\s*\(' -C 3
Length of output: 105
Script:
#!/bin/bash
# Description: Check for calls to withdrawAndCall that might need updating
# Search for calls to withdrawAndCall across all files
rg 'withdrawAndCall\s*\(' -C 3
Length of output: 106857
| /// @param messageContext Message context containing sender and arbitrary call flag. | ||
| /// @param token The address of the ERC20 token. | ||
| /// @param to The address of the contract to call. | ||
| /// @param amount The amount of tokens to transfer. | ||
| /// @param data The calldata to pass to the contract call. |
There was a problem hiding this comment.
Update Documentation to Match MessageContext Structure
The documentation for the messageContext parameter mentions that it contains "sender and arbitrary call flag," but the MessageContext struct only includes address sender;. Please update the documentation to accurately reflect the actual structure of MessageContext or include the "arbitrary call flag" in the struct if necessary.
If the "arbitrary call flag" is not required, apply this diff to update the documentation:
/// @param messageContext Message context containing sender and arbitrary call flag.
+/// @param messageContext Message context containing the sender address.📝 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.
| /// @param messageContext Message context containing sender and arbitrary call flag. | |
| /// @param token The address of the ERC20 token. | |
| /// @param to The address of the contract to call. | |
| /// @param amount The amount of tokens to transfer. | |
| /// @param data The calldata to pass to the contract call. | |
| /// @param messageContext Message context containing the sender address. | |
| /// @param token The address of the ERC20 token. | |
| /// @param to The address of the contract to call. | |
| /// @param amount The amount of tokens to transfer. | |
| /// @param data The calldata to pass to the contract call. |
| _grantRole(PAUSER_ROLE, tssAddress_); | ||
| _grantRole(WITHDRAWER_ROLE, tssAddress_); | ||
| _grantRole(WHITELISTER_ROLE, admin_); | ||
| _grantRole(WHITELISTER_ROLE, tssAddress_); |
There was a problem hiding this comment.
💡 Codebase verification
Security Concern: Multiple Critical Roles Assigned to tssAddress_
The tssAddress_ is granted the following roles:
PAUSER_ROLEWITHDRAWER_ROLEWHITELISTER_ROLETSS_ROLE
Granting these roles provides significant permissions, including the ability to pause the contract and withdraw funds. Ensure that tssAddress_ is a trusted entity with appropriate security measures in place.
🔗 Analysis chain
Security Concern: Role Assignments to tssAddress_
The tssAddress_ is being granted the PAUSER_ROLE, WITHDRAWER_ROLE, and WHITELISTER_ROLE:
_grantRole(PAUSER_ROLE, tssAddress_);
_grantRole(WITHDRAWER_ROLE, tssAddress_);
_grantRole(WHITELISTER_ROLE, admin_);
_grantRole(WHITELISTER_ROLE, tssAddress_);Granting the PAUSER_ROLE to the TSS address allows it to pause and unpause the contract, which is a critical operation. Ensure that this level of access is intended and that the TSS address is trusted to manage these roles.
Run the following script to review all role assignments to tssAddress_ and assess potential security implications:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all instances where roles are granted to tssAddress_
# Test: Ensure only intended roles are granted to tssAddress_
rg --type solidity $'grantRole\\(.+, tssAddress_\\)'
Length of output: 156
Script:
#!/bin/bash
# Description: Find all instances where roles are granted to tssAddress_ in Solidity files
# Test: Ensure only intended roles are granted to tssAddress_
rg --glob "*.sol" 'grantRole\(.+, tssAddress_\)'
Length of output: 1195
| if (messageContext.sender == address(0)) { | ||
| result = _executeArbitraryCall(destination, data); | ||
| } else { | ||
| result = _executeAuthenticatedCall(messageContext, destination, data); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using an explicit flag to indicate call type instead of checking messageContext.sender
Currently, the code distinguishes between arbitrary and authenticated calls by checking if messageContext.sender == address(0). While this approach works, it relies on a special value for the sender address, which can be less readable and might lead to errors if not handled carefully.
Consider introducing an explicit boolean flag in MessageContext, such as isAuthenticated, to make the distinction clearer and improve code readability:
struct MessageContext {
address sender;
bool isAuthenticated;
}Then update the condition:
-if (messageContext.sender == address(0)) {
+if (!messageContext.isAuthenticated) {
result = _executeArbitraryCall(destination, data);
} else {
result = _executeAuthenticatedCall(messageContext, destination, data);
}| function _executeArbitraryCall(address destination, bytes calldata data) private returns (bytes memory) { | ||
| _revertIfOnCallOrOnRevert(data); | ||
| (bool success, bytes memory result) = destination.call{ value: msg.value }(data); | ||
| if (!success) revert ExecutionFailed(); | ||
|
|
There was a problem hiding this comment.
Ensure correct extraction of function selector in _revertIfOnCallOrOnRevert
The assembly code in _revertIfOnCallOrOnRevert may not correctly extract the function selector from data. Consider using safer methods to extract the function selector without using assembly.
You can replace the assembly code with Solidity's built-in functions:
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 approach avoids direct memory manipulation and enhances code safety and readability.
Also applies to: 452-468
| @@ -240,7 +241,7 @@ | |||
|
|
|||
| vm.expectRevert(EnforcedPause.selector); | |||
| vm.prank(tssAddress); | |||
| custody.withdrawAndCall(address(receiver), address(token), amount, data); | |||
| custody.withdrawAndCall(arbitraryCallMessageContext, address(receiver), address(token), amount, data); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a realistic MessageContext.sender in tests
As above, updating arbitraryCallMessageContext.sender to a valid address can improve test accuracy by reflecting actual usage conditions.
| @@ -256,7 +257,7 @@ | |||
| vm.expectEmit(true, true, true, true, address(custody)); | |||
| emit WithdrawnAndCalled(address(receiver), address(token), amount, data); | |||
| vm.prank(tssAddress); | |||
| custody.withdrawAndCall(address(receiver), address(token), amount, data); | |||
| custody.withdrawAndCall(arbitraryCallMessageContext, address(receiver), address(token), amount, data); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a realistic MessageContext.sender in tests
Again, consider setting arbitraryCallMessageContext.sender to a meaningful address to enhance the effectiveness of this test.
| @@ -320,7 +321,7 @@ | |||
| vm.expectEmit(true, true, true, true, address(custody)); | |||
| emit WithdrawnAndCalled(address(receiver), address(token), amount, data); | |||
| vm.prank(tssAddress); | |||
| custody.withdrawAndCall(address(receiver), address(token), amount, data); | |||
| custody.withdrawAndCall(arbitraryCallMessageContext, address(receiver), address(token), amount, data); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a realistic MessageContext.sender in tests
In this test, consider assigning a valid address to arbitraryCallMessageContext.sender to more accurately simulate typical usage and enhance test coverage.
| @@ -373,7 +426,7 @@ | |||
| vm.expectEmit(true, true, true, true, address(custody)); | |||
| emit WithdrawnAndCalled(address(receiver), address(token), amount, data); | |||
| vm.prank(tssAddress); | |||
| custody.withdrawAndCall(address(receiver), address(token), amount, data); | |||
| custody.withdrawAndCall(arbitraryCallMessageContext, address(receiver), address(token), amount, data); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a realistic MessageContext.sender in tests
In testForwardCallToReceiveNoParamsThroughCustody, consider setting arbitraryCallMessageContext.sender to a valid address to reflect real-world use cases and improve test reliability.
| /// @param messageContext Message context containing sender. | ||
| /// @param to The address to withdraw tokens to. | ||
| /// @param amount The amount of tokens to withdraw. | ||
| /// @param data The calldata to pass to the contract call. | ||
| /// @param internalSendHash A hash used for internal tracking of the transaction. | ||
| /// @dev This function can only be called by the TSS address, and mints if supply is not reached. | ||
| function withdrawAndCall( | ||
| MessageContext calldata messageContext, |
There was a problem hiding this comment.
💡 Codebase verification
Function withdrawAndCall does not correctly override the base contract. The override keyword is missing in the function definition.
- File:
v2/test/utils/upgrades/ZetaConnectorNonNativeUpgradeTest.sol, Lines: 68-75
🔗 Analysis chain
Ensure function signature correctly overrides base contract
The withdrawAndCall function now includes MessageContext calldata messageContext as a parameter and is marked as override. Please verify that the function signature matches the one in the base contract or interface. A mismatch can lead to compilation errors or unexpected behavior during contract interactions.
Run the following script to verify that the overridden function signatures align:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the `withdrawAndCall` function signature matches in base contracts/interfaces.
# Search for the `withdrawAndCall` function definitions in Solidity files
# Expect: Function signatures should include `MessageContext` as the first parameter
rg --type solidity -A3 -B3 'function withdrawAndCall\('
Length of output: 136
Script:
#!/bin/bash
# Description: Verify that the `withdrawAndCall` function signature matches in base contracts/interfaces.
# Search for the `withdrawAndCall` function definitions in Solidity files
# Expect: Function signatures should include `MessageContext` as the first parameter
rg --glob '*.sol' -A3 -B3 'function withdrawAndCall\('
Length of output: 10212
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (9)
v2/docs/src/contracts/evm/GatewayEVM.sol/contract.GatewayEVM.mdis excluded by!v2/docs/**v2/pkg/erc20custody.t.sol/erc20custodytest.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/gatewayevmzevm.t.sol/gatewayevmzevmtest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornative.t.sol/zetaconnectornativetest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.t.sol/zetaconnectornonnativetest.gois excluded by!v2/pkg/**v2/types/factories/GatewayEVM__factory.tsis excluded by!v2/types/**
📒 Files selected for processing (1)
- v2/contracts/evm/GatewayEVM.sol (4 hunks)
🧰 Additional context used
| // Execute the call on the target contract | ||
| // if sender is provided in messageContext call is authenticated and target is Callable.onCall | ||
| // otherwise, call is arbitrary | ||
| if (messageContext.sender == address(0)) { | ||
| result = _executeArbitraryCall(destination, data); | ||
| } else { | ||
| result = _executeAuthenticatedCall(messageContext, destination, data); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring duplicated conditional logic into a private function
The conditional logic that checks messageContext.sender == address(0) to decide between _executeArbitraryCall and _executeAuthenticatedCall is duplicated in both the execute and executeWithERC20 functions. Extracting this logic into a private helper function can improve maintainability and reduce code duplication.
| if (messageContext.sender == address(0)) { | ||
| _executeArbitraryCall(to, data); | ||
| } else { | ||
| _executeAuthenticatedCall(messageContext, to, data); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Return value of executeWithERC20 is not being captured
The return value of executeWithERC20 is not captured or returned in any of its calls. To ensure proper logging, error handling, or further processing, consider capturing the execution result as done in the execute function.
🔗 Analysis chain
Verify if the execution result in executeWithERC20 should be captured
In the execute function, the result of the call is captured and returned to the caller. However, in executeWithERC20, the result of _executeArbitraryCall or _executeAuthenticatedCall is neither captured nor returned. If the execution result is important for logging, error handling, or further processing, consider capturing and handling it similarly in executeWithERC20.
You can run the following script to check if any usage or overrides of executeWithERC20 expect a return value:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of `executeWithERC20` and check if the return value is utilized.
# Find all Solidity files that call `executeWithERC20`
rg --type solidity 'executeWithERC20\('
# Check if any derived contracts override `executeWithERC20` with a return value
rg --type solidity 'function executeWithERC20\('
Length of output: 158
Script:
#!/bin/bash
# Description: Search for usages of `executeWithERC20` and check if the return value is utilized within Solidity files.
# Find all Solidity files that call `executeWithERC20`
rg 'executeWithERC20\(' --glob '*.sol'
# Check if any derived contracts override `executeWithERC20` with a return value
rg 'function executeWithERC20\(' --glob '*.sol'
Length of output: 1554
Script:
#!/bin/bash
# Description: Check if the return values of `executeWithERC20` are captured or utilized within Solidity files.
# Search for `executeWithERC20` calls that are part of an assignment
rg '=\s*executeWithERC20\(' --glob '*.sol'
# Search for `executeWithERC20` calls that are returned directly
rg 'return\s+executeWithERC20\(' --glob '*.sol'
# Search for `executeWithERC20` calls used in expressions (e.g., inside require or assert)
rg '(require|assert)\s*\(\s*executeWithERC20\(' --glob '*.sol'
Length of output: 159
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
v2/test/ERC20Custody.t.sol (3)
38-38: Consider using a realistic sender address forarbitraryCallMessageContextThe
arbitraryCallMessageContextis initialized withaddress(0)as the sender. For more robust testing, consider using a non-zero address that better represents a realistic scenario. This could help catch potential issues related to sender validation in your contract.You could modify the line as follows:
- MessageContext arbitraryCallMessageContext = MessageContext({ sender: address(0) }); + MessageContext arbitraryCallMessageContext = MessageContext({ sender: address(0x1234) });
343-373: LGTM! Comprehensive test forwithdrawAndCallwith customMessageContextThis new test function thoroughly checks the behavior of
withdrawAndCallwhen called with a specificMessageContext. It verifies token balances, event emissions, and proper handling of the message context.Consider adding a test case where the
senderinMessageContextis set toaddress(0)to ensure the contract handles this edge case correctly.
386-393: LGTM! Crucial security check for preventing arbitraryonRevertinvocationsThis test function effectively verifies that direct calls to
onRevertthroughwithdrawAndCallare not allowed when using an arbitrary call context. This is a critical security measure to prevent potential vulnerabilities.Consider adding a similar test for other sensitive functions (if any) that should not be callable through
withdrawAndCallto ensure comprehensive coverage of this security measure.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (15)
v2/docs/src/contracts/evm/ZetaConnectorNative.sol/contract.ZetaConnectorNative.mdis excluded by!v2/docs/**v2/pkg/erc20custody.t.sol/erc20custodytest.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/gatewayevmzevm.t.sol/gatewayevmzevmtest.gois excluded by!v2/pkg/**v2/pkg/receiverevm.sol/receiverevm.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/zetaconnectornativeupgradetest.sol/zetaconnectornativeupgradetest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.t.sol/zetaconnectornonnativetest.gois excluded by!v2/pkg/**v2/types/ZetaConnectorNative.tsis excluded by!v2/types/**v2/types/ZetaConnectorNativeUpgradeTest.tsis excluded by!v2/types/**v2/types/factories/ReceiverEVM__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNativeUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNative__factory.tsis excluded by!v2/types/**
📒 Files selected for processing (4)
- v2/contracts/evm/ZetaConnectorNative.sol (2 hunks)
- v2/test/ERC20Custody.t.sol (12 hunks)
- v2/test/utils/ReceiverEVM.sol (1 hunks)
- v2/test/utils/upgrades/ZetaConnectorNativeUpgradeTest.sol (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- v2/contracts/evm/ZetaConnectorNative.sol
- v2/test/utils/ReceiverEVM.sol
- v2/test/utils/upgrades/ZetaConnectorNativeUpgradeTest.sol
🧰 Additional context used
🔇 Additional comments (4)
v2/test/ERC20Custody.t.sol (4)
207-207: LGTM! Consider verifying the impact ofarbitraryCallMessageContextThe update correctly incorporates the new
MessageContextparameter in thewithdrawAndCallfunction call. This change aligns with the updated function signature.To ensure this change doesn't affect the test's behavior unexpectedly, you might want to verify that the
arbitraryCallMessageContextis properly handled within thewithdrawAndCallfunction. Consider adding assertions to check if the sender from the context is used as expected in the contract logic.
244-244: LGTM!arbitraryCallMessageContextcorrectly incorporatedThe
withdrawAndCallfunction calls have been properly updated to include thearbitraryCallMessageContextparameter. This change maintains the test's integrity while adapting to the new function signature.Also applies to: 260-260
375-384: LGTM! Important security check for preventing arbitraryonCallinvocationsThis test function effectively verifies that direct calls to
onCallthroughwithdrawAndCallare not allowed when using an arbitrary call context. This is an important security measure to prevent potential vulnerabilities.
286-286: LGTM! Consistent application ofarbitraryCallMessageContextThe
withdrawAndCallfunction calls across various test cases have been consistently updated to include thearbitraryCallMessageContextparameter. This change maintains the integrity of the tests while adapting to the new function signature.To ensure these changes haven't inadvertently altered the behavior of the tests, consider running a diff of the test results before and after these modifications. This will help verify that the introduction of
arbitraryCallMessageContexthasn't changed the expected outcomes of these tests.Also applies to: 296-296, 306-306, 324-324, 402-402, 412-412, 429-429, 462-462
| // Execute the call on the target contract | ||
| // if sender is provided in messageContext call is authenticated and target is Callable.onCall | ||
| // otherwise, call is arbitrary | ||
| if (messageContext.sender == address(0)) { |
There was a problem hiding this comment.
nit: this can cause problems to developer experience if not properly documented what it means to fill or not the sender.
There was a problem hiding this comment.
this part is called by protocol, but i see your point, i think currently comment explain what its doing: sender = authenticated call, no sender = arbitrary call
when devs use this feature, they would set up callOptions.isArbitrary field on zevm side, so there is more important from devex side of things
Summary by CodeRabbit
Release Notes
New Features
MessageContextparameter in multiple contract functions for enhanced context during withdrawals and executions.Bug Fixes
Tests