feat: ERC20Custody and ZetaConnector uups upgradable #385
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #385 +/- ##
==========================================
- Coverage 84.77% 84.69% -0.09%
==========================================
Files 8 8
Lines 394 405 +11
Branches 134 134
==========================================
+ Hits 334 343 +9
- Misses 60 62 +2 ☔ View full report in Codecov by Sentry. |
skosito
requested review from
andresaiello,
brewmaster012,
lumtis,
CharlieMc0,
fadeev and
fbac
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant changes across multiple contracts to transition them from non-upgradeable to upgradeable architectures using OpenZeppelin's upgradeable contracts. Key modifications include replacing constructors with 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (3)
v2/scripts/deploy/deterministic/DeployERC20Custody.s.sol (1)
51-53: Add a verification step for the admin addressWhile you are verifying that
tssAddressandgatewayare correctly set, it would be beneficial to also confirm that theadminaddress is set as expected. This ensures that all initialization parameters have been properly configured.Apply this addition to include the admin verification:
require(erc20Custody.tssAddress() == tss, "tss not set"); require(address(erc20Custody.gateway()) == gateway, "gateway not set"); +require(erc20Custody.hasRole(DEFAULT_ADMIN_ROLE, admin), "admin not set");v2/test/ZetaConnectorNonNative.t.sol (1)
67-71: Consider using distinct proxy variables for clarityReusing the
proxyvariable for multiple contract deployments may reduce code readability. Consider using separate variables for each proxy deployment to enhance clarity.Apply this diff to implement the suggestion:
- proxy = Upgrades.deployUUPSProxy( + address custodyProxy = Upgrades.deployUUPSProxy( "ERC20Custody.sol", abi.encodeCall(ERC20Custody.initialize, (address(gateway), tssAddress, owner)) ); - custody = ERC20Custody(proxy); + custody = ERC20Custody(custodyProxy); - proxy = Upgrades.deployUUPSProxy( + address connectorProxy = Upgrades.deployUUPSProxy( "ZetaConnectorNonNative.sol", abi.encodeCall(ZetaConnectorNonNative.initialize, (address(gateway), address(zetaToken), tssAddress, owner)) ); - zetaConnector = ZetaConnectorNonNative(proxy); + zetaConnector = ZetaConnectorNonNative(connectorProxy);v2/test/ERC20Custody.t.sol (1)
65-69: Suggestion: Add comments to explain proxy deploymentsAdding brief comments before each proxy deployment can help future readers understand the purpose of each deployment step, improving the maintainability of the test setup.
Apply this diff to include explanatory comments:
+// Deploy and initialize the ERC20Custody contract proxy proxy = Upgrades.deployUUPSProxy( "ERC20Custody.sol", abi.encodeCall(ERC20Custody.initialize, (address(gateway), tssAddress, owner)) ); custody = ERC20Custody(proxy); +// Deploy and initialize the ZetaConnectorNonNative contract proxy proxy = Upgrades.deployUUPSProxy( "ZetaConnectorNonNative.sol", abi.encodeCall(ZetaConnectorNonNative.initialize, (address(gateway), address(zeta), tssAddress, owner)) ); zetaConnector = ZetaConnectorNonNative(proxy);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (16)
- v2/contracts/evm/ERC20Custody.sol (3 hunks)
- v2/contracts/evm/ZetaConnectorBase.sol (4 hunks)
- v2/contracts/evm/ZetaConnectorNative.sol (1 hunks)
- v2/contracts/evm/ZetaConnectorNonNative.sol (1 hunks)
- v2/scripts/deploy/deterministic/DeployERC20Custody.s.sol (1 hunks)
- v2/scripts/deploy/deterministic/DeployGatewayEVM.s.sol (1 hunks)
- v2/scripts/deploy/deterministic/DeployZetaConnectorNonNative.s.sol (2 hunks)
- v2/scripts/deploy/deterministic/UpgradeGatewayEVM.s.sol (1 hunks)
- v2/test/ERC20Custody.t.sol (1 hunks)
- v2/test/GatewayEVM.t.sol (2 hunks)
- v2/test/GatewayEVMUpgrade.t.sol (1 hunks)
- v2/test/GatewayEVMZEVM.t.sol (1 hunks)
- v2/test/ZetaConnectorNative.t.sol (1 hunks)
- v2/test/ZetaConnectorNonNative.t.sol (1 hunks)
- v2/test/fuzz/ERC20CustodyEchidnaTest.sol (1 hunks)
- v2/test/fuzz/GatewayEVMEchidnaTest.sol (1 hunks)
✅ Files skipped from review due to trivial changes (4)
- v2/scripts/deploy/deterministic/DeployGatewayEVM.s.sol
- v2/scripts/deploy/deterministic/UpgradeGatewayEVM.s.sol
- v2/test/fuzz/ERC20CustodyEchidnaTest.sol
- v2/test/fuzz/GatewayEVMEchidnaTest.sol
🧰 Additional context used
🔇 Additional comments (24)
v2/test/GatewayEVMZEVM.t.sol (4)
74-77: LGTM: Proper initialization of upgradeable GatewayEVM proxyThe changes correctly implement the deployment of an upgradeable GatewayEVM proxy using
Upgrades.deployUUPSProxy. The initialization parameters are correctly passed, and thegatewayEVMvariable is properly assigned the proxy address. This approach allows for future upgrades of the contract.
78-81: LGTM: Efficient initialization of upgradeable ERC20Custody proxyThe changes correctly implement the deployment of an upgradeable ERC20Custody proxy using
Upgrades.deployUUPSProxy. The initialization parameters are correctly passed, and thecustodyvariable is properly assigned the proxy address. The reuse of theproxyvariable is an efficient approach.
82-88: LGTM: Well-structured initialization of upgradeable ZetaConnectorNonNative proxyThe changes correctly implement the deployment of an upgradeable ZetaConnectorNonNative proxy using
Upgrades.deployUUPSProxy. The initialization parameters are correctly passed, and thezetaConnectorvariable is properly assigned the proxy address. The multi-line format of the initialization call improves readability without affecting functionality.
74-88: Summary: Successful implementation of upgradeable proxiesThe changes in this file successfully implement upgradeable proxies for GatewayEVM, ERC20Custody, and ZetaConnectorNonNative using
Upgrades.deployUUPSProxy. This aligns well with the PR objective of making the custody and connectors upgradable in anticipation of upcoming changes in version 21. The implementation is consistent across all three contracts, and the code is well-structured for readability and efficiency.v2/scripts/deploy/deterministic/DeployZetaConnectorNonNative.s.sol (3)
5-6: Updated import statements are correctThe import paths for
ZetaConnectorNonNative.solandERC1967Proxyhave been appropriately updated to reflect the new locations and dependencies.
52-55: Confirm proper casting of proxy to implementation contractCasting the proxy address to
ZetaConnectorNonNativemay have implications if the proxy does not expose all implementation functions directly. Ensure that the methods called are accessible and behave as expected when interacting through the proxy.You can verify the functions are callable via the proxy with the following script:
#!/bin/bash # Description: Verify that the functions used are accessible through the proxy grep -E 'function\s+(tssAddress|gateway|zetaToken)\(' contracts/evm/ZetaConnectorNonNative.sol
39-40: Verify the parameter order in theinitializefunctionEnsure that the parameters passed to
ZetaConnectorNonNative.initializematch the expected order in the contract'sinitializefunction to prevent initialization errors.Run the following script to confirm the
initializefunction's signature:v2/contracts/evm/ZetaConnectorNative.sol (1)
14-25: Initialization function is correctly implementedThe
initializefunction appropriately replaces the constructor for an upgradeable contract. It correctly uses theinitializermodifier and callssuper.initializewith the necessary parameters. The use ofoverrideis appropriate since it overrides the parent contract'sinitializefunction.v2/contracts/evm/ZetaConnectorNonNative.sol (2)
17-19: Initialization ofmaxSupplymoved toinitializefunctionThe
maxSupplyvariable is now declared without an initial value and is initialized in theinitializefunction. This change aligns with the upgradeable contract pattern and ensures flexibility in initialization.
20-33: Proper implementation of theinitializefunction for upgradeabilityThe
initializefunction correctly replaces the constructor, includes theinitializermodifier to prevent multiple initializations, and callssuper.initialize. SettingmaxSupplywithin this function ensures that it's set during contract initialization.v2/contracts/evm/ZetaConnectorBase.sol (5)
4-8: Updated imports to use upgradeable contracts.The import statements have been correctly updated to use the upgradeable versions of OpenZeppelin contracts, which is essential for implementing upgradeable contract patterns.
19-26: Inherited from upgradeable base contracts appropriately.The contract now inherits from the upgradeable versions of OpenZeppelin contracts, enabling upgradeability and consistent with the use of proxy patterns.
33-35: Modified state variables for upgradeability compliance.The
gatewayandzetaTokenvariables have been changed fromimmutableto regular state variables. This modification is necessary because immutable variables are set during construction and are not compatible with upgradeable proxy contracts.
46-66: Initialize function correctly replaces constructor.The
initializefunction appropriately replaces the constructor for an upgradeable contract. It includes necessary checks for zero addresses, initializes state variables, and sets up access control roles correctly.
77-79: Implemented secure_authorizeUpgradefunction.The
_authorizeUpgradefunction correctly restricts the upgrade authorization to accounts with theDEFAULT_ADMIN_ROLE, ensuring that only authorized users can upgrade the contract implementation.v2/contracts/evm/ERC20Custody.sol (5)
9-14: Imports updated for upgradeable contractsThe imports have been correctly updated to use OpenZeppelin's upgradeable contracts, which is appropriate for implementing upgradeable functionality.
21-28: Contract inheritance modified for upgradeabilityThe contract now inherits from OpenZeppelin's upgradeable base contracts, enabling upgradeable functionality as intended.
32-32: Changed 'gateway' variable to suit upgradeable contractThe
gatewayvariable has been changed fromimmutableto a regular state variable, which is necessary since immutable variables are not compatible with upgradeable contracts.
48-57: Initializer function implemented correctlyThe
initializefunction replaces the constructor and appropriately initializes the contract's state and roles. All necessary parent initializers are called in the correct order.
69-70: Upgrade authorization method added correctlyThe
_authorizeUpgradefunction is implemented to restrict upgrades to only those with theDEFAULT_ADMIN_ROLE, adhering to the recommended pattern for UUPS upgradeable contracts.v2/test/ZetaConnectorNonNative.t.sol (2)
58-60: LGTM: GatewayEVM UUPS proxy deployment is correctly implementedThe
GatewayEVMcontract is correctly deployed as a UUPS proxy and initialized with the appropriate parameters.
63-66: LGTM: ERC20Custody UUPS proxy deployment is correctly implementedThe
ERC20Custodycontract is properly deployed as a UUPS proxy and initialized with the correct parameters.v2/test/ERC20Custody.t.sol (1)
57-69: Verify the initialization parameters for the proxiesEnsure that the initialization parameters provided for each proxy deployment are correct and correspond to the expected constructor arguments of the respective contracts. Incorrect parameters could lead to unexpected behavior during tests.
Run the following script to confirm the constructor signatures and the initialization parameters:
v2/test/ZetaConnectorNative.t.sol (1)
58-70: Good practice: Limiting the scope of theproxyvariableReducing the scope of the
proxyvariable from a state variable to a local variable within thesetUpfunction enhances encapsulation and prevents unintended side effects. This change improves code organization and maintainability.
| bytes32 implSalt = keccak256("ERC20Custody"); | ||
| bytes32 proxySalt = keccak256("ERC20CustodyProxy"); | ||
|
|
||
| vm.startBroadcast(); | ||
|
|
||
| ERC20Custody custody = new ERC20Custody{salt: salt}(gateway, tss, admin); | ||
| require(address(custody) != address(0), "deployment failed"); | ||
| expectedImplAddress = vm.computeCreate2Address( | ||
| implSalt, | ||
| hashInitCode(type(ERC20Custody).creationCode) | ||
| ); | ||
|
|
||
| ERC20Custody erc20CustodyImpl = new ERC20Custody{salt: implSalt}(); | ||
| require(address(erc20CustodyImpl) != address(0), "erc20CustodyImpl deployment failed"); | ||
|
|
||
| require(expectedImplAddress == address(erc20CustodyImpl), "impl address doesn't match expected address"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repetitive deployment logic into helper functions
The code for computing expected addresses and deploying both the implementation and proxy contracts is repetitive. To enhance maintainability and reduce duplication, consider abstracting this logic into reusable helper functions.
For example, you could create helper functions like computeExpectedAddress and deployContract:
function computeExpectedAddress(bytes32 salt, bytes memory bytecode) internal view returns (address) {
return vm.computeCreate2Address(salt, hashInitCode(bytecode));
}
function deployContract(bytes32 salt, bytes memory bytecode) internal returns (address deployedAddress) {
assembly {
deployedAddress := create2(0, add(bytecode, 0x20), mload(bytecode), salt)
}
require(deployedAddress != address(0), "Deployment failed");
}Refactored implementation deployment:
-expectedImplAddress = vm.computeCreate2Address(
- implSalt,
- hashInitCode(type(ERC20Custody).creationCode)
-);
-
-ERC20Custody erc20CustodyImpl = new ERC20Custody{salt: implSalt}();
-require(address(erc20CustodyImpl) != address(0), "erc20CustodyImpl deployment failed");
-
-require(expectedImplAddress == address(erc20CustodyImpl), "impl address doesn't match expected address");
+bytes memory implBytecode = type(ERC20Custody).creationCode;
+expectedImplAddress = computeExpectedAddress(implSalt, implBytecode);
+address implAddress = deployContract(implSalt, implBytecode);
+require(expectedImplAddress == implAddress, "Implementation address mismatch");
+ERC20Custody erc20CustodyImpl = ERC20Custody(implAddress);Refactored proxy deployment:
-expectedProxyAddress = vm.computeCreate2Address(
- proxySalt,
- hashInitCode(
- type(ERC1967Proxy).creationCode,
- abi.encode(
- address(erc20CustodyImpl),
- abi.encodeWithSelector(ERC20Custody.initialize.selector, gateway, tss, admin)
- )
- )
-);
-
-ERC1967Proxy erc20CustodyProxy = new ERC1967Proxy{salt: proxySalt}(
- address(erc20CustodyImpl),
- abi.encodeWithSelector(ERC20Custody.initialize.selector, gateway, tss, admin)
-);
-require(address(erc20CustodyProxy) != address(0), "erc20CustodyProxy deployment failed");
-
-require(expectedProxyAddress == address(erc20CustodyProxy), "Proxy address mismatch");
+bytes memory proxyInitCode = abi.encodePacked(
+ type(ERC1967Proxy).creationCode,
+ abi.encode(
+ address(erc20CustodyImpl),
+ abi.encodeWithSelector(ERC20Custody.initialize.selector, gateway, tss, admin)
+ )
+);
+expectedProxyAddress = computeExpectedAddress(proxySalt, proxyInitCode);
+address proxyAddress = deployContract(proxySalt, proxyInitCode);
+require(expectedProxyAddress == proxyAddress, "Proxy address mismatch");
+ERC1967Proxy erc20CustodyProxy = ERC1967Proxy(payable(proxyAddress));Also applies to: 32-49
| bytes32 implSalt = keccak256("ZetaConnectorNonNative"); | ||
| bytes32 proxySalt = keccak256("ZetaConnectorNonNativeProxy"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define salt strings as constants for clarity
Consider defining the salt strings used in keccak256 as constants to enhance readability and maintainability.
Apply this diff to define the salt strings as constants:
+ string constant IMPL_SALT_STRING = "ZetaConnectorNonNative";
+ string constant PROXY_SALT_STRING = "ZetaConnectorNonNativeProxy";
- bytes32 implSalt = keccak256("ZetaConnectorNonNative");
- bytes32 proxySalt = keccak256("ZetaConnectorNonNativeProxy");
+ bytes32 implSalt = keccak256(abi.encodePacked(IMPL_SALT_STRING));
+ bytes32 proxySalt = keccak256(abi.encodePacked(PROXY_SALT_STRING));📝 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.
| bytes32 implSalt = keccak256("ZetaConnectorNonNative"); | |
| bytes32 proxySalt = keccak256("ZetaConnectorNonNativeProxy"); | |
| string constant IMPL_SALT_STRING = "ZetaConnectorNonNative"; | |
| string constant PROXY_SALT_STRING = "ZetaConnectorNonNativeProxy"; | |
| bytes32 implSalt = keccak256(abi.encodePacked(IMPL_SALT_STRING)); | |
| bytes32 proxySalt = keccak256(abi.encodePacked(PROXY_SALT_STRING)); |
v2/test/GatewayEVMUpgrade.t.sol
Outdated
| address erc20CustodyProxy = Upgrades.deployUUPSProxy( | ||
| "ERC20Custody.sol", abi.encodeCall(ERC20Custody.initialize, (address(gateway), tssAddress, owner)) | ||
| ); | ||
| custody = ERC20Custody(erc20CustodyProxy); | ||
| address connectorProxy = Upgrades.deployUUPSProxy( | ||
| "ZetaConnectorNonNative.sol", | ||
| abi.encodeCall(ZetaConnectorNonNative.initialize, (address(gateway), address(zeta), tssAddress, owner)) | ||
| ); | ||
| zetaConnector = ZetaConnectorNonNative(connectorProxy); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor proxy deployment into a helper function to reduce code duplication
The code for deploying UUPS proxies for ERC20Custody and ZetaConnectorNonNative contracts is repetitive. Refactoring this into a reusable helper function will enhance maintainability and make the setUp function cleaner.
Consider adding a private function to handle proxy deployment:
function deployProxy(string memory contractName, bytes memory initData) private returns (address) {
return Upgrades.deployUUPSProxy(contractName, initData);
}Then update the proxy deployments as follows:
-address erc20CustodyProxy = Upgrades.deployUUPSProxy(
- "ERC20Custody.sol", abi.encodeCall(ERC20Custody.initialize, (address(gateway), tssAddress, owner))
-);
+address erc20CustodyProxy = deployProxy(
+ "ERC20Custody.sol",
+ abi.encodeCall(ERC20Custody.initialize, (address(gateway), tssAddress, owner))
+);
custody = ERC20Custody(erc20CustodyProxy);
-address connectorProxy = Upgrades.deployUUPSProxy(
- "ZetaConnectorNonNative.sol",
- abi.encodeCall(ZetaConnectorNonNative.initialize, (address(gateway), address(zeta), tssAddress, owner))
-);
+address connectorProxy = deployProxy(
+ "ZetaConnectorNonNative.sol",
+ abi.encodeCall(ZetaConnectorNonNative.initialize, (address(gateway), address(zeta), tssAddress, owner))
+);
zetaConnector = ZetaConnectorNonNative(connectorProxy);| proxy = Upgrades.deployUUPSProxy( | ||
| "ERC20Custody.sol", abi.encodeCall(ERC20Custody.initialize, (address(gateway), tssAddress, owner)) | ||
| ); | ||
| custody = ERC20Custody(proxy); | ||
| proxy = Upgrades.deployUUPSProxy( | ||
| "ZetaConnectorNonNative.sol", | ||
| abi.encodeCall(ZetaConnectorNonNative.initialize, (address(gateway), address(zeta), tssAddress, owner)) | ||
| ); | ||
| zetaConnector = ZetaConnectorNonNative(proxy); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using distinct variables for each proxy instance to improve readability
The proxy variable is reused for deploying multiple proxies (GatewayEVM, ERC20Custody, and ZetaConnectorNonNative), which could lead to confusion and reduce code clarity. Using distinct variable names like gatewayProxy, custodyProxy, and connectorProxy can enhance the readability and maintainability of the test setup code.
|
|
||
| proxy = Upgrades.deployUUPSProxy( | ||
| "ERC20Custody.sol", abi.encodeCall(ERC20Custody.initialize, (address(gateway), tssAddress, owner)) | ||
| ); | ||
| custody = ERC20Custody(proxy); | ||
| proxy = Upgrades.deployUUPSProxy( | ||
| "ZetaConnectorNonNative.sol", | ||
| abi.encodeCall(ZetaConnectorBase.initialize, (address(gateway), address(zeta), tssAddress, owner)) | ||
| ); | ||
| zetaConnector = ZetaConnectorNonNative(proxy); | ||
|
|
There was a problem hiding this comment.
Inconsistent initializer used for ZetaConnectorNonNative
In line 404, the initialize function of ZetaConnectorBase is called instead of ZetaConnectorNonNative.initialize. Ensure that the correct initializer is used to properly initialize ZetaConnectorNonNative. This inconsistency might lead to incorrect initialization and unexpected behavior during testing.
| address proxy = Upgrades.deployUUPSProxy( | ||
| "GatewayEVM.sol", abi.encodeCall(GatewayEVM.initialize, (tssAddress, address(zetaToken), owner)) | ||
| ); | ||
| gateway = GatewayEVM(proxy); | ||
| custody = new ERC20Custody(address(gateway), tssAddress, owner); | ||
| zetaConnector = new ZetaConnectorNative(address(gateway), address(zetaToken), tssAddress, owner); | ||
| proxy = Upgrades.deployUUPSProxy( | ||
| "ERC20Custody.sol", abi.encodeCall(ERC20Custody.initialize, (address(gateway), tssAddress, owner)) | ||
| ); | ||
| custody = ERC20Custody(proxy); | ||
| proxy = Upgrades.deployUUPSProxy( | ||
| "ZetaConnectorNative.sol", | ||
| abi.encodeCall(ZetaConnectorNative.initialize, (address(gateway), address(zetaToken), tssAddress, owner)) | ||
| ); | ||
| zetaConnector = ZetaConnectorNative(proxy); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using distinct variable names for each proxy to enhance readability
Reusing the proxy variable for deploying multiple contracts can lead to confusion and reduce code clarity. Assigning unique variable names to each proxy improves maintainability and helps prevent potential errors.
Apply this diff to use distinct proxy variables:
-address proxy = Upgrades.deployUUPSProxy(
+address gatewayProxy = Upgrades.deployUUPSProxy(
"GatewayEVM.sol", abi.encodeCall(GatewayEVM.initialize, (tssAddress, address(zetaToken), owner))
);
-gateway = GatewayEVM(proxy);
+gateway = GatewayEVM(gatewayProxy);
-proxy = Upgrades.deployUUPSProxy(
+address custodyProxy = Upgrades.deployUUPSProxy(
"ERC20Custody.sol", abi.encodeCall(ERC20Custody.initialize, (address(gateway), tssAddress, owner))
);
-custody = ERC20Custody(proxy);
+custody = ERC20Custody(custodyProxy);
-proxy = Upgrades.deployUUPSProxy(
+address connectorProxy = Upgrades.deployUUPSProxy(
"ZetaConnectorNative.sol",
abi.encodeCall(ZetaConnectorNative.initialize, (address(gateway), address(zetaToken), tssAddress, owner))
);
-zetaConnector = ZetaConnectorNative(proxy);
+zetaConnector = ZetaConnectorNative(connectorProxy);📝 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.
| address proxy = Upgrades.deployUUPSProxy( | |
| "GatewayEVM.sol", abi.encodeCall(GatewayEVM.initialize, (tssAddress, address(zetaToken), owner)) | |
| ); | |
| gateway = GatewayEVM(proxy); | |
| custody = new ERC20Custody(address(gateway), tssAddress, owner); | |
| zetaConnector = new ZetaConnectorNative(address(gateway), address(zetaToken), tssAddress, owner); | |
| proxy = Upgrades.deployUUPSProxy( | |
| "ERC20Custody.sol", abi.encodeCall(ERC20Custody.initialize, (address(gateway), tssAddress, owner)) | |
| ); | |
| custody = ERC20Custody(proxy); | |
| proxy = Upgrades.deployUUPSProxy( | |
| "ZetaConnectorNative.sol", | |
| abi.encodeCall(ZetaConnectorNative.initialize, (address(gateway), address(zetaToken), tssAddress, owner)) | |
| ); | |
| zetaConnector = ZetaConnectorNative(proxy); | |
| address gatewayProxy = Upgrades.deployUUPSProxy( | |
| "GatewayEVM.sol", abi.encodeCall(GatewayEVM.initialize, (tssAddress, address(zetaToken), owner)) | |
| ); | |
| gateway = GatewayEVM(gatewayProxy); | |
| address custodyProxy = Upgrades.deployUUPSProxy( | |
| "ERC20Custody.sol", abi.encodeCall(ERC20Custody.initialize, (address(gateway), tssAddress, owner)) | |
| ); | |
| custody = ERC20Custody(custodyProxy); | |
| address connectorProxy = Upgrades.deployUUPSProxy( | |
| "ZetaConnectorNative.sol", | |
| abi.encodeCall(ZetaConnectorNative.initialize, (address(gateway), address(zetaToken), tssAddress, owner)) | |
| ); | |
| zetaConnector = ZetaConnectorNative(connectorProxy); |
yes good point, will add, useful to see if some change in contract makes it non-upgradable (eg importing some 3rd party contract etc), i will add tests in this PR |
@lumtis added |
There was a problem hiding this comment.
Actionable comments posted: 22
🧹 Outside diff range and nitpick comments (7)
v2/test/ZetaConnectorNonNative.t.sol (3)
48-49: LGTM: New event for upgraded contractThe addition of the
WithdrawnV2event is appropriate for testing the upgraded version of the contract. It maintains consistency with the originalWithdrawnevent.Consider adding a comment explaining the purpose of this new event to improve code readability.
61-74: LGTM: Upgradeable proxy setupThe modifications to use
Upgrades.deployUUPSProxyfor deploying GatewayEVM, ERC20Custody, and ZetaConnectorNonNative are correct and align with the PR objective of making these contracts upgradeable.Consider extracting the initialization parameters for each contract into separate variables to improve readability and maintainability.
Potential Untested Functions Identified
The following public/external functions in
ZetaConnectorNonNative.solare not covered by existing test cases:
receiveTokenssetMaxSupplyRecommendations:
- Add Test Cases: Implement comprehensive tests for
receiveTokensandsetMaxSupplyto ensure their correct behavior and security.- Verify Functionality: Ensure that these functions handle all possible edge cases and adhere to the intended logic.
🔗 Analysis chain
Line range hint
1-383: Overall LGTM with suggestions for improvementThe changes to this test file successfully implement upgradeable contract testing for ZetaConnectorNonNative. The modifications align well with the PR objectives of making the custody and connectors upgradable.
Consider the following improvements to enhance the test coverage and maintainability:
- Add more test cases to cover edge cases in the upgrade process (e.g., failed upgrades, unauthorized upgrades).
- Create helper functions for common setup steps to reduce code duplication.
- Add tests to verify that the storage layout is preserved after upgrades.
- Include tests for the
upgradeToandupgradeToAndCallfunctions of the UUPS upgradeable pattern.To ensure comprehensive test coverage, let's check for any untested public or external functions in the ZetaConnectorNonNative contract:
This script will help identify any functions in ZetaConnectorNonNative that might not have corresponding test cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find potentially untested functions in ZetaConnectorNonNative # Test: Extract public and external functions from ZetaConnectorNonNative ast-grep --lang solidity --pattern $'contract ZetaConnectorNonNative { $$$ function $FUNC_NAME($_) $VISIBILITY $$$ $$$ }' | grep -E 'function.*\b(public|external)\b' | awk '{print $2}' | sort > /tmp/all_functions.txt # Test: Extract tested functions from the test file rg --type solidity 'function test' | awk '{print $2}' | sed 's/test//g' | sed 's/()//g' | sort > /tmp/tested_functions.txt # Test: Find untested functions comm -23 /tmp/all_functions.txt /tmp/tested_functions.txtLength of output: 576
Script:
#!/bin/bash # Description: Find potentially untested public and external functions in ZetaConnectorNonNative.sol # Locate the ZetaConnectorNonNative.sol file FILE_PATH=$(fd "ZetaConnectorNonNative\.sol$" .) if [ -z "$FILE_PATH" ]; then echo "ZetaConnectorNonNative.sol not found." exit 1 fi # Extract all public and external function names rg -oP 'function\s+\K\w+(?=\s*\()(?=.*\b(public|external)\b)' "$FILE_PATH" | sort > /tmp/all_functions.txt # Extract tested functions from the test file by removing 'test' prefix and parentheses TEST_FILE="v2/test/ZetaConnectorNonNative.t.sol" if [ ! -f "$TEST_FILE" ]; then echo "Test file v2/test/ZetaConnectorNonNative.t.sol not found." exit 1 fi rg -oP 'function\s+test\K\w+(?=\s*\()' "$TEST_FILE" | sort > /tmp/tested_functions.txt # Find untested functions if command -v comm >/dev/null 2>&1; then comm -23 /tmp/all_functions.txt /tmp/tested_functions.txt else echo "comm command not found. Please ensure it is available in the environment." exit 1 fiLength of output: 960
Script:
#!/bin/bash # Description: Find potentially untested public and external functions in ZetaConnectorNonNative.sol # Locate the ZetaConnectorNonNative.sol file within the contracts directory FILE_PATH=$(fd "ZetaConnectorNonNative\.sol$" ./v2/contracts/evm/) if [ -z "$FILE_PATH" ]; then echo "ZetaConnectorNonNative.sol not found in ./v2/contracts/evm/." exit 1 fi # Ensure only one file is found FILE_COUNT=$(echo "$FILE_PATH" | wc -l) if [ "$FILE_COUNT" -gt 1 ]; then echo "Multiple ZetaConnectorNonNative.sol files found. Please ensure only one exists in ./v2/contracts/evm/." exit 1 fi # Extract all public and external function names rg -oP 'function\s+\K\w+(?=\s*\()(?=.*\b(public|external)\b)' "$FILE_PATH" | sort > /tmp/all_functions.txt # Extract tested functions from the test file by removing 'test' prefix and parentheses TEST_FILE="v2/test/ZetaConnectorNonNative.t.sol" if [ ! -f "$TEST_FILE" ]; then echo "Test file v2/test/ZetaConnectorNonNative.t.sol not found." exit 1 fi rg -oP 'function\s+test\K\w+(?=\s*\()' "$TEST_FILE" | sort > /tmp/tested_functions.txt # Find untested functions using grep grep -Fxv -f /tmp/tested_functions.txt /tmp/all_functions.txtLength of output: 665
v2/test/utils/upgrades/ZetaConnectorNonNativeUpgradeTest.sol (1)
125-127: AddnonReentrantmodifier toreceiveTokensfunction to enhance security.The
receiveTokensfunction interacts with external contracts and alters the state by burning tokens. While there is no apparent reentrancy risk at the moment, adding thenonReentrantmodifier is a good defensive practice to prevent potential vulnerabilities in future code changes.Update the function signature:
function receiveTokens(uint256 amount) external override nonReentrant whenNotPaused { IZetaNonEthNew(zetaToken).burnFrom(msg.sender, amount); }v2/test/utils/upgrades/GatewayZEVMUpgradeTest.sol (1)
70-84: Consider Initializing Parent Contracts in Correct OrderAccording to OpenZeppelin's recommendations, it's best practice to initialize parent contracts in the order they are inherited to avoid any potential issues.
Apply this diff to reorder the initializers:
__UUPSUpgradeable_init(); + __AccessControl_init(); __Pausable_init(); __ReentrancyGuard_init(); + __AccessControl_init();Ensure that
__AccessControl_init()is called before__UUPSUpgradeable_init()if necessary.v2/test/ERC20Custody.t.sol (1)
580-580: Fix typo in comment: 'substracted' should be 'subtracted'There is a typographical error in the comment on line 580. The word "substracted" should be corrected to "subtracted" for proper spelling.
Apply this diff to fix the typo:
-// Verify that the tokens were substracted from custody +// Verify that the tokens were subtracted from custodyv2/test/GatewayEVM.t.sol (1)
43-43: Consider RenamingExecutedV2Event for ClarityThe newly added
ExecutedV2event serves to distinguish post-upgrade executions. To enhance readability, consider renaming it to something more descriptive likeExecutedAfterUpgradeorUpgradedExecutedto clearly convey its purpose.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
- v2/test/ERC20Custody.t.sol (4 hunks)
- v2/test/GatewayEVM.t.sol (5 hunks)
- v2/test/GatewayEVMUpgrade.t.sol (0 hunks)
- v2/test/GatewayZEVM.t.sol (3 hunks)
- v2/test/ZetaConnectorNative.t.sol (4 hunks)
- v2/test/ZetaConnectorNonNative.t.sol (4 hunks)
- v2/test/utils/upgrades/ERC20CustodyUpgradeTest.sol (1 hunks)
- v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol (1 hunks)
- v2/test/utils/upgrades/GatewayZEVMUpgradeTest.sol (1 hunks)
- v2/test/utils/upgrades/ZetaConnectorNativeUpgradeTest.sol (1 hunks)
- v2/test/utils/upgrades/ZetaConnectorNonNativeUpgradeTest.sol (1 hunks)
💤 Files with no reviewable changes (1)
- v2/test/GatewayEVMUpgrade.t.sol
🧰 Additional context used
🔇 Additional comments (23)
v2/test/ZetaConnectorNonNative.t.sol (2)
10-10: LGTM: New import for upgrade testingThe addition of the import statement for
ZetaConnectorNonNativeUpgradeTestis appropriate for testing the upgrade functionality of the ZetaConnectorNonNative contract.
364-383: LGTM: Upgrade and withdrawal testThe new
testUpgradeAndWithdrawfunction effectively tests the upgrade process and verifies that the withdrawal functionality is maintained after the upgrade.Consider the following improvements:
- Add assertions to verify the upgrade was successful (e.g., check the implementation address).
- Test more functionalities of the upgraded contract to ensure complete compatibility.
- Add comments explaining the purpose of each step in the test for better readability.
To ensure the upgrade process is correctly implemented, let's verify the upgrade-related functions in the ZetaConnectorNonNative contract:
v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol (5)
4-7: LGTM: Import paths updated consistentlyThe import paths have been updated from "../../contracts/" to "../../../contracts/" consistently across all modified import statements. This change reflects a restructuring of the project directory.
Line range hint
1-498: Summary of changes and recommendationsThe main changes in this upgrade test contract include:
- Updated import paths
- Addition of a new
ExecutedV2event- Modifications to the
executefunctionsRecommendations:
- Ensure consistency in event usage across the contract. The
executefunction withMessageContextshould emitExecutedV2instead ofExecuted.- Clearly document the differences between the two
executefunctions and their intended use cases.- Consider adding deprecation notices if any functions or events are planned to be phased out in future upgrades.
- Verify that the changes don't introduce any security vulnerabilities or break existing functionality, especially regarding the handling of
MessageContext.- Update any external systems or off-chain processes that may be affected by these changes.
Please address these points to ensure a smooth and consistent upgrade process.
Line range hint
138-159: Function signature updated, but event usage is inconsistentThe
executefunction signature has been updated to include aMessageContextparameter, which enhances the message handling capability of the contract. However, the function still emits the oldExecutedevent instead of the newExecutedV2event. This is inconsistent with the event versioning approach introduced earlier.Consider updating the event emission to use
ExecutedV2for consistency:- emit Executed(destination, msg.value, data); + emit ExecutedV2(destination, msg.value, data);Also, ensure that any external systems or off-chain processes that depend on this function are updated to handle the new
MessageContextparameter.To ensure that the
MessageContextis properly used within the function, please run the following verification:#!/bin/bash # Check for proper usage of MessageContext in execute function rg -U "function execute.*MessageContext.*\n.*\n.*\n.*\n.*messageContext" v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol
Line range hint
36-37: New eventExecutedV2added for versioningA new event
ExecutedV2has been added, which is similar to the existingExecutedevent. This approach allows for versioning of events, which can be beneficial for tracking changes in event structure across contract upgrades. However, ensure that any external systems or off-chain processes that depend on these events are updated accordingly.To ensure that the new event is properly implemented and doesn't conflict with existing functionality, please run the following verification:
✅ Verification successful
ExecutedV2 Event Properly Implemented
The
ExecutedV2event is correctly declared and emitted inv2/test/utils/upgrades/GatewayEVMUpgradeTest.sol. This implementation aligns with the versioning approach for events, ensuring consistent tracking across contract upgrades.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for proper implementation of ExecutedV2 event rg -U "event Executed.*\n.*\n.*emit Executed" v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol rg -U "event ExecutedV2.*\n.*\n.*emit ExecutedV2" v2/test/utils/upgrades/GatewayEVMUpgradeTest.solLength of output: 217
Script:
#!/bin/bash # Check for the declaration of the ExecutedV2 event rg -U 'event ExecutedV2\s*\(.*\);' v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol # Check for the emission of the ExecutedV2 event rg -U 'emit ExecutedV2\s*\(.*\);' v2/test/utils/upgrades/GatewayEVMUpgradeTest.solLength of output: 303
Line range hint
161-182: New overloadedexecutefunction addedA new overloaded version of the
executefunction has been added without theMessageContextparameter. This provides backward compatibility for calls that don't include theMessageContext. The function correctly emits the newExecutedV2event.While this approach maintains compatibility, it's important to consider the following:
- Ensure that the documentation clearly explains the difference between the two
executefunctions and when to use each.- Consider adding a deprecation notice to this overloaded function if the intention is to phase it out in future upgrades.
- Verify that the absence of
MessageContextdoesn't lead to any security or functionality issues in this version.To ensure that both
executefunctions are properly implemented and don't conflict, please run the following verification:✅ Verification successful
To ensure the
executefunctions are correctly implemented and emit the appropriate events, please run the following verification scripts:
To further verify the implementation of the
executefunctions and their corresponding emit statements, please run the following verification scripts:
To accurately associate each
emitstatement with its correspondingexecutefunction, please run the following verification scripts:
Verification Successful
Both
executefunctions inv2/test/utils/upgrades/GatewayEVMUpgradeTest.solare correctly implemented and emit their respective events (ExecutedandExecutedV2). No issues were found with the current implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for proper implementation of both execute functions rg -U "function execute.*\n.*\n.*\n.*\n.*emit Executed" v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol rg -U "function execute.*\n.*\n.*\n.*\n.*emit ExecutedV2" v2/test/utils/upgrades/GatewayEVMUpgradeTest.solLength of output: 215
Script:
#!/bin/bash # Verify execute function emits Executed event rg -U "function execute.*\n.*\nemit Executed" v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol # Verify execute function emits ExecutedV2 event rg -U "function execute.*\n.*\nemit ExecutedV2" v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol # Alternatively, use ast-grep for more precise AST-based matching ast-grep --lang solidity --pattern ' FunctionDeclaration name: "execute" body: EmitEvent[name: "Executed"] ' v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol ast-grep --lang solidity --pattern ' FunctionDeclaration name: "execute" body: EmitEvent[name: "ExecutedV2"] ' v2/test/utils/upgrades/GatewayEVMUpgradeTest.solLength of output: 817
Script:
#!/bin/bash # Search for emit Executed event in GatewayEVMUpgradeTest.sol rg "emit\s+Executed" v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol # Search for emit ExecutedV2 event in GatewayEVMUpgradeTest.sol rg "emit\s+ExecutedV2" v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol # Additionally, list all execute functions to verify their implementations rg "function\s+execute\s*\(" v2/test/utils/upgrades/GatewayEVMUpgradeTest.solLength of output: 489
Script:
#!/bin/bash # Extract context around 'emit Executed' rg -A5 "emit\s+Executed" v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol # Extract context around 'emit ExecutedV2' rg -A5 "emit\s+ExecutedV2" v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol # Extract context around 'emit ExecutedWithERC20' rg -A5 "emit\s+ExecutedWithERC20" v2/test/utils/upgrades/GatewayEVMUpgradeTest.solLength of output: 1263
v2/test/utils/upgrades/ZetaConnectorNonNativeUpgradeTest.sol (4)
24-37: Ensure proper usage ofoverridekeyword ininitializefunction.The
initializefunction is marked with theoverridekeyword, but it's not clear if the parent contractZetaConnectorBasedeclares aninitializefunction that isvirtual. If not, usingoverridemay cause a compilation error.Please confirm that
super.initializeis intended and that the parent contract'sinitializefunction is appropriately declared for overriding.
47-51:⚠️ Potential issueEnsure consistency between function access control and Natspec comments in
withdraw.The Natspec comment states that this function can only be called by the TSS address, but the function uses
onlyRole(WITHDRAWER_ROLE)as an access control modifier. This could lead to confusion ifWITHDRAWER_ROLEis not exclusively assigned to the TSS address.Please verify if
WITHDRAWER_ROLEis intended to represent the TSS address. If so, consider updating the Natspec comment to reflect the role:/// @dev This function can only be called by accounts with the WITHDRAWER_ROLE.Alternatively, if the function should be restricted to the TSS address, update the access control modifier accordingly.
67-73:⚠️ Potential issueEnsure consistency between function access control and Natspec comments in
withdrawAndCall.Similar to the previous comment, the Natspec states the function can only be called by the TSS address, but it uses
onlyRole(WITHDRAWER_ROLE). Confirm that the access control aligns with the intended design.Update the Natspec comment or the access control modifier for clarity and accuracy.
94-101:⚠️ Potential issueEnsure consistency between function access control and Natspec comments in
withdrawAndRevert.Again, there is a mismatch between the Natspec comment and the access control modifier. The function uses
onlyRole(WITHDRAWER_ROLE)while the comment mentions it can only be called by the TSS address.Please reconcile the discrepancy by updating the comment or the modifier to accurately reflect who can call the function.
v2/test/utils/upgrades/ERC20CustodyUpgradeTest.sol (1)
87-99: Verify Role Updates inupdateTSSAddressFunctionThe
updateTSSAddressfunction correctly revokes and grants roles when updating thetssAddress. Ensure that there are no missed permissions and that the order of operations maintains contract security.Run the following script to verify that all roles associated with the old
tssAddressare properly revoked and that the newtssAddresshas been granted the necessary roles:✅ Verification successful
Role Updates Verified Successfully
The
updateTSSAddressfunction inv2/test/utils/upgrades/ERC20CustodyUpgradeTest.solcorrectly revokesWITHDRAWER_ROLEandWHITELISTER_ROLEfrom the oldtssAddressand grants them to thenewTSSAddress. The order of operations ensures contract security by revoking roles before granting them to the new address.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check role assignments for tssAddress updates # Expected: No remaining roles assigned to the old tssAddress and correct roles for newTSSAddress # Find all role assignments to the old tssAddress rg --type solidity "grantRole|revokeRole" -A 2 # Verify role constants are correct rg --type solidity "bytes32 public constant" -A 1Length of output: 166
Script:
#!/bin/bash # Description: Check role assignments for tssAddress updates using file extensions # Expected: No remaining roles assigned to the old tssAddress and correct roles for newTSSAddress # Find all role assignments to the old tssAddress rg "grantRole|revokeRole" --glob "*.sol" -A 2 # Verify role constants are correct rg "bytes32 public constant" --glob "*.sol" -A 1Length of output: 11100
Script:
#!/bin/bash # Description: Verify the updateTSSAddress function in ERC20CustodyUpgradeTest.sol # Extract the updateTSSAddress function rg "function updateTSSAddress" --glob "*.sol" -A 15 ERC20CustodyUpgradeTest.sol # Check for revocation of WITHDRAWER_ROLE and WHITELISTER_ROLE from the old tssAddress rg "_revokeRole(WITHDRAWER_ROLE, tssAddress)" --glob "*.sol" ERC20CustodyUpgradeTest.sol rg "_revokeRole(WHITELISTER_ROLE, tssAddress)" --glob "*.sol" ERC20CustodyUpgradeTest.sol # Check for granting of WITHDRAWER_ROLE and WHITELISTER_ROLE to the newTSSAddress rg "_grantRole(WITHDRAWER_ROLE, newTSSAddress)" --glob "*.sol" ERC20CustodyUpgradeTest.sol rg "_grantRole(WHITELISTER_ROLE, newTSSAddress)" --glob "*.sol" ERC20CustodyUpgradeTest.sol # Verify that tssAddress is updated after revoking and granting roles rg "tssAddress = newTSSAddress" --glob "*.sol" ERC20CustodyUpgradeTest.solLength of output: 936
Script:
#!/bin/bash # Description: Verify the updateTSSAddress function in v2/test/utils/upgrades/ERC20CustodyUpgradeTest.sol # Define the correct file path FILE_PATH="v2/test/utils/upgrades/ERC20CustodyUpgradeTest.sol" # Check if the file exists if [ ! -f "$FILE_PATH" ]; then echo "Error: File $FILE_PATH does not exist." exit 1 fi # Extract the updateTSSAddress function with surrounding lines for context rg "function updateTSSAddress" --glob "*.sol" -A 15 "$FILE_PATH" # Check for revocation of WITHDRAWER_ROLE and WHITELISTER_ROLE from the old tssAddress rg "_revokeRole(WITHDRAWER_ROLE, tssAddress);" "$FILE_PATH" rg "_revokeRole(WHITELISTER_ROLE, tssAddress);" "$FILE_PATH" # Check for granting of WITHDRAWER_ROLE and WHITELISTER_ROLE to the newTSSAddress rg "_grantRole(WITHDRAWER_ROLE, newTSSAddress);" "$FILE_PATH" rg "_grantRole(WHITELISTER_ROLE, newTSSAddress);" "$FILE_PATH" # Verify that tssAddress is updated after revoking and granting roles rg "tssAddress = newTSSAddress;" "$FILE_PATH"Length of output: 1353
v2/test/ZetaConnectorNative.t.sol (3)
10-10: Import statement added correctlyThe import of
ZetaConnectorNativeUpgradeTest.solis appropriately included and ensures access to the upgraded contract for testing purposes.
47-47: EventWithdrawnV2declared appropriatelyThe new event
WithdrawnV2is correctly declared with the intended parameters and will facilitate tracking withdrawals post-upgrade.
61-73: Deployment and initialization of proxies are correctly implementedThe UUPS proxies for
GatewayEVM,ERC20Custody, andZetaConnectorNativeare properly deployed and initialized with the correct parameters. This ensures that each contract is upgradeable and maintains the expected functionality.v2/test/utils/upgrades/GatewayZEVMUpgradeTest.sol (4)
90-93: Ensure All Custom Errors Are DeclaredThe
OnlyWZETAOrProtocolerror used in thereceivefunction is not declared. This will cause a compilation error.
58-63: Ensure the 'CallerIsNotProtocol' Error Is DeclaredThe
CallerIsNotProtocolerror used in theonlyProtocolmodifier is not declared.
134-142: Confirm Correct Implementation of WETH9 InterfaceEnsure that the
IWETH9interface correctly matches the WETH9 contract, especially thewithdrawandtransferFromfunctions, to avoid unexpected behaviors.Run the following script to check the
IWETH9interface:✅ Verification successful
IWETH9 Interface Implementation Confirmed
The
IWETH9interface correctly includes bothtransferFromandwithdrawfunctions as per the standard WETH9 interface.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that IWETH9 interface matches the standard WETH9 interface. # Expectation: The IWETH9 interface should have `transferFrom` and `withdraw` functions. ast-grep --lang solidity --pattern $'interface IWETH9 { function transferFrom($_) external returns ($_); function withdraw($_) external; }'Length of output: 265
Script:
#!/bin/bash # Description: Verify that IWETH9 interface includes `transferFrom` and `withdraw` functions using ripgrep. # Search for the IWETH9 interface rg 'interface\s+IWETH9' -A 10 # Check for the transferFrom function within the interface rg 'interface\s+IWETH9' -A 10 | rg 'function\s+transferFrom\s*\(.*\)\s+external\s+returns' # Check for the withdraw function within the interface rg 'interface\s+IWETH9' -A 10 | rg 'function\s+withdraw\s*\(.*\)\s+external'Length of output: 5682
70-84: Validate Admin Role AssignmentWhen assigning roles during initialization, ensure that the admin role is correctly set and that role hierarchy is appropriately defined.
Run the following script to verify role assignments:
✅ Verification successful
Admin Role Assignment Verified
The admin address has been correctly assigned both
DEFAULT_ADMIN_ROLEandPAUSER_ROLEas expected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that DEFAULT_ADMIN_ROLE and PAUSER_ROLE are correctly assigned. # Expectation: The admin address should have both DEFAULT_ADMIN_ROLE and PAUSER_ROLE assigned. ast-grep --lang solidity --pattern $'function initialize($_) public initializer { $$$ _grantRole(DEFAULT_ADMIN_ROLE, $_); _grantRole(PAUSER_ROLE, $_); $$$ }'Length of output: 291
Script:
#!/bin/bash # Description: Verify that DEFAULT_ADMIN_ROLE and PAUSER_ROLE are correctly assigned using ripgrep. # Expectation: The admin address should have both DEFAULT_ADMIN_ROLE and PAUSER_ROLE assigned within the initialize function. rg -A3 -B1 "_grantRole\(DEFAULT_ADMIN_ROLE," v2/test/utils/upgrades/GatewayZEVMUpgradeTest.sol rg -A3 -B1 "_grantRole\(PAUSER_ROLE," v2/test/utils/upgrades/GatewayZEVMUpgradeTest.solLength of output: 442
v2/test/GatewayEVM.t.sol (1)
10-10: Import Statement Added for Upgrade TestingThe addition of
import "./utils/upgrades/GatewayEVMUpgradeTest.sol";correctly includes the necessary contract for upgrade testing.v2/test/GatewayZEVM.t.sol (3)
12-12: Added import forGatewayZEVMUpgradeTestThe import of
GatewayZEVMUpgradeTest.solis necessary for the upgrade tests introduced in this file.
603-628:⚠️ Potential issueEnsure proper initialization during proxy upgrade
In the
testUpgradeAndWithdrawZRC20function, the proxy is upgraded using:Upgrades.upgradeProxy(proxy, "GatewayZEVMUpgradeTest.sol", "", owner);Passing an empty string
""as the initialization data might result in the upgraded contract not being properly initialized. If the new implementation contract requires initialization, consider encoding the initializer function call and passing it as the third argument.Please verify whether the upgraded contract requires initialization, and update the upgrade call accordingly.
36-48: New eventWithdrawnV2introduced with additional parametersThe
WithdrawnV2event adds new parametersgasfee,protocolFlatFee, andmessage, extending the originalWithdrawnevent. Ensure that all relevant parts of the code emitWithdrawnV2where these additional parameters are needed. Also, update any systems that listen for this event to handle the new parameters.To confirm that
WithdrawnV2is emitted correctly throughout the codebase, you can run the following shell script:
| function withdraw( | ||
| address to, | ||
| uint256 amount, | ||
| bytes32 internalSendHash | ||
| ) | ||
| external | ||
| override | ||
| nonReentrant | ||
| onlyRole(WITHDRAWER_ROLE) | ||
| whenNotPaused | ||
| { |
There was a problem hiding this comment.
Unused parameter internalSendHash in withdraw function
The parameter internalSendHash in the withdraw function is declared but not used within the function body. If it's not needed, consider removing it to simplify the function signature. If it's intended for future use or to maintain interface consistency, adding a comment explaining its purpose would improve code clarity.
| function withdrawAndCall( | ||
| address to, | ||
| uint256 amount, | ||
| bytes calldata data, | ||
| bytes32 internalSendHash | ||
| ) | ||
| external | ||
| override | ||
| nonReentrant | ||
| onlyRole(WITHDRAWER_ROLE) | ||
| whenNotPaused | ||
| { |
There was a problem hiding this comment.
Unused parameter internalSendHash in withdrawAndCall function
The parameter internalSendHash in the withdrawAndCall function is declared but not utilized within the function body. To enhance code readability, either remove the unused parameter or implement it appropriately if it's required for tracking or logging purposes.
| function withdrawAndRevert( | ||
| address to, | ||
| uint256 amount, | ||
| bytes calldata data, | ||
| bytes32 internalSendHash, | ||
| RevertContext calldata revertContext | ||
| ) | ||
| external | ||
| override | ||
| nonReentrant | ||
| onlyRole(WITHDRAWER_ROLE) | ||
| whenNotPaused | ||
| { |
There was a problem hiding this comment.
Unused parameter internalSendHash in withdrawAndRevert function
In the withdrawAndRevert function, the parameter internalSendHash is not used within the function body. Consider removing it if it's unnecessary, or ensure it's used correctly if it's meant for internal tracking.
| // Forward the call to the Gateway contract | ||
| gateway.executeWithERC20(address(zetaToken), to, amount, data); | ||
|
|
||
| emit WithdrawnAndCalled(to, amount, data); |
There was a problem hiding this comment.
Event WithdrawnAndCalled is not declared
The function withdrawAndCall emits an event WithdrawnAndCalled, but this event is not declared in the contract. To prevent compilation errors and ensure proper event logging, please declare the event before emitting it.
Consider adding the event declaration:
event WithdrawnAndCalled(address indexed to, uint256 amount, bytes data);| // Forward the call to the Gateway contract | ||
| gateway.revertWithERC20(address(zetaToken), to, amount, data, revertContext); | ||
|
|
||
| emit WithdrawnAndReverted(to, amount, data, revertContext); |
There was a problem hiding this comment.
Event WithdrawnAndReverted is not declared
The function withdrawAndRevert emits an event WithdrawnAndReverted, but this event is not declared in the contract. Ensure that you declare the event to avoid compilation issues and to facilitate accurate event tracking.
Consider adding the event declaration:
event WithdrawnAndReverted(address indexed to, uint256 amount, bytes data, RevertContext revertContext);| emit Called(msg.sender, zrc20, receiver, message, callOptions, revertOptions); | ||
| } |
There was a problem hiding this comment.
Declare the 'Called' Event Before Emitting It
The Called event is emitted in the _call function but is not declared in the contract. This will result in a compilation error.
Declare the Called event with the appropriate parameters:
+ /// @notice Event emitted when a cross-chain call is initiated.
+ event Called(
+ address indexed sender,
+ address zrc20,
+ bytes receiver,
+ bytes message,
+ CallOptions callOptions,
+ RevertOptions revertOptions
+ );Committable suggestion was skipped due to low confidence.
| @@ -40,6 +40,8 @@ contract ERC20CustodyTest is Test, IGatewayEVMErrors, IGatewayEVMEvents, IReceiv | |||
| error AccessControlUnauthorizedAccount(address account, bytes32 neededRole); | |||
| error LegacyMethodsNotSupported(); | |||
|
|
|||
| event WithdrawnV2(address indexed to, address indexed token, uint256 amount); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider renaming the WithdrawnV2 event for clarity
The event name WithdrawnV2 indicates versioning by appending V2. To enhance readability and maintainability, consider renaming the event to better reflect its purpose in the upgraded contract. A more descriptive name can provide clarity on the event's functionality without relying on version numbers.
|
|
||
| function testUpgradeAndWithdraw() public { | ||
| // upgrade | ||
| Upgrades.upgradeProxy(address(custody), "ERC20CustodyUpgradeTest.sol", "", owner); | ||
| ERC20CustodyUpgradeTest custodyV2 = ERC20CustodyUpgradeTest(address(custody)); | ||
| // withdraw | ||
| uint256 amount = 100_000; | ||
| uint256 balanceBefore = token.balanceOf(destination); | ||
| assertEq(balanceBefore, 0); | ||
| uint256 balanceBeforeCustody = token.balanceOf(address(custodyV2)); | ||
|
|
||
| bytes memory transferData = abi.encodeWithSignature("transfer(address,uint256)", address(destination), amount); | ||
| vm.expectCall(address(token), 0, transferData); | ||
| vm.expectEmit(true, true, true, true, address(custodyV2)); | ||
| emit WithdrawnV2(destination, address(token), amount); | ||
| vm.prank(tssAddress); | ||
| custodyV2.withdraw(destination, address(token), amount); | ||
|
|
||
| // Verify that the tokens were transferred to the destination address | ||
| uint256 balanceAfter = token.balanceOf(destination); | ||
| assertEq(balanceAfter, amount); | ||
|
|
||
| // Verify that the tokens were substracted from custody | ||
| uint256 balanceAfterCustody = token.balanceOf(address(custodyV2)); | ||
| assertEq(balanceAfterCustody, balanceBeforeCustody - amount); | ||
|
|
||
| // Verify that gateway doesn't hold any tokens | ||
| uint256 balanceGateway = token.balanceOf(address(gateway)); | ||
| assertEq(balanceGateway, 0); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions to verify successful contract upgrade
In the testUpgradeAndWithdraw function, consider adding assertions to confirm that the contract has been successfully upgraded to ERC20CustodyUpgradeTest. This could include:
- Checking new functions or variables introduced in the upgraded contract.
- Verifying that the contract's storage layout matches expectations after the upgrade.
Adding these assertions ensures that the upgrade process works correctly and the contract behaves as intended post-upgrade.
| function testUpgradeAndForwardCallToReceivePayable() public { | ||
| // upgrade | ||
| Upgrades.upgradeProxy(address(gateway), "GatewayEVMUpgradeTest.sol", "", owner); | ||
| GatewayEVMUpgradeTest gatewayUpgradeTest = GatewayEVMUpgradeTest(address(gateway)); | ||
| // call | ||
| address custodyBeforeUpgrade = gateway.custody(); | ||
| address tssBeforeUpgrade = gateway.tssAddress(); | ||
|
|
||
| string memory str = "Hello, Foundry!"; | ||
| uint256 num = 42; | ||
| bool flag = true; | ||
| uint256 value = 1 ether; | ||
|
|
||
| bytes memory data = abi.encodeWithSignature("receivePayable(string,uint256,bool)", str, num, flag); | ||
| vm.expectCall(address(receiver), value, data); | ||
| vm.expectEmit(true, true, true, true, address(receiver)); | ||
| emit ReceivedPayable(address(gateway), value, str, num, flag); | ||
| vm.expectEmit(true, true, true, true, address(gateway)); | ||
| emit ExecutedV2(address(receiver), value, data); | ||
| vm.prank(tssAddress); | ||
| gatewayUpgradeTest.execute{ value: value }(address(receiver), data); | ||
|
|
||
| assertEq(custodyBeforeUpgrade, gateway.custody()); | ||
| assertEq(tssBeforeUpgrade, gateway.tssAddress()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance State Validation in Upgrade Test
In the testUpgradeAndForwardCallToReceivePayable function, while you verify that custody and tssAddress remain unchanged after the upgrade, consider extending your assertions to include other critical state variables and roles. This ensures comprehensive validation of the contract's state integrity post-upgrade.
| function testUpgradeAndWithdrawZRC20() public { | ||
| // upgrade | ||
| Upgrades.upgradeProxy(proxy, "GatewayZEVMUpgradeTest.sol", "", owner); | ||
| GatewayZEVMUpgradeTest gatewayUpgradeTest = GatewayZEVMUpgradeTest(proxy); | ||
| // withdraw | ||
| uint256 amount = 1; | ||
| uint256 ownerBalanceBefore = zrc20.balanceOf(owner); | ||
|
|
||
| vm.expectEmit(true, true, true, true, address(gatewayUpgradeTest)); | ||
| emit WithdrawnV2( | ||
| owner, | ||
| 0, | ||
| abi.encodePacked(addr1), | ||
| address(zrc20), | ||
| amount, | ||
| 0, | ||
| zrc20.PROTOCOL_FLAT_FEE(), | ||
| "", | ||
| CallOptions({ gasLimit: 0, isArbitraryCall: true }), | ||
| revertOptions | ||
| ); | ||
| gatewayUpgradeTest.withdraw(abi.encodePacked(addr1), amount, address(zrc20), revertOptions); | ||
|
|
||
| uint256 ownerBalanceAfter = zrc20.balanceOf(owner); | ||
| assertEq(ownerBalanceBefore - amount, ownerBalanceAfter); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand test coverage for upgraded contract
While the testUpgradeAndWithdrawZRC20 function tests the withdrawal functionality post-upgrade, consider adding tests for other functionalities of the upgraded contract to ensure comprehensive coverage.
NOTE: will run
yarn generatebefore merge to make review a bit easierSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores