chore: use deployer address in deployment scripts #419
Conversation
lumtis
requested review from
andresaiello,
brewmaster012,
CharlieMc0,
fadeev,
skosito and
fbac
as code owners
📝 WalkthroughWalkthroughThe pull request introduces enhancements to the deployment scripts for the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #419 +/- ##
=======================================
Coverage 84.27% 84.27%
=======================================
Files 8 8
Lines 388 388
Branches 123 123
=======================================
Hits 327 327
Misses 61 61 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
v2/scripts/deploy/deterministic/DeployGatewayZEVM.s.sol (2)
56-62: Consider adding more post-deployment verifications
While the basic configuration check is good, consider adding more verifications such as:
- Proxy implementation address verification
- Initial deployer role verification before transfer
// Verify initial configuration
require(gateway.zetaToken() == zeta, "zeta token not set");
+require(gateway.implementation() == address(gatewayImpl), "implementation not set correctly");
+require(gateway.hasRole(gateway.DEFAULT_ADMIN_ROLE(), msg.sender), "deployer not set as initial admin");
// Transfer admin role from deployer to admin
transferAdmin(gateway, msg.sender, admin);Line range hint 1-80: Consider implementing atomic deployment and role transfer
While the current implementation is solid, consider wrapping the deployment and role transfer in a try-catch block to ensure atomicity. This would prevent scenarios where deployment succeeds but role transfer fails, potentially leaving the contract in an inconsistent state.
This could be implemented by:
- Adding a cleanup function to handle failed deployments
- Using a two-step deployment process where roles are verified before finalizing the deployment
- Implementing a recovery mechanism for failed role transfers
Would you like me to provide a detailed implementation for any of these approaches?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- v2/scripts/deploy/deterministic/DeployERC20Custody.s.sol (1 hunks)
- v2/scripts/deploy/deterministic/DeployGatewayEVM.s.sol (1 hunks)
- v2/scripts/deploy/deterministic/DeployGatewayZEVM.s.sol (1 hunks)
🔇 Additional comments (11)
v2/scripts/deploy/deterministic/DeployGatewayZEVM.s.sol (4)
19-21: LGTM: Good addition of admin address validation
This check aligns with the PR objectives by ensuring deterministic deployments can't proceed with an invalid admin address.
35-54: LGTM: Proxy deployment correctly implements deployer pattern
The proxy deployment correctly uses msg.sender as the initial admin, which aligns with the PR objective of using a deployer address that will later transfer roles to the actual admin.
24-34: LGTM: Robust implementation contract deployment with proper verification
The implementation uses CREATE2 correctly with appropriate checks for deployment success and address verification.
Let's verify the implementation salt usage across other deployment scripts for consistency:
✅ Verification successful
Salt usage is consistent and well-structured across deployment scripts
The verification shows that salt generation follows a consistent pattern across all deterministic deployments:
- Implementation contracts use
keccak256("<ContractName>")format - Proxy contracts use
keccak256("<ContractName>Proxy")format - Version suffixes (-2) are properly added where applicable
Each deployment script correctly uses these salts with CREATE2 and includes proper address verification checks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent salt usage across deployment scripts
rg -A 1 'bytes32.*Salt.*=.*keccak256.*Gateway' --type sol
Length of output: 87
Script:
#!/bin/bash
# Search for salt definitions in deployment scripts
rg -t sol "bytes32.*Salt.*=" v2/scripts/deploy/
Length of output: 77
Script:
#!/bin/bash
# Search for CREATE2 deployments in deployment scripts
rg -t sol "new.*\{salt:" v2/scripts/deploy/
Length of output: 73
Script:
#!/bin/bash
# List all deployment script files for context
fd -e sol . v2/scripts/deploy/
Length of output: 413
Script:
#!/bin/bash
# Search for salt definitions and CREATE2 deployments without file type flag
rg "bytes32.*Salt.*=" v2/scripts/deploy/
rg "new.*\{salt:" v2/scripts/deploy/
Length of output: 2328
Script:
#!/bin/bash
# Get content of the deployment scripts to analyze salt usage
cat v2/scripts/deploy/deterministic/DeployGatewayEVM.s.sol
cat v2/scripts/deploy/deterministic/DeployGatewayZEVM.s.sol
Length of output: 7410
66-80: LGTM: Well-structured role transfer with comprehensive verification
The function implements a secure role transfer pattern with proper verification. The order of operations (grant before renounce) prevents any potential access gaps.
Let's verify consistent role transfer implementation across other deployment scripts:
✅ Verification successful
Role transfer implementation is consistent and secure across deployment scripts
The verification confirms that the transferAdmin function follows the same secure pattern across all deployment scripts:
DeployGatewayZEVM.s.sol: Handles PAUSER_ROLE and DEFAULT_ADMIN_ROLEDeployGatewayEVM.s.sol: Identical implementation for the same rolesDeployERC20Custody.s.sol: Same pattern plus WHITELISTER_ROLE specific to ERC20Custody
All implementations maintain the secure order (grant before renounce) and include comprehensive role verification checks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent role transfer implementation
ast-grep --pattern 'function transferAdmin($_, $_, $_) {
$$$
grantRole($_, $_);
$$$
renounceRole($_, $_);
$$$
}'
Length of output: 124
Script:
#!/bin/bash
# Search for transferAdmin function implementations
rg -U "function transferAdmin.*?\{.*?\}" -g "*.sol" -A 20
Length of output: 59
Script:
#!/bin/bash
# Let's try a simpler search for transferAdmin function
rg "function transferAdmin" --type sol
# Also search for similar role transfer patterns
rg "grantRole.*DEFAULT_ADMIN_ROLE|PAUSER_ROLE" --type sol
Length of output: 156
Script:
#!/bin/bash
# Search for transferAdmin function without file type filter
rg "function transferAdmin"
# Search for role transfer patterns
rg "grantRole.*(DEFAULT_ADMIN_ROLE|PAUSER_ROLE)"
Length of output: 2911
Script:
#!/bin/bash
# Let's examine the implementation of transferAdmin in other deployment scripts
cat v2/scripts/deploy/deterministic/DeployERC20Custody.s.sol v2/scripts/deploy/deterministic/DeployGatewayEVM.s.sol
Length of output: 8138
v2/scripts/deploy/deterministic/DeployGatewayEVM.s.sol (3)
21-23: LGTM: Good addition of admin address validation!
This check aligns well with the PR objectives and prevents accidental deployments with an unset admin address.
26-34: LGTM: Robust implementation address computation and verification!
The code correctly implements deterministic deployment using CREATE2 with proper verification of both successful deployment and address matching.
37-54: 🛠️ Refactor suggestion
Verify proxy deployment error handling
While the deployment logic is correct, consider enhancing the error handling for proxy deployment failure. The current check only verifies the address but doesn't explicitly verify successful initialization.
Consider adding a try-catch block around the proxy deployment to handle potential initialization failures:
- ERC1967Proxy gatewayProxy = new ERC1967Proxy{salt: proxySalt}(
- address(gatewayImpl),
- abi.encodeWithSelector(GatewayEVM.initialize.selector, tss, address(zeta), msg.sender)
- );
+ ERC1967Proxy gatewayProxy;
+ try new ERC1967Proxy{salt: proxySalt}(
+ address(gatewayImpl),
+ abi.encodeWithSelector(GatewayEVM.initialize.selector, tss, address(zeta), msg.sender)
+ ) returns (ERC1967Proxy proxy) {
+ gatewayProxy = proxy;
+ } catch Error(string memory reason) {
+ revert(string.concat("Proxy initialization failed: ", reason));
+ }v2/scripts/deploy/deterministic/DeployERC20Custody.s.sol (4)
20-22: Good practice: Ensure admin address is set
The added check require(admin != address(0)) ensures that the contract is not deployed with an unset admin address, preventing potential misconfigurations.
43-43: Appropriate use of msg.sender as initial admin
Using msg.sender as the initial admin in the initialize call means the deployer will hold the admin roles initially, aligning with the deployment strategy to transfer roles after deployment.
Also applies to: 51-51
58-60: Verification of initial configuration is added
The added checks confirm that tss and gateway are correctly set after deployment, ensuring the contract is initialized with the correct parameters.
62-64: Transferring admin roles from deployer to admin
Calling transferAdmin(erc20Custody, msg.sender, admin) properly transfers the admin roles from the deployer to the specified admin address, following the intended deployment process.
|
@lumtis will this be backported to v20? |
Yes, I originally made the branch there but there is no conflict between both version so changes are the same |
Closes: #411
Use deployer address for all contract with a deterministic address
Summary by CodeRabbit
New Features
transferAdminfunction to manage admin role transitions securely.Bug Fixes