fix: review fixes #383
fix: review fixes #383
Conversation
skosito
requested review from
andresaiello,
brewmaster012,
lumtis,
CharlieMc0,
fadeev and
fbac
as code owners
📝 WalkthroughWalkthroughThe pull request introduces significant modifications across multiple contracts, focusing on the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #383 +/- ##
==========================================
+ Coverage 84.80% 84.95% +0.14%
==========================================
Files 8 8
Lines 408 412 +4
Branches 134 138 +4
==========================================
+ Hits 346 350 +4
Misses 62 62 ☔ 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: 2
🧹 Outside diff range and nitpick comments (10)
v2/contracts/evm/interfaces/IZetaConnector.sol (1)
28-29: Minor correction needed in the event comment.The comment for the
oldTSSAddressparameter incorrectly describes it as the "new tss address". This should be updated to accurately reflect that it's the old TSS address.Here's the suggested correction:
- /// @param oldTSSAddress new tss address + /// @param oldTSSAddress old tss address /// @param newTSSAddress new tss addressv2/contracts/evm/interfaces/IERC20Custody.sol (1)
45-47: Approved: Enhanced event signature and improved documentationThe changes to the
UpdatedCustodyTSSAddressevent are well-implemented and improve the contract's transparency. Including both the old and new TSS addresses in the event allows for better tracking of historical changes.Consider updating the parameter descriptions for clarity:
/// @param oldTSSAddress The previous TSS address /// @param newTSSAddress The updated TSS addressv2/contracts/evm/ERC20Custody.sol (3)
72-74: LGTM! Consider adding a NatSpec comment for the event.The changes to the
updateTSSAddressfunction improve transparency by emitting an event before updating the state. The function signature update also enhances security by restricting access to admin roles.Consider adding a NatSpec comment for the
UpdatedCustodyTSSAddressevent at the contract level for better documentation. For example:/// @notice Emitted when the TSS address is updated /// @param oldTSSAddress The previous TSS address /// @param newTSSAddress The new TSS address event UpdatedCustodyTSSAddress(address oldTSSAddress, address newTSSAddress);
Line range hint
185-185: Consider a more robust approach for handling the deprecateddepositfunction.While marking the function as deprecated is a good first step, it may not be sufficient to prevent its usage. Consider the following suggestions:
- Add a deprecation notice in the function body that emits an event or logs a warning when the function is called.
- Implement a time-based approach where the function becomes non-functional after a certain date.
- If possible, remove the function entirely and update any dependent contracts or interfaces.
Here's an example of how you could implement suggestion 1:
event DeprecatedFunctionCalled(string functionName); function deposit( bytes calldata recipient, IERC20 asset, uint256 amount, bytes calldata message ) external nonReentrant whenNotPaused { emit DeprecatedFunctionCalled("deposit"); // ... rest of the function }This approach provides clearer signals to users and allows for easier tracking of deprecated function usage.
Line range hint
1-214: Overall, the changes improve contract functionality and transparency.The modifications to the
ERC20Custodycontract, particularly theupdateTSSAddressfunction, enhance transparency and security. The deprecation of thedepositfunction aligns with the PR objectives.However, consider the following recommendations for further improvement:
- Add NatSpec comments for events to improve documentation.
- Implement a more robust approach for handling the deprecated
depositfunction.- Ensure that any external contracts or interfaces depending on the
depositfunction are updated accordingly.To maintain consistency across the codebase and prevent potential issues, it's crucial to update any external contracts or interfaces that might be relying on the now-deprecated
depositfunction. Consider creating a migration plan for users of this function if it's going to be removed in the future.v2/test/GatewayEVM.t.sol (4)
444-451: Maintain Consistent Revert Expectation MethodsIn the
testDepositERC20ToCustodyFailsIfPayloadSizeExceededfunction, you're usingvm.expectRevert(PayloadSizeExceeded.selector);to expect a revert. However, in other tests, you use the revert message string. For consistency, it's recommended to adopt a uniform approach across all tests.Consider updating the code to use the error message string:
-vm.expectRevert(PayloadSizeExceeded.selector); +vm.expectRevert("PayloadSizeExceeded");Alternatively, if you prefer using error selectors, update the other tests to match this style.
499-503: Inconsistent Revert Expectation StyleIn
testFailDepositEthToTssIfPayloadSizeExceeded, you usevm.expectRevert("PayloadSizeExceeded");to expect the revert. This is inconsistent with the previous test where you use the error selector. Consistency improves readability and reduces potential confusion.Update the code to use the error selector:
-vm.expectRevert("PayloadSizeExceeded"); +vm.expectRevert(PayloadSizeExceeded.selector);
590-590: Apply Payload Size Constant for ConsistencySimilarly, at line 590, you set
revertOptions.revertMessage = new bytes(513);. Using the definedMAX_REVERT_MESSAGE_SIZEconstant here will maintain consistency across your tests.Update the code as follows:
-revertOptions.revertMessage = new bytes(513); +revertOptions.revertMessage = new bytes(MAX_REVERT_MESSAGE_SIZE + 1);
622-622: Consistent Use of Payload Size ConstantAt line 622, in
testCallWithPayloadFailsIfPayloadSizeExceeded, consider using theMAX_REVERT_MESSAGE_SIZEconstant to ensure consistency.Apply this change:
-revertOptions.revertMessage = new bytes(513); +revertOptions.revertMessage = new bytes(MAX_REVERT_MESSAGE_SIZE + 1);v2/test/GatewayZEVM.t.sol (1)
123-127: Use a constant for maximum message size limitIn
testWithdrawZRC20FailsIfMessageSizeExceeded, consider defining a constant for the maximum allowed message size instead of hardcoding the value1025. This enhances readability and maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (89)
v2/docs/src/contracts/Revert.sol/interface.Revertable.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/struct.RevertContext.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/struct.RevertOptions.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ERC20Custody.sol/contract.ERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/GatewayEVM.sol/contract.GatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorBase.sol/abstract.ZetaConnectorBase.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorNative.sol/contract.ZetaConnectorNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorNonNative.sol/contract.ZetaConnectorNonNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20CustodyErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20CustodyEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.Callable.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/struct.MessageContext.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IZetaConnector.sol/interface.IZetaConnectorEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IZetaNonEthNew.sol/interface.IZetaNonEthNew.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/GatewayZEVM.sol/contract.GatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/SystemContract.sol/contract.SystemContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/SystemContract.sol/interface.SystemContractErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/ZRC20.sol/contract.ZRC20.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/ZRC20.sol/interface.ZRC20Errors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/struct.CallOptions.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/ISystem.sol/interface.ISystem.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IWZETA.sol/interface.IWETH9.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/enum.CoinType.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.IZRC20.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.IZRC20Metadata.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.ZRC20Events.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.UniversalContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.zContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/struct.zContext.mdis excluded by!v2/docs/**v2/pkg/erc20custody.sol/erc20custody.gois excluded by!v2/pkg/**v2/pkg/erc20custody.t.sol/erc20custodytest.gois excluded by!v2/pkg/**v2/pkg/erc20custodyechidnatest.sol/erc20custodyechidnatest.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.sol/gatewayevm.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevmtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmechidnatest.sol/gatewayevmechidnatest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmupgrade.t.sol/gatewayevmuupsupgradetest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmupgradetest.sol/gatewayevmupgradetest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmzevm.t.sol/gatewayevmzevmtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.sol/gatewayzevm.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.t.sol/gatewayzevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.t.sol/gatewayzevmoutboundtest.gois excluded by!v2/pkg/**v2/pkg/ierc20custody.sol/ierc20custody.gois excluded by!v2/pkg/**v2/pkg/ierc20custody.sol/ierc20custodyevents.gois excluded by!v2/pkg/**v2/pkg/igatewayevm.sol/igatewayevm.gois excluded by!v2/pkg/**v2/pkg/igatewayevm.sol/igatewayevmevents.gois excluded by!v2/pkg/**v2/pkg/izetaconnector.sol/izetaconnectorevents.gois excluded by!v2/pkg/**v2/pkg/receiverevm.sol/receiverevm.gois excluded by!v2/pkg/**v2/pkg/zetaconnectorbase.sol/zetaconnectorbase.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornative.sol/zetaconnectornative.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornative.t.sol/zetaconnectornativetest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.sol/zetaconnectornonnative.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.t.sol/zetaconnectornonnativetest.gois excluded by!v2/pkg/**v2/pkg/zrc20.t.sol/zrc20test.gois excluded by!v2/pkg/**v2/types/ERC20Custody.tsis excluded by!v2/types/**v2/types/ERC20CustodyEchidnaTest.tsis excluded by!v2/types/**v2/types/GatewayEVM.tsis excluded by!v2/types/**v2/types/GatewayEVMEchidnaTest.tsis excluded by!v2/types/**v2/types/GatewayEVMUpgradeTest.tsis excluded by!v2/types/**v2/types/IERC20Custody.sol/IERC20Custody.tsis excluded by!v2/types/**v2/types/IERC20Custody.sol/IERC20CustodyEvents.tsis excluded by!v2/types/**v2/types/IGatewayEVM.sol/IGatewayEVM.tsis excluded by!v2/types/**v2/types/IGatewayEVM.sol/IGatewayEVMEvents.tsis excluded by!v2/types/**v2/types/IZetaConnector.sol/IZetaConnectorEvents.tsis excluded by!v2/types/**v2/types/ZetaConnectorBase.tsis excluded by!v2/types/**v2/types/ZetaConnectorNative.tsis excluded by!v2/types/**v2/types/ZetaConnectorNonNative.tsis excluded by!v2/types/**v2/types/factories/ERC20CustodyEchidnaTest__factory.tsis excluded by!v2/types/**v2/types/factories/ERC20Custody__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVMEchidnaTest__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVMUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVM__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayZEVM__factory.tsis excluded by!v2/types/**v2/types/factories/IERC20Custody.sol/IERC20CustodyEvents__factory.tsis excluded by!v2/types/**v2/types/factories/IERC20Custody.sol/IERC20Custody__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayEVM.sol/IGatewayEVMEvents__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayEVM.sol/IGatewayEVM__factory.tsis excluded by!v2/types/**v2/types/factories/IZetaConnector.sol/IZetaConnectorEvents__factory.tsis excluded by!v2/types/**v2/types/factories/ReceiverEVM__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorBase__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNative__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNonNative__factory.tsis excluded by!v2/types/**
📒 Files selected for processing (12)
- v2/contracts/evm/ERC20Custody.sol (1 hunks)
- v2/contracts/evm/GatewayEVM.sol (12 hunks)
- v2/contracts/evm/ZetaConnectorBase.sol (1 hunks)
- v2/contracts/evm/ZetaConnectorNonNative.sol (1 hunks)
- v2/contracts/evm/interfaces/IERC20Custody.sol (1 hunks)
- v2/contracts/evm/interfaces/IGatewayEVM.sol (1 hunks)
- v2/contracts/evm/interfaces/IZetaConnector.sol (1 hunks)
- v2/contracts/zevm/GatewayZEVM.sol (11 hunks)
- v2/test/ERC20Custody.t.sol (1 hunks)
- v2/test/GatewayEVM.t.sol (6 hunks)
- v2/test/GatewayZEVM.t.sol (8 hunks)
- v2/test/ZetaConnectorNative.t.sol (1 hunks)
🧰 Additional context used
🔇 Additional comments (31)
v2/contracts/evm/interfaces/IZetaConnector.sol (1)
30-30: LGTM: Event signature update improves traceability.The addition of the
oldTSSAddressparameter to theUpdatedZetaConnectorTSSAddressevent is a good improvement. It enhances the event's informativeness by allowing tracking of both the old and new TSS addresses when an update occurs.Let's verify the impact of this change on the codebase:
✅ Verification successful
Action Required: Verification Scripts Adjustment Needed
The previous verification attempt failed due to an unrecognized file type. To accurately verify the impact of the
UpdatedZetaConnectorTSSAddressevent signature change, new scripts targeting.solfiles are necessary.
: Event signature update confirmed.
All instances of the
UpdatedZetaConnectorTSSAddressevent have been successfully updated to include botholdTSSAddressandnewTSSAddressparameters. The old event signature is no longer present, and allemitstatements correctly use the new signature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for occurrences of the old event signature and verify that all emits use the new signature. # Test 1: Search for old event signature. Expect: No results. echo "Searching for old event signature..." rg --type solidity "event UpdatedZetaConnectorTSSAddress\(address newTSSAddress\)" # Test 2: Verify all emit statements use the new signature. Expect: All results to have two address parameters. echo "Verifying emit statements..." rg --type solidity "emit UpdatedZetaConnectorTSSAddress\("Length of output: 361
Script:
#!/bin/bash # Description: Check for occurrences of the old event signature and verify that all emits use the new signature within .sol files. # Test 1: Search for old event signature. Expect: No results. echo "Searching for old event signature..." rg 'event UpdatedZetaConnectorTSSAddress\(address newTSSAddress\)' --type-add 'solidity:*.sol' --type solidity # Test 2: Verify all emit statements use the new signature with two address parameters. echo "Verifying emit statements with updated signature..." rg 'emit UpdatedZetaConnectorTSSAddress\([^,]+, [^)]+\)' --type-add 'solidity:*.sol' --type solidityLength of output: 628
v2/contracts/evm/ZetaConnectorNonNative.sol (2)
120-123: Approved: Improved encapsulation by changing_mintToto privateThe change of
_mintTofunction visibility frominternaltoprivateis a good security practice. It restricts the function's accessibility to only within theZetaConnectorNonNativecontract, preventing potential misuse in derived contracts and adhering to the principle of least privilege.This modification enhances the contract's security without affecting its core functionality. The function's internal logic remains unchanged, maintaining its checks for max supply and using the appropriate interface for minting.
Line range hint
1-124: Overall impact: Improved security with minimal riskThe change to make
_mintToprivate improves the contract's security by restricting access to this critical function. This modification aligns well with the PR's objective of implementing fixes from previous reviews.The change doesn't affect the overall functionality of the contract, as the internal logic of
_mintToand the functions that use it (withdraw, withdrawAndCall, withdrawAndRevert) remain unchanged.To ensure no breaking changes were introduced, please run the following script to check for any derived contracts or external calls to
_mintTo:If the script returns any results, please review them to ensure that changing
_mintToto private doesn't break any existing functionality.✅ Verification successful
Verified: Changing
_mintToto private does not impact any external contracts or functionality. All calls to_mintToare internal withinZetaConnectorNonNative.sol, ensuring enhanced security without introducing breaking changes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential derived contracts or external calls to _mintTo echo "Searching for potential derived contracts:" rg --type solidity "contract \w+ is ZetaConnectorNonNative" echo "Searching for external calls to _mintTo:" rg --type solidity "_mintTo\("Length of output: 346
Script:
#!/bin/bash # Search for potential derived contracts using .sol extension echo "Searching for potential derived contracts:" rg -g '*.sol' 'contract\s+\w+\s+is\s+ZetaConnectorNonNative' echo "Searching for external calls to _mintTo:" rg -g '*.sol' '_mintTo\('Length of output: 706
v2/contracts/evm/ZetaConnectorBase.sol (1)
64-66: Improved event emission order inupdateTSSAddressThe changes to the
updateTSSAddressfunction improve its implementation by emitting theUpdatedZetaConnectorTSSAddressevent after updating thetssAddressstate variable. This ensures that the event accurately reflects the new state when emitted, which is considered a best practice.The core logic and security measures of the function remain intact, including the zero address check and proper role management.
v2/contracts/evm/interfaces/IGatewayEVM.sol (1)
56-56: Verify impact of event signature changeThe change in the
UpdatedGatewayTSSAddressevent signature is localized to theIGatewayEVMEventsinterface. However, this change may affect other parts of the codebase.Please run the following script to identify potential areas that need to be updated:
Ensure that all identified usages are updated to match the new event signature.
✅ Verification successful
Re-run verification with adjusted search parameters
The previous script failed to recognize Solidity files. Please execute the following updated script to locate usages of the
UpdatedGatewayTSSAddressevent:
Impact of
UpdatedGatewayTSSAddressEvent Signature Change VerifiedThe
UpdatedGatewayTSSAddressevent signature change in theIGatewayEVM.solinterface has been properly localized. All event emissions and interface implementations identified in the test and contract files have been reviewed to ensure compatibility with the new signature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of UpdatedGatewayTSSAddress event in the codebase # Search for event emissions echo "Searching for UpdatedGatewayTSSAddress event emissions:" rg --type solidity "emit UpdatedGatewayTSSAddress\(" -A 1 # Search for event listeners echo "\nSearching for UpdatedGatewayTSSAddress event listeners:" rg --type solidity "UpdatedGatewayTSSAddress\(.*\)" -A 1 # Search for interface implementations echo "\nSearching for IGatewayEVMEvents interface implementations:" rg --type solidity "contract.*is.*IGatewayEVMEvents"Length of output: 649
Script:
#!/bin/bash # Description: Find usages of UpdatedGatewayTSSAddress event in Solidity (.sol) files # Search for event emissions echo "Searching for UpdatedGatewayTSSAddress event emissions in .sol files:" rg --glob "*.sol" "emit UpdatedGatewayTSSAddress\(" -A 1 # Search for event listeners echo "\nSearching for UpdatedGatewayTSSAddress event listeners in .sol files:" rg --glob "*.sol" "UpdatedGatewayTSSAddress\(.*\)" -A 1 # Search for interface implementations echo "\nSearching for IGatewayEVMEvents interface implementations in .sol files:" rg --glob "*.sol" "contract\s+.*\s+is\s+.*IGatewayEVMEvents"Length of output: 1952
v2/test/ZetaConnectorNative.t.sol (3)
96-96: Improved event emission for better traceabilityThe update to include both the old and new TSS addresses in the
UpdatedZetaConnectorTSSAddressevent emission is a positive change. This modification enhances the traceability of TSS address updates, allowing for easier tracking and auditing of these critical changes.
Line range hint
321-325: Excellent addition of zero address validation testThe new
testTSSUpgradeFailsIfZeroAddressfunction is a valuable addition to the test suite. It verifies that theupdateTSSAddressfunction correctly rejects attempts to set the TSS address to a zero address, which is a critical security measure. This test enhances the robustness of the contract by ensuring proper input validation and error handling.
Line range hint
1-434: Summary: Improved TSS address update process and testingThe changes in this file focus on enhancing the TSS address update process and its associated tests. The modifications include:
- Improved event emission for
UpdatedZetaConnectorTSSAddress, now including both old and new TSS addresses for better traceability.- Addition of a new test
testTSSUpgradeFailsIfZeroAddressto ensure proper handling of zero address inputs.These changes align with best practices for event emission, input validation, and thorough testing. They contribute to the overall robustness and security of the ZetaConnectorNative contract.
v2/contracts/evm/GatewayEVM.sol (8)
84-86: Improved transparency in TSS address updatesThe addition of the
UpdatedGatewayTSSAddressevent emission enhances the contract's transparency by providing a clear audit trail of TSS address changes. This is a positive improvement that allows for better off-chain tracking and monitoring of critical contract updates.
193-199: Enhanced security in ERC20 approvalsThe changes to the
executeWithERC20function improve both code readability and security:
- Renaming
resetApprovalto_resetApprovalfollows the convention for private functions, enhancing code clarity.- Calling
_resetApprovalboth before and after the main execution ensures that no lingering approvals remain, reducing the risk of unauthorized token transfers.These modifications represent a positive step towards more secure ERC20 token handling.
204-204: Improved consistency in function namingThe renaming of
transferToAssetHandlerto_transferToAssetHandleraligns with the naming convention for private functions. This change enhances code readability and maintains consistency throughout the contract.
252-252: Added safety check for revert message sizeA new validation has been introduced to ensure that the
revertOptions.revertMessagelength does not exceedMAX_PAYLOAD_SIZE. This is a valuable safety measure that prevents potential issues related to excessively large revert messages, such as out-of-gas errors or unexpected behavior in cross-chain communications.
278-280: Consistent improvements in deposit functions
- The addition of the revert message size check (
revertOptions.revertMessage.length > MAX_PAYLOAD_SIZE) aligns with the ETH deposit function, ensuring consistent safety measures across different deposit types.- Renaming
transferFromToAssetHandlerto_transferFromToAssetHandlermaintains the naming convention for private functions, enhancing code readability and consistency throughout the contract.These changes demonstrate a systematic approach to improving both safety and code quality.
301-301: Enhanced payload size management in depositAndCall functionsBoth ETH and ERC20
depositAndCallfunctions have been updated with improved payload size checks:
The combined length of
payloadandrevertOptions.revertMessageis now checked againstMAX_PAYLOAD_SIZE. This comprehensive check prevents potential issues related to excessively large payloads in cross-chain communications.In the ERC20 version,
transferFromToAssetHandlerhas been consistently renamed to_transferFromToAssetHandler, maintaining the naming convention for private functions.These changes demonstrate a thorough approach to payload size management and code consistency.
Also applies to: 329-331
350-350: Consistent payload size check in call functionThe
callfunction has been updated to include a check on the combined length ofpayloadandrevertOptions.revertMessageagainstMAX_PAYLOAD_SIZE. This change aligns with similar modifications in the deposit and depositAndCall functions, ensuring consistent payload size management across all relevant contract interactions.This update contributes to the overall robustness of the contract by preventing potential issues related to excessively large payloads in cross-chain communications.
Line range hint
1-470: Summary of GatewayEVM.sol changesThe modifications to the GatewayEVM contract demonstrate a comprehensive approach to improving code quality, security, and functionality:
- Enhanced transparency through event emissions for critical operations.
- Improved security in ERC20 token handling with double approval resets.
- Consistent naming conventions for private functions, enhancing code readability.
- Robust payload size management across all relevant functions, preventing potential issues in cross-chain communications.
- Maintained consistency in error handling and existing functionality.
These changes collectively contribute to a more secure, transparent, and maintainable contract. The systematic nature of the improvements suggests a thorough code review and refactoring process.
v2/contracts/zevm/GatewayZEVM.sol (6)
94-94: Improved encapsulation by changing function visibility to privateThe change from
internaltoprivatefor the_withdrawZRC20function enhances the contract's security by restricting access to this function. This modification prevents potential misuse in derived contracts and reduces the overall attack surface.
104-104: Enhanced security by changing function visibility to privateThe visibility change from
internaltoprivatefor the_withdrawZRC20WithGasLimitfunction improves the contract's security posture. This modification ensures that the function can only be called within the contract itself, preventing potential misuse in derived contracts.
122-122: Strengthened encapsulation by changing function visibility to privateThe modification of the
_transferZETAfunction's visibility frominternaltoprivatealigns with best practices for encapsulation. This change restricts the function's accessibility to within the contract, reducing the risk of unintended use in derived contracts and enhancing overall security.
146-146: Verify intention of relaxed message size checkThe condition for checking
revertOptions.revertMessage.lengthhas been changed from>=to>. This subtle modification now allows messages of exactlyMAX_MESSAGE_SIZElength, whereas previously it would have reverted.Please confirm if this change is intentional and if there are any potential implications or edge cases that need to be considered with this relaxed condition.
393-393: Improved contract security by changing function visibility to privateThe visibility change of the
_callfunction frominternaltoprivateenhances the contract's security and encapsulation. This modification ensures that the function can only be invoked within the contract itself, preventing potential misuse or unexpected behavior in derived contracts.
185-185: Consistent relaxation of message size checks across multiple functionsThe condition for checking message length (including
revertOptions.revertMessage.length) has been consistently changed from>=to>across multiple functions:
withdrawAndCall(lines 185, 224, 293, 332)withdraw(line 257)call(lines 358, 381)This change allows messages of exactly
MAX_MESSAGE_SIZElength in all these functions, whereas previously they would have reverted.Please confirm:
- Is this change intentional and applied consistently as part of a broader design decision?
- Have potential implications or edge cases been considered, especially in cross-function interactions?
- Are there any updates needed in the documentation or external interfaces to reflect this change in behavior?
To help verify the consistency and impact of these changes, you can run the following script:
Also applies to: 224-224, 257-257, 293-293, 332-332, 358-358, 381-381
✅ Verification successful
Message size checks consistently updated and verified
The message size condition has been uniformly changed from
>=to>across all relevant functions:
withdrawAndCall(lines 185, 224, 293, 332)withdraw(line 257)call(lines 358, 381)Additionally, the
MAX_MESSAGE_SIZEconstant is correctly defined as1024.These changes allow messages with a length exactly equal to
MAX_MESSAGE_SIZE, aligning with the intended contract behavior.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of message size checks across the contract # Test: Search for all occurrences of message size checks rg --type solidity "revertOptions\.revertMessage\.length \s*>\s*MAX_MESSAGE_SIZE" v2/contracts/zevm/GatewayZEVM.sol # Test: Check if there are any remaining occurrences of the old condition rg --type solidity "revertOptions\.revertMessage\.length \s*>=\s*MAX_MESSAGE_SIZE" v2/contracts/zevm/GatewayZEVM.sol # Test: Verify the value of MAX_MESSAGE_SIZE constant rg --type solidity "MAX_MESSAGE_SIZE\s*=\s*\d+" v2/contracts/zevm/GatewayZEVM.solLength of output: 419
Script:
#!/bin/bash # Description: Verify the consistency of message size checks across the contract # Test: Search for all occurrences of message size checks using '*.sol' extension rg -g '*.sol' "revertOptions\.revertMessage\.length\s*>\s*MAX_MESSAGE_SIZE" v2/contracts/zevm/GatewayZEVM.sol # Test: Check if there are any remaining occurrences of the old condition using '*.sol' extension rg -g '*.sol' "revertOptions\.revertMessage\.length\s*>=\s*MAX_MESSAGE_SIZE" v2/contracts/zevm/GatewayZEVM.sol # Test: Verify the value of MAX_MESSAGE_SIZE constant using '*.sol' extension rg -g '*.sol' "MAX_MESSAGE_SIZE\s*=\s*\d+" v2/contracts/zevm/GatewayZEVM.solLength of output: 1234
v2/test/ERC20Custody.t.sol (1)
99-99: Improved event emission for TSS address updates.The updated event emission now includes both the old and new TSS addresses, which enhances the traceability and auditability of TSS address changes. This modification aligns well with best practices for event logging in smart contracts.
v2/test/GatewayEVM.t.sol (1)
92-92: Ensure Event Definition Matches Emitted EventAt line 92, the
emit UpdatedGatewayTSSAddress(tssAddress, newTSSAddress);statement now emits both the old and new TSS addresses. Please verify that theUpdatedGatewayTSSAddressevent definition has been updated accordingly to accept twoaddressparameters.To confirm the event definition, you can run the following script:
✅ Verification successful
Event Definition Verified
The
UpdatedGatewayTSSAddressevent is correctly defined with twoaddressparameters.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the UpdatedGatewayTSSAddress event is defined with two address parameters. ast-grep --lang solidity --pattern 'event UpdatedGatewayTSSAddress(address, address);'Length of output: 205
Script:
#!/bin/bash # Description: Verify that the UpdatedGatewayTSSAddress event is defined with two address parameters using ripgrep. rg --type solidity 'event\s+UpdatedGatewayTSSAddress\s*\(\s*address\s+\w+\s*,\s*address\s+\w+\s*\)\s*;' .Length of output: 140
Script:
#!/bin/bash # Description: Verify that the UpdatedGatewayTSSAddress event is defined with two address parameters in Solidity files using ripgrep. rg 'event\s+UpdatedGatewayTSSAddress\s*\(\s*address\s+\w+\s*,\s*address\s+\w+\s*\)\s*;' --glob '*.sol'Length of output: 230
v2/test/GatewayZEVM.t.sol (7)
163-163: Use a constant for maximum message size limitSimilarly, in
testWithdrawAndCallZRC20FailsIfMessageSizeExceeded, using a defined constant for the maximum message size improves code clarity and consistency.
232-232: Use a constant for maximum message size limitAgain, consider defining a constant for the maximum allowed message size in this test to enhance maintainability.
298-302: Use a constant for maximum message size limitIn
testWithdrawZETAFailsIfMessageSizeExceeded, replacing the hardcoded value1025with a defined constant for the maximum message size will improve code readability and ease future updates.
317-317: Use a constant for maximum message size limitAs before, using a defined constant for the maximum message size in this test will enhance code maintainability.
336-336: Use a constant for maximum message size limitConsider using a constant for the maximum allowed message size in this test to improve consistency across the codebase.
554-554: Use a constant for maximum message size limitIn this test, defining a constant for the maximum message size instead of hardcoding
513will enhance readability and maintainability.
581-581: Use a constant for maximum message size limitSimilarly, consider using a defined constant for the maximum message size in this test to ensure consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (61)
v2/docs/src/contracts/Revert.sol/interface.Revertable.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/struct.RevertContext.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/struct.RevertOptions.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ERC20Custody.sol/contract.ERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/GatewayEVM.sol/contract.GatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorBase.sol/abstract.ZetaConnectorBase.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorNative.sol/contract.ZetaConnectorNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorNonNative.sol/contract.ZetaConnectorNonNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20CustodyErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20CustodyEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.Callable.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/struct.MessageContext.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IZetaConnector.sol/interface.IZetaConnectorEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IZetaNonEthNew.sol/interface.IZetaNonEthNew.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/GatewayZEVM.sol/contract.GatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/SystemContract.sol/contract.SystemContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/SystemContract.sol/interface.SystemContractErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/ZRC20.sol/contract.ZRC20.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/ZRC20.sol/interface.ZRC20Errors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/struct.CallOptions.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/ISystem.sol/interface.ISystem.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IWZETA.sol/interface.IWETH9.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/enum.CoinType.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.IZRC20.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.IZRC20Metadata.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.ZRC20Events.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.UniversalContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.zContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/struct.zContext.mdis excluded by!v2/docs/**v2/pkg/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.t.sol/gatewayzevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.t.sol/gatewayzevmoutboundtest.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/zetaconnectornonnative.sol/zetaconnectornonnative.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.t.sol/zetaconnectornonnativetest.gois excluded by!v2/pkg/**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/ReceiverEVM__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNative__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNonNative__factory.tsis excluded by!v2/types/**
📒 Files selected for processing (5)
- v2/contracts/evm/interfaces/IERC20Custody.sol (1 hunks)
- v2/contracts/evm/interfaces/IGatewayEVM.sol (1 hunks)
- v2/contracts/evm/interfaces/IZetaConnector.sol (1 hunks)
- v2/test/GatewayEVM.t.sol (6 hunks)
- v2/test/GatewayZEVM.t.sol (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- v2/contracts/evm/interfaces/IERC20Custody.sol
- v2/contracts/evm/interfaces/IGatewayEVM.sol
- v2/contracts/evm/interfaces/IZetaConnector.sol
- v2/test/GatewayEVM.t.sol
🧰 Additional context used
🔇 Additional comments (7)
v2/test/GatewayZEVM.t.sol (7)
123-127: Test accurately checks message size limit forwithdrawThe new test
testWithdrawZRC20FailsIfMessageSizeExceededcorrectly verifies thatwithdrawreverts withMessageSizeExceededwhenrevertOptions.revertMessageexceedsMAX_MESSAGE_SIZE.
231-232: Duplicate of previous suggestion to refactor repetitive test codeThe same setup code is repeated here. Refactoring can improve maintainability.
298-302: Test correctly verifies message size limit for ZETA withdrawalThe test
testWithdrawZETAFailsIfMessageSizeExceededaccurately checks thatwithdrawreverts withMessageSizeExceededwhenrevertOptions.revertMessageexceedsMAX_MESSAGE_SIZE.
316-317: Duplicate of previous suggestion to refactor repetitive test codeReiterating the suggestion to refactor the message size setup code.
335-336: Duplicate of previous suggestion to refactor repetitive test codeRefactoring repeated code enhances maintainability.
553-554: Duplicate of previous suggestion to refactor repetitive test codeConsider extracting repeated setup code into a helper.
580-581: Duplicate of previous suggestion to refactor repetitive test codeSame as above.
| bytes memory message = new bytes(gateway.MAX_MESSAGE_SIZE() / 2); | ||
| revertOptions.revertMessage = new bytes(gateway.MAX_MESSAGE_SIZE() / 2 + 1); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring repetitive test setup code for exceeding message size
The code initializing message and revertOptions.revertMessage to exceed MAX_MESSAGE_SIZE is repeated across multiple tests. Refactoring this code into a helper function or utility can reduce duplication and improve maintainability.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
v2/contracts/evm/ZetaConnectorNonNative.sol (1)
Line range hint
1-129: Suggestions for improving contract documentation and security
Consider adding a comment at the beginning of the contract to explicitly state that it's upgradeable. This helps developers understand the contract's nature and the need for careful state management during upgrades.
It's recommended to conduct a thorough security audit, particularly focusing on the interactions with the
gatewaycontract. External contract calls can be a source of vulnerabilities if not properly secured.Here's a suggested comment to add at the beginning of the contract:
/// @dev This contract is upgradeable. Be careful when adding new state variables /// and ensure proper initialization in the initialize function. contract ZetaConnectorNonNative is ZetaConnectorBase { // ... rest of the contract }v2/test/utils/upgrades/ERC20CustodyUpgradeTest.sol (1)
Line range hint
1-224: Consider future handling of deprecated functions and update documentation.The contract looks well-structured and follows best practices for upgradeable contracts. However, there are a couple of points to consider:
The
depositfunction is marked as deprecated. In future upgrades, you might want to consider removing this function entirely or providing a clear migration path for users.The contract name
ERC20CustodyUpgradeTestand the comment/// @dev The only difference is in event namingmight be slightly misleading, as there are functional changes beyond just event naming (e.g., theWithdrawnV2event in thewithdrawfunction).Consider the following actions:
- Plan for the removal of the deprecated
depositfunction in future upgrades.- Update the contract documentation to accurately reflect all changes, not just event naming differences.
- If this contract is intended for testing upgrades, consider adding more comprehensive tests to cover all changes and potential upgrade scenarios.
v2/test/ERC20Custody.t.sol (1)
110-110: LGTM! Consider adding a test for the old address.The update to include both the old and new TSS addresses in the
UpdatedCustodyTSSAddressevent is a good improvement. It enhances traceability and provides more context for the address change.Consider adding an assertion to verify that the
tssAddressvariable holds the correct old address before the update. This would ensure that the test is using the expected initial state:+ assertEq(tssAddress, custody.tssAddress()); vm.expectEmit(true, true, true, true, address(custody)); emit UpdatedCustodyTSSAddress(tssAddress, newTSSAddress); custody.updateTSSAddress(newTSSAddress);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (69)
v2/docs/src/contracts/Revert.sol/interface.Revertable.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/struct.RevertContext.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/struct.RevertOptions.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ERC20Custody.sol/contract.ERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/GatewayEVM.sol/contract.GatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorBase.sol/abstract.ZetaConnectorBase.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorNative.sol/contract.ZetaConnectorNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorNonNative.sol/contract.ZetaConnectorNonNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20CustodyErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20CustodyEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.Callable.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/struct.MessageContext.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IZetaConnector.sol/interface.IZetaConnectorEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IZetaNonEthNew.sol/interface.IZetaNonEthNew.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/GatewayZEVM.sol/contract.GatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/SystemContract.sol/contract.SystemContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/SystemContract.sol/interface.SystemContractErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/ZRC20.sol/contract.ZRC20.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/ZRC20.sol/interface.ZRC20Errors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/struct.CallOptions.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/ISystem.sol/interface.ISystem.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IWZETA.sol/interface.IWETH9.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/enum.CoinType.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.IZRC20.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.IZRC20Metadata.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.ZRC20Events.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.UniversalContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.zContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/struct.zContext.mdis excluded by!v2/docs/**v2/pkg/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/gatewayzevm.t.sol/gatewayzevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.t.sol/gatewayzevmoutboundtest.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/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/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 (9)
- v2/contracts/evm/ERC20Custody.sol (1 hunks)
- v2/contracts/evm/GatewayEVM.sol (12 hunks)
- v2/contracts/evm/ZetaConnectorBase.sol (1 hunks)
- v2/contracts/evm/ZetaConnectorNonNative.sol (1 hunks)
- v2/test/ERC20Custody.t.sol (1 hunks)
- v2/test/GatewayEVM.t.sol (6 hunks)
- v2/test/GatewayZEVM.t.sol (8 hunks)
- v2/test/ZetaConnectorNative.t.sol (1 hunks)
- v2/test/utils/upgrades/ERC20CustodyUpgradeTest.sol (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- v2/contracts/evm/ERC20Custody.sol
- v2/contracts/evm/ZetaConnectorBase.sol
- v2/test/GatewayEVM.t.sol
- v2/test/ZetaConnectorNative.t.sol
🧰 Additional context used
🔇 Additional comments (13)
v2/contracts/evm/ZetaConnectorNonNative.sol (1)
126-129: Approved: Improved encapsulation of minting logicChanging
_mintTofrominternaltoprivateenhances the security of the minting process by restricting access to only within this contract. This change aligns with the principle of least privilege and reduces the potential attack surface.To ensure this change doesn't introduce any regressions, please verify:
- No derived contracts were relying on this function.
- This change doesn't impact any upgrade patterns used in the system.
Run the following script to check for any derived contracts or references to
_mintTo:✅ Verification successful
Approved: Improved encapsulation of minting logic
Changing
_mintTofrominternaltoprivateenhances the security of the minting process by restricting access to only within this contract. This change aligns with the principle of least privilege and reduces the potential attack surface.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for contracts deriving from ZetaConnectorNonNative echo "Contracts deriving from ZetaConnectorNonNative:" rg --type solidity "contract \w+ is .*ZetaConnectorNonNative" # Search for references to _mintTo outside this contract echo "References to _mintTo outside ZetaConnectorNonNative:" rg --type solidity "_mintTo" -g "!ZetaConnectorNonNative.sol"Length of output: 413
Script:
#!/bin/bash # Search for contracts deriving from ZetaConnectorNonNative echo "Contracts deriving from ZetaConnectorNonNative:" rg "contract \w+ is .*ZetaConnectorNonNative" -g '*.sol' # Search for references to _mintTo outside ZetaConnectorNonNative.sol echo "References to _mintTo outside ZetaConnectorNonNative.sol:" rg "_mintTo" -g '*.sol' -g '!v2/contracts/evm/ZetaConnectorNonNative.sol'Length of output: 861
v2/test/ERC20Custody.t.sol (1)
Line range hint
1-669: Overall, the test file looks good with the updated event emission.The change to include both old and new TSS addresses in the
UpdatedCustodyTSSAddressevent improves the test coverage and aligns with likely changes in the main contract. The rest of the test cases remain comprehensive, covering various scenarios for the ERC20Custody contract functionality.v2/test/GatewayZEVM.t.sol (6)
137-141: LGTM: New test for message size limit in ZRC20 withdrawal.This new test function effectively checks the failure case when the revert message size exceeds the maximum allowed size during ZRC20 withdrawal. It's a valuable addition to ensure proper error handling for oversized messages.
176-177: LGTM: Improved test for message size limit in ZRC20 withdrawal and call.The changes enhance the test by setting both the message and revert message sizes. This ensures a more comprehensive check of the size limit enforcement.
Regarding the previous suggestion to refactor repetitive setup code:
While the current implementation is acceptable, consider creating a helper function to generate oversized messages if this pattern is repeated in multiple tests. This could improve maintainability and readability.
245-246: LGTM: Consistent improvement in message size testing for ZRC20 withdrawal and call with options.These changes align with the previous modifications, ensuring consistent and comprehensive testing of message size limits across different scenarios. The use of
MAX_MESSAGE_SIZEfor both message and revert message sizes is a good practice.
312-316: LGTM: New test for message size limit in ZETA withdrawal.This new test function effectively checks the failure case when the revert message size exceeds the maximum allowed size during ZETA withdrawal. It's a valuable addition to ensure consistent error handling across different token types.
330-331: LGTM: Consistent improvement in message size testing for ZETA withdrawal and call.These changes align with the previous modifications for ZRC20, ensuring consistent and comprehensive testing of message size limits for ZETA operations. The use of
MAX_MESSAGE_SIZEfor both message and revert message sizes maintains good testing practices across different token types.
567-568: LGTM: Consistent improvement in message size testing for call operations.These changes in both
testCallFailsIfMessageSizeExceededandtestCallWithCallOptsFailsIfMessageSizeExceededalign with the previous modifications for withdrawal operations. They ensure consistent and comprehensive testing of message size limits across different operation types (withdrawal and call). The continued use ofMAX_MESSAGE_SIZEfor both message and revert message sizes maintains good testing practices throughout the test suite.Also applies to: 594-595
v2/contracts/evm/GatewayEVM.sol (5)
85-87: Emission of event before state update is appropriateEmitting the
UpdatedGatewayTSSAddressevent before updating thetssAddressstate variable ensures the event captures the transition from the old address to the new one accurately.
194-200: Proper resetting of token approvals enhances securityThe use of
_resetApprovalboth before and after the external call ensures that token approvals are correctly managed, minimizing the risk of unauthorized token transfers.
205-205: Safe transfer of remaining token balanceTransferring any remaining token balance back to the asset handler prevents tokens from being left in the contract, which is a good practice for security and fund management.
Line range hint
381-455: Consistent naming for private functions improves readabilityRenaming internal functions with a leading underscore and setting them as
privateclarifies their intended scope, adhering to Solidity best practices and enhancing code clarity.
431-431: Validation before executing arbitrary calls enhances securityCalling
_revertIfOnCallOrOnRevert(data);before executing an arbitrary call effectively prevents calls to sensitive functions likeonCalloronRevert, mitigating potential security risks.
| @@ -95,7 +95,7 @@ contract ERC20CustodyUpgradeTest is | |||
|
|
|||
| tssAddress = newTSSAddress; | |||
|
|
|||
| emit UpdatedCustodyTSSAddress(newTSSAddress); | |||
| emit UpdatedCustodyTSSAddress(tssAddress, newTSSAddress); | |||
There was a problem hiding this comment.
Approve the event update with a minor suggestion.
The updated event emission is an improvement, providing both the old and new TSS addresses. This change enhances the contract's ability to track important state changes.
However, there's a minor logical issue:
Consider updating the event emission to use the old TSS address before updating the tssAddress variable. Here's a suggested modification:
- emit UpdatedCustodyTSSAddress(tssAddress, newTSSAddress);
+ address oldTSSAddress = tssAddress;
+ tssAddress = newTSSAddress;
+ emit UpdatedCustodyTSSAddress(oldTSSAddress, newTSSAddress);This ensures that the event accurately reflects the old and new addresses.
Committable suggestion was skipped due to low confidence.
fixes from review in PRs:
#363 (review)
#361 (review)
#376 (review)
this should be backported to node/v20 branch
Summary by CodeRabbit
New Features
Bug Fixes
depositfunction in theERC20Custodycontract to prevent further use.Documentation
Refactor