fix: add pauser role to tss #386
Conversation
skosito
requested review from
andresaiello,
brewmaster012,
lumtis,
CharlieMc0,
fadeev and
fbac
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant modifications across several contracts, focusing on role management and the handling of the TSS (Threshold Signature Scheme) address. Key updates include granting 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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #386 +/- ##
==========================================
+ Coverage 84.69% 84.80% +0.11%
==========================================
Files 8 8
Lines 405 408 +3
Branches 134 134
==========================================
+ Hits 343 346 +3
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 (13)
v2/contracts/evm/ZetaConnectorBase.sol (1)
51-51: Approved: PAUSER_ROLE granted to tssAddress, aligning with PR objective.The addition of
_grantRole(PAUSER_ROLE, tssAddress_);successfully implements the PR objective of introducing a pauser role to the TSS. This change allows the TSS address to pause the contract, potentially improving emergency response capabilities.However, it's important to note the security implications:
- Granting PAUSER_ROLE to tssAddress_ increases the number of entities that can pause the contract.
- This change slightly increases the attack surface of the contract.
Consider adding a comment explaining the rationale behind granting PAUSER_ROLE to tssAddress_. This will help future maintainers understand the design decision.
v2/contracts/evm/ERC20Custody.sol (2)
Line range hint
69-76: LGTM: Improved role management in updateTSSAddressThe changes ensure proper role management when updating the TSS address. Revoking roles from the old address before granting them to the new one is a good security practice.
Consider adding a check to ensure that the new TSS address is different from the current one to avoid unnecessary operations and event emissions.
Here's a suggested improvement:
function updateTSSAddress(address newTSSAddress) external onlyRole(DEFAULT_ADMIN_ROLE) { if (newTSSAddress == address(0)) revert ZeroAddress(); + if (newTSSAddress == tssAddress) revert SameAddress(); _revokeRole(WITHDRAWER_ROLE, tssAddress); _revokeRole(WHITELISTER_ROLE, tssAddress);
Line range hint
208-211: LGTM: Improved backward compatibility handling in deposit functionThe addition of the
supportsLegacycheck and marking the function as deprecated are good practices for managing the contract's evolution.Consider adding a deprecation notice in the revert message to provide more context to users.
Here's a suggested improvement:
function deposit( bytes calldata recipient, IERC20 asset, uint256 amount, bytes calldata message ) external nonReentrant whenNotPaused { - if (!supportsLegacy) revert LegacyMethodsNotSupported(); + if (!supportsLegacy) revert LegacyMethodsNotSupported("deposit function is deprecated"); if (!whitelisted[address(asset)]) revert NotWhitelisted();v2/test/ZetaConnectorNonNative.t.sol (2)
57-57: Add a comment explaining the role of thefooaddress.The
fooaddress is initialized with a specific value (0x9876). To improve code readability and maintainability, it would be helpful to add a comment explaining what this address represents in the context of these tests. For example, is it meant to represent an unauthorized user, a specific role, or serve some other purpose?
105-110: Approve changes and suggest additional test cases.The modifications to use
fooinstead oftssAddressfor testing unauthorized pause/unpause attempts are good. They verify that an address without thePAUSER_ROLEcannot perform these actions.However, to ensure comprehensive testing:
- Consider adding test cases to verify that
tssAddress(which should have thePAUSER_ROLE) can successfully pause and unpause the contract.- It might be beneficial to add a test case where the
PAUSER_ROLEis granted tofoo, and then verify that it can pause/unpause the contract.These additional tests would provide a more complete coverage of the pause functionality and role-based access control.
v2/test/ZetaConnectorNative.t.sol (4)
42-42: Consider using a more descriptive name for thefoovariable.The purpose of the
fooaddress variable is not clear from its name. Consider using a more descriptive name that reflects its intended use in the test cases. Additionally, please provide a comment explaining why this new address is being introduced and how it differs fromtssAddress.Also applies to: 57-57
147-148: Approved: Good test for unauthorized access, but consider adding explanatory comments.The changes correctly test the behavior when an unauthorized address attempts to pause and unpause the contract. This is a valuable test case for ensuring proper access control. However, to improve readability and maintainability:
- Consider adding a comment explaining why
foois being used instead oftssAddressin these specific test cases.- You might want to rename
footo something more descriptive, likeunauthorizedAddress, to make the intent of the test clearer.Also applies to: 151-152
155-155: Clarify the different roles or permissions being tested in this function.The
testWithdrawTogglePausefunction uses bothfoo(an unauthorized address) andtssAddressfor different operations. This mixed usage might be confusing to readers. Consider:
- Adding comments to explain the different scenarios being tested (e.g., unauthorized pause attempt, authorized pause, etc.).
- Structuring the test to clearly differentiate between the unauthorized and authorized actions.
- Ensuring that all relevant permission scenarios are covered in this test.
Line range hint
1-385: Overall: Good addition of access control tests, but improve clarity and documentation.The changes introduce valuable tests for unauthorized access attempts, which is crucial for ensuring proper access control in the ZetaConnectorNative contract. However, to enhance the maintainability and readability of the test suite:
- Use more descriptive variable names (e.g., replace
foowith something likeunauthorizedAddress).- Add comments explaining the purpose of new variables and test scenarios.
- Consider restructuring the
testWithdrawTogglePausefunction to clearly separate tests for different permission levels.- Ensure consistent naming conventions and test structure throughout the file.
These improvements will make the tests more self-explanatory and easier to maintain in the future.
v2/test/utils/GatewayEVMUpgradeTest.sol (2)
Line range hint
35-35: LGTM. Consider updating documentation.The addition of the
ExecutedV2event enhances the contract's ability to log execution actions in more detail. This change maintains backwards compatibility while allowing for future improvements in event handling.Consider updating the contract's documentation to reflect this new event and its usage. Also, it might be helpful to add a comment explaining the difference between
ExecutedandExecutedV2events for future developers.
Line range hint
173-195: LGTM. Consider adding documentation for the new execute function.The addition of this new
executefunction provides a simpler interface for non-authenticated calls, which is a good improvement for usability. It maintains the same access controls as the original function, ensuring security.Consider adding NatSpec documentation for this new function to clarify its purpose and usage, especially highlighting the difference between this and the authenticated
executefunction. For example:/// @notice Executes a non-authenticated call to a destination address without ERC20 tokens. /// @dev This function can only be called by the TSS address and it is payable. /// @param destination Address to call. /// @param data Calldata to pass to the call. /// @return The result of the call.v2/test/GatewayEVM.t.sol (1)
37-37: Consider using a more descriptive variable name.The variable name
foois not descriptive and doesn't convey its purpose. Consider using a more meaningful name that reflects its role in the tests, such asnonAuthorizedAddressorunprivilegedUser.v2/test/ERC20Custody.t.sol (1)
36-36: Consider using a more descriptive variable name.The variable name
foodoesn't provide context about its purpose in the test. Consider using a more descriptive name that reflects its role in the test cases, such asnonPauserAddressorunauthorizedAddress.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- v2/contracts/evm/ERC20Custody.sol (1 hunks)
- v2/contracts/evm/GatewayEVM.sol (1 hunks)
- v2/contracts/evm/ZetaConnectorBase.sol (1 hunks)
- v2/test/ERC20Custody.t.sol (3 hunks)
- v2/test/GatewayEVM.t.sol (3 hunks)
- v2/test/ZetaConnectorNative.t.sol (3 hunks)
- v2/test/ZetaConnectorNonNative.t.sol (3 hunks)
- v2/test/utils/GatewayEVMUpgradeTest.sol (1 hunks)
🧰 Additional context used
🔇 Additional comments (8)
v2/contracts/evm/ERC20Custody.sol (2)
46-46: LGTM: Pauser role granted to TSS addressThis change aligns well with the PR objective of introducing a pauser role to the TSS. It's consistent with the existing role assignments in the constructor and enhances the contract's control mechanisms.
Line range hint
1-228: Overall: Improved role management and backward compatibilityThe changes in this file effectively implement the PR objectives by:
- Introducing the pauser role for the TSS address.
- Enhancing the
updateTSSAddressfunction with proper role management.- Adding backward compatibility support through the
supportsLegacyflag.These improvements enhance the contract's security, flexibility, and maintainability. The code quality is good, with clear and consistent implementations throughout the file.
v2/test/ZetaConnectorNonNative.t.sol (1)
42-42: Clarify the purpose of thefooaddress variable.A new address variable
foohas been introduced. Could you please explain the intended purpose of this variable in the context of these tests? Understanding its role will help ensure that the tests accurately reflect the contract's behavior under different scenarios.v2/test/utils/GatewayEVMUpgradeTest.sol (2)
Line range hint
1-686: Overall assessment: LGTM with minor suggestions.The changes in this file align well with the PR objectives, introducing the pauser role to the TSS and improving the contract's functionality. The new
ExecutedV2event and the additionalexecutefunction enhance the contract's capabilities while maintaining backwards compatibility.Key points:
- The PAUSER_ROLE is now granted to the TSS address, allowing for emergency pausing.
- A new
ExecutedV2event has been added for more detailed logging.- An additional
executefunction has been introduced for non-authenticated calls.While the changes look good, consider the following minor improvements:
- Document the security implications of granting PAUSER_ROLE to the TSS address.
- Update the contract's documentation to reflect the new
ExecutedV2event and its usage.- Add NatSpec documentation for the new
executefunction to clarify its purpose and usage.These changes enhance the contract's functionality and maintainability. Good job!
68-68: LGTM, but consider security implications.The addition of
_grantRole(PAUSER_ROLE, tssAddress_);aligns with the PR objective of introducing a pauser role to the TSS. This allows the TSS to pause the contract in emergency situations, which can be beneficial for system security.However, it's important to consider the security implications of granting this role to the TSS address. If the TSS is compromised, an attacker could potentially pause the contract. Please ensure that appropriate safeguards are in place to protect the TSS and consider implementing additional checks or a multi-sig approach for pausing.
To verify the role assignment, you can run the following script:
✅ Verification successful
PAUSER_ROLE assignment to tssAddress_ verified.
The line
_grantRole(PAUSER_ROLE, tssAddress_);is present inv2/test/utils/GatewayEVMUpgradeTest.solas specified.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify PAUSER_ROLE assignment to tssAddress in initialize function # Expected: The script should find the line granting PAUSER_ROLE to tssAddress_ rg --type solidity '_grantRole\(PAUSER_ROLE,\s*tssAddress_\)' v2/test/utils/GatewayEVMUpgradeTest.solLength of output: 136
Script:
#!/bin/bash # Description: Verify PAUSER_ROLE assignment to tssAddress in initialize function # Expected: The script should find the line granting PAUSER_ROLE to tssAddress_ rg '_grantRole\(PAUSER_ROLE,\s*tssAddress_\)' v2/test/utils/GatewayEVMUpgradeTest.sol -g "*.sol"Length of output: 144
v2/test/ERC20Custody.t.sol (2)
230-231: LGTM: Good test for unauthorized pause attempt.This change correctly tests that an unauthorized address (foo) cannot pause the contract. It's a good practice to ensure proper access control for critical functions like pause.
234-235: Verify the use ofgatewayfor unpause test.This change correctly tests that an unauthorized address (foo) cannot unpause the contract. However, it's testing the unpause function on the
gatewayinstead ofcustody. Is this intentional? If not, consider changing it tocustody.unpause()for consistency with the previous pause test and the overall test function name.v2/contracts/evm/GatewayEVM.sol (1)
66-66: Appropriate assignment ofPAUSER_ROLEtotssAddress_Granting the
PAUSER_ROLEtotssAddress_allows the TSS to pause the contract when necessary, enhancing control and security over contract operations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
v2/contracts/evm/ZetaConnectorBase.sol (2)
75-75: Approved: Enhanced role management for TSS addressThe addition of
_grantRole(PAUSER_ROLE, tssAddress_);is a good security enhancement. It allows the TSS address to pause the contract in case of emergencies or when upgrades are needed, which aligns with its existing roles (WITHDRAWER_ROLE and TSS_ROLE).Consider adding a comment above this line to explain why the TSS address is granted the PAUSER_ROLE, for better code documentation. For example:
// Grant PAUSER_ROLE to TSS for enhanced security and quicker response to potential issues _grantRole(PAUSER_ROLE, tssAddress_);
Line range hint
84-97: UpdateupdateTSSAddressfunction to handle PAUSER_ROLEThe
updateTSSAddressfunction currently doesn't manage the PAUSER_ROLE when updating the TSS address. This could lead to inconsistency in role management.Consider updating the
updateTSSAddressfunction to handle the PAUSER_ROLE:function updateTSSAddress(address newTSSAddress) external onlyRole(DEFAULT_ADMIN_ROLE) { if (newTSSAddress == address(0)) revert ZeroAddress(); _revokeRole(WITHDRAWER_ROLE, tssAddress); _revokeRole(TSS_ROLE, tssAddress); + _revokeRole(PAUSER_ROLE, tssAddress); _grantRole(WITHDRAWER_ROLE, newTSSAddress); _grantRole(TSS_ROLE, newTSSAddress); + _grantRole(PAUSER_ROLE, newTSSAddress); tssAddress = newTSSAddress; emit UpdatedZetaConnectorTSSAddress(newTSSAddress); }This change ensures that the PAUSER_ROLE is properly transferred to the new TSS address, maintaining consistency with the other roles.
v2/test/ZetaConnectorNonNative.t.sol (2)
59-59: Consider using a more descriptive name for the new address variable.The initialization of
foowith a specific address (0x9876) is correct for testing purposes. However, to improve code readability and maintainability, consider using a more descriptive name that reflects its role or purpose in the tests, such asunauthorizedAddressornonPauserAddress.
114-119: Approve changes and suggest additional test case.The modifications to use
fooinstead oftssAddressfor testing unauthorized access are appropriate. These changes correctly verify that an address without thePAUSER_ROLEcannot pause or unpause the contract.To further strengthen the test suite, consider adding an additional test case that verifies the following:
- Grant the
PAUSER_ROLEto thefooaddress.- Verify that
foocan now successfully pause and unpause the contract.This would provide complete coverage of the role-based access control for the pause functionality.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- v2/contracts/evm/ERC20Custody.sol (1 hunks)
- v2/contracts/evm/ZetaConnectorBase.sol (1 hunks)
- v2/test/ERC20Custody.t.sol (3 hunks)
- v2/test/GatewayEVM.t.sol (3 hunks)
- v2/test/ZetaConnectorNative.t.sol (3 hunks)
- v2/test/ZetaConnectorNonNative.t.sol (3 hunks)
- v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- v2/contracts/evm/ERC20Custody.sol
- v2/test/ERC20Custody.t.sol
- v2/test/GatewayEVM.t.sol
- v2/test/ZetaConnectorNative.t.sol
🧰 Additional context used
🔇 Additional comments (6)
v2/test/ZetaConnectorNonNative.t.sol (2)
42-42: Clarify the purpose of thefooaddress variable.A new address variable
foohas been introduced. Could you please explain its intended use and how it differs from existing address variables liketssAddress? This will help in understanding the context of the changes made to the test cases.
Line range hint
1-424: Overall, the changes improve test coverage and clarity.The introduction of the new address variable (currently named
foo) and the corresponding updates to thetestWithdrawTogglePausefunction enhance the test suite by explicitly verifying the behavior for unauthorized access attempts. These changes strengthen the validation of the contract's access control mechanisms.To further improve the test file:
- Consider using a more descriptive name for the
foovariable.- Add an additional test case to verify the behavior when the new address is granted the
PAUSER_ROLE.These modifications will result in a more comprehensive and easily understandable test suite for the ZetaConnectorNonNative contract.
v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol (4)
Line range hint
41-41: LGTM: New eventExecutedV2addedThe addition of the
ExecutedV2event is a good practice for versioning and maintaining backward compatibility. It allows for distinguishing between different execution scenarios while preserving the existingExecutedevent.
68-68: LGTM: GrantingPAUSER_ROLEto TSS addressGranting the
PAUSER_ROLEto the TSS address is a good security measure. It allows the TSS to pause the contract if necessary, which is consistent with its existing permissions and responsibilities.
Line range hint
132-157: LGTM: Updatedexecutefunction withMessageContextThe changes to this
executefunction enhance its capabilities by including aMessageContextparameter. This modification allows for better handling of cross-chain message context. The function correctly emits the originalExecutedevent, maintaining backward compatibility.
Line range hint
164-185: LGTM: New overloadedexecutefunction addedThe addition of this overloaded
executefunction is a good design choice. It maintains the existing functionality for calls that don't require message context, while emitting the newExecutedV2event. This approach provides flexibility and backward compatibility.
add pauser role to tss in all evm contracts
Related to zeta-chain/node#3000
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
PAUSER_ROLEandWITHDRAWER_ROLE.Bug Fixes
depositfunction to streamline operations and improve clarity.Tests