fix: missing tss update in erc20custody, zetaConnector and gatewayEVM #363
Conversation
skosito
requested review from
andresaiello,
brewmaster012,
lumtis,
CharlieMc0,
fadeev and
fbac
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new function, Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #363 +/- ##
==========================================
- Coverage 83.10% 83.07% -0.03%
==========================================
Files 8 8
Lines 361 384 +23
Branches 118 122 +4
==========================================
+ Hits 300 319 +19
- Misses 61 65 +4 ☔ 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 (4)
v2/contracts/evm/ERC20Custody.sol (1)
64-69: LGTM with suggestion: Consider adding an event for TSS address updates.The
updateTSSAddressfunction is correctly implemented with proper access control and a zero address check. This aligns with the PR objectives of adding a TSS update function with appropriate role permissions.Consider emitting an event when the TSS address is updated. This would be beneficial for off-chain monitoring and transparency. Here's a suggested implementation:
function updateTSSAddress(address newTSSAddress) external onlyRole(TSS_UPDATER_ROLE) { if (newTSSAddress == address(0)) revert ZeroAddress(); + address oldTSSAddress = tssAddress; tssAddress = newTSSAddress; + emit TSSAddressUpdated(oldTSSAddress, newTSSAddress); } +event TSSAddressUpdated(address indexed oldTSSAddress, address indexed newTSSAddress);v2/contracts/evm/GatewayEVM.sol (2)
88-92: Approve updateTSSAddress function with suggestions for improvementThe
updateTSSAddressfunction successfully implements the main objective of the PR, allowing for secure updates to the TSS address. The use ofonlyRole(TSS_UPDATER_ROLE)ensures proper access control, and the zero address check is a good security practice.However, consider the following improvements:
- Emit an event when the TSS address is updated for better off-chain monitoring.
- Add a check to prevent updating the TSS address to the same value to save gas.
Here's a suggested implementation incorporating these improvements:
function updateTSSAddress(address newTSSAddress) external onlyRole(TSS_UPDATER_ROLE) { if (newTSSAddress == address(0)) revert ZeroAddress(); + if (newTSSAddress == tssAddress) revert SameAddress(); tssAddress = newTSSAddress; + emit TSSAddressUpdated(newTSSAddress); } +event TSSAddressUpdated(address newTSSAddress);Don't forget to add the
SameAddresserror to your error declarations.
Line range hint
45-92: Summary: TSS update functionality successfully implementedThe changes in this PR successfully implement the TSS update functionality in the
GatewayEVMcontract. The implementation includes:
- A new
TSS_UPDATER_ROLEfor access control.- Granting this role to the initial TSS address in the
initializefunction.- A new
updateTSSAddressfunction with appropriate access control and basic safety checks.These changes align well with the PR objectives and enhance the contract's functionality. The suggested improvements (emitting an event and checking for same address updates) would further increase the robustness and efficiency of the implementation.
Consider documenting the rationale behind granting the
TSS_UPDATER_ROLEto the initial TSS address, as this architectural decision might have implications for the system's security model and upgrade patterns.v2/test/GatewayEVM.t.sol (1)
47-47: [Nitpick] Group role constants together for better readabilityConsider moving the declaration of
TSS_UPDATER_ROLEto be with the other role constants (TSS_ROLE,WITHDRAWER_ROLE, etc.) to maintain consistency and improve code organization.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
🔇 Files ignored due to path filters (55)
v2/docs/src/SUMMARY.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/interface.Revertable.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/struct.RevertContext.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/struct.RevertOptions.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ERC20Custody.sol/contract.ERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/GatewayEVM.sol/contract.GatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorBase.sol/abstract.ZetaConnectorBase.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorNative.sol/contract.ZetaConnectorNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorNonNative.sol/contract.ZetaConnectorNonNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20CustodyErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20CustodyEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IZetaConnector.sol/interface.IZetaConnectorEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IZetaNonEthNew.sol/interface.IZetaNonEthNew.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/GatewayZEVM.sol/contract.GatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/README.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/SystemContract.sol/contract.SystemContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/SystemContract.sol/interface.SystemContractErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/ZRC20.sol/contract.ZRC20.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/ZRC20.sol/interface.ZRC20Errors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/ISystem.sol/interface.ISystem.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IWZETA.sol/interface.IWETH9.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/enum.CoinType.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.IZRC20.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.IZRC20Metadata.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.ZRC20Events.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.UniversalContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.zContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/struct.zContext.mdis excluded by!v2/docs/**v2/docs/src/index.mdis excluded by!v2/docs/**v2/pkg/erc20custody.sol/erc20custody.gois excluded by!v2/pkg/**v2/pkg/erc20custody.t.sol/erc20custodytest.gois excluded by!v2/pkg/**v2/pkg/erc20custodyechidnatest.sol/erc20custodyechidnatest.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.sol/gatewayevm.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevmtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmechidnatest.sol/gatewayevmechidnatest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmupgrade.t.sol/gatewayevmuupsupgradetest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmzevm.t.sol/gatewayevmzevmtest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornative.t.sol/zetaconnectornativetest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.t.sol/zetaconnectornonnativetest.gois excluded by!v2/pkg/**v2/types/ERC20Custody.tsis excluded by!v2/types/**v2/types/ERC20CustodyEchidnaTest.tsis excluded by!v2/types/**v2/types/GatewayEVM.tsis excluded by!v2/types/**v2/types/GatewayEVMEchidnaTest.tsis excluded by!v2/types/**v2/types/factories/ERC20CustodyEchidnaTest__factory.tsis excluded by!v2/types/**v2/types/factories/ERC20Custody__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVMEchidnaTest__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVM__factory.tsis excluded by!v2/types/**
📒 Files selected for processing (4)
- v2/contracts/evm/ERC20Custody.sol (3 hunks)
- v2/contracts/evm/GatewayEVM.sol (3 hunks)
- v2/test/ERC20Custody.t.sol (2 hunks)
- v2/test/GatewayEVM.t.sol (2 hunks)
🔇 Additional comments not posted (10)
v2/contracts/evm/ERC20Custody.sol (2)
35-36: LGTM: New role constant added correctly.The
TSS_UPDATER_ROLEconstant is properly defined usingkeccak256, consistent with other role definitions in the contract. This addition aligns with the PR objective of implementing a TSS update function with appropriate role permissions.
51-51: LGTM: TSS_UPDATER_ROLE granted in constructor.The
TSS_UPDATER_ROLEis correctly granted to thetssAddress_in the constructor. This ensures that the TSS address has the necessary permissions to update itself from the contract's initialization, aligning with the PR objectives.v2/contracts/evm/GatewayEVM.sol (1)
45-46: LGTM: New role constant added correctlyThe addition of the
TSS_UPDATER_ROLEconstant is well-implemented and consistent with the existing role definitions in the contract. This new role aligns with the PR objectives to implement a TSS update function with appropriate permissions.v2/test/GatewayEVM.t.sol (3)
83-88: TesttestTSSUpgradecorrectly verifies TSS address updateThe test correctly verifies that the TSS address can be updated by an account with the
TSS_UPDATER_ROLE.
90-94: TesttestTSSUpgradeFailsIfSenderIsNotTSSUpdatercorrectly checks unauthorized accessThe test properly ensures that an account without the
TSS_UPDATER_ROLEcannot update the TSS address, expecting a revert with the appropriate error.
96-100: TesttestTSSUpgradeFailsIfZeroAddresscorrectly handles zero address updateThe test correctly verifies that attempting to update the TSS address to the zero address results in a revert with the expected
ZeroAddresserror.v2/test/ERC20Custody.t.sol (4)
48-48: Addition ofTSS_UPDATER_ROLEconstant is appropriate and consistent.The new constant
TSS_UPDATER_ROLEis correctly defined and follows the existing naming conventions alongside other role constants.
84-89: TesttestTSSUpgradeeffectively verifies TSS address update functionality.This test ensures that the TSS address can be updated by an authorized account (
tssAddress) and that the new address is correctly set toowner. The use ofvm.startPrank(tssAddress)accurately simulates the correct permissions.
91-95: Test correctly handles unauthorized access intestTSSUpgradeFailsIfSenderIsNotTSSUpdater.The test verifies that an account without the
TSS_UPDATER_ROLE(in this case,owner) cannot update the TSS address, and it expects the correct revert withAccessControlUnauthorizedAccounterror.
97-101: Validation of zero address intestTSSUpgradeFailsIfZeroAddressis correctly implemented.The test confirms that attempting to update the TSS address to the zero address reverts with the
ZeroAddresserror, ensuring robustness against invalid inputs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
v2/contracts/evm/interfaces/IERC20Custody.sol (1)
43-46: LGTM! Consider a minor comment improvement.The new
UpdatedCustodyTSSAddressevent is well-defined and appropriately placed within theIERC20CustodyEventsinterface. It follows the existing naming conventions and provides a clear purpose.Consider updating the comment to be more specific:
- /// @notice Emitted when tss address is updated + /// @notice Emitted when the custody TSS address is updated /// @param newTSSAddress new tss address event UpdatedCustodyTSSAddress(address newTSSAddress);This change aligns the comment more closely with the event's name and purpose.
v2/contracts/evm/interfaces/IGatewayEVM.sol (1)
52-55: LGTM! Consider adding more detailed documentation.The addition of the
UpdatedGatewayTSSAddressevent is appropriate and well-placed within theIGatewayEVMEventsinterface. It will be useful for tracking changes to the TSS address.Consider expanding the event documentation to provide more context:
/// @notice Emitted when the Gateway TSS (Threshold Signature Scheme) address is updated /// @param newTSSAddress The new TSS address event UpdatedGatewayTSSAddress(address newTSSAddress);This additional context helps developers understand the significance of the TSS in the Gateway system.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
🔇 Files ignored due to path filters (71)
v2/docs/src/contracts/Revert.sol/interface.Revertable.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/struct.RevertContext.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/struct.RevertOptions.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ERC20Custody.sol/contract.ERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/GatewayEVM.sol/contract.GatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorBase.sol/abstract.ZetaConnectorBase.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorNative.sol/contract.ZetaConnectorNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorNonNative.sol/contract.ZetaConnectorNonNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20CustodyErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20CustodyEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IZetaConnector.sol/interface.IZetaConnectorEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IZetaNonEthNew.sol/interface.IZetaNonEthNew.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/GatewayZEVM.sol/contract.GatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/SystemContract.sol/contract.SystemContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/SystemContract.sol/interface.SystemContractErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/ZRC20.sol/contract.ZRC20.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/ZRC20.sol/interface.ZRC20Errors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/ISystem.sol/interface.ISystem.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IWZETA.sol/interface.IWETH9.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/enum.CoinType.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.IZRC20.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.IZRC20Metadata.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.ZRC20Events.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.UniversalContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.zContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/struct.zContext.mdis excluded by!v2/docs/**v2/pkg/erc20custody.sol/erc20custody.gois excluded by!v2/pkg/**v2/pkg/erc20custody.t.sol/erc20custodytest.gois excluded by!v2/pkg/**v2/pkg/erc20custodyechidnatest.sol/erc20custodyechidnatest.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.sol/gatewayevm.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevmtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmechidnatest.sol/gatewayevmechidnatest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmupgrade.t.sol/gatewayevmuupsupgradetest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmupgradetest.sol/gatewayevmupgradetest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmzevm.t.sol/gatewayevmzevmtest.gois excluded by!v2/pkg/**v2/pkg/ierc20custody.sol/ierc20custody.gois excluded by!v2/pkg/**v2/pkg/ierc20custody.sol/ierc20custodyevents.gois excluded by!v2/pkg/**v2/pkg/igatewayevm.sol/igatewayevm.gois excluded by!v2/pkg/**v2/pkg/igatewayevm.sol/igatewayevmevents.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornative.sol/zetaconnectornative.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornative.t.sol/zetaconnectornativetest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.sol/zetaconnectornonnative.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.t.sol/zetaconnectornonnativetest.gois excluded by!v2/pkg/**v2/types/ERC20Custody.tsis excluded by!v2/types/**v2/types/ERC20CustodyEchidnaTest.tsis excluded by!v2/types/**v2/types/GatewayEVM.tsis excluded by!v2/types/**v2/types/GatewayEVMEchidnaTest.tsis excluded by!v2/types/**v2/types/GatewayEVMUpgradeTest.tsis excluded by!v2/types/**v2/types/IERC20Custody.sol/IERC20Custody.tsis excluded by!v2/types/**v2/types/IERC20Custody.sol/IERC20CustodyEvents.tsis excluded by!v2/types/**v2/types/IGatewayEVM.sol/IGatewayEVM.tsis excluded by!v2/types/**v2/types/IGatewayEVM.sol/IGatewayEVMEvents.tsis excluded by!v2/types/**v2/types/factories/ERC20CustodyEchidnaTest__factory.tsis excluded by!v2/types/**v2/types/factories/ERC20Custody__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVMEchidnaTest__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVMUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVM__factory.tsis excluded by!v2/types/**v2/types/factories/IERC20Custody.sol/IERC20CustodyEvents__factory.tsis excluded by!v2/types/**v2/types/factories/IERC20Custody.sol/IERC20Custody__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayEVM.sol/IGatewayEVMEvents__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayEVM.sol/IGatewayEVM__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNative__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNonNative__factory.tsis excluded by!v2/types/**
📒 Files selected for processing (6)
- v2/contracts/evm/ERC20Custody.sol (1 hunks)
- v2/contracts/evm/GatewayEVM.sol (1 hunks)
- v2/contracts/evm/interfaces/IERC20Custody.sol (1 hunks)
- v2/contracts/evm/interfaces/IGatewayEVM.sol (1 hunks)
- v2/test/ERC20Custody.t.sol (2 hunks)
- v2/test/GatewayEVM.t.sol (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- v2/contracts/evm/ERC20Custody.sol
- v2/contracts/evm/GatewayEVM.sol
- v2/test/GatewayEVM.t.sol
🔇 Additional comments not posted (6)
v2/contracts/evm/interfaces/IERC20Custody.sol (1)
43-46: Verify implementation and test coverage for TSS address update.The addition of the
UpdatedCustodyTSSAddressevent suggests a new functionality to update the TSS address. However, there's no corresponding method in theIERC20Custodyinterface for this update.Please ensure that:
- The implementation contract correctly implements the TSS address update functionality.
- Appropriate access controls are in place for this update.
- Unit tests cover this new functionality, including emitting the event.
Run the following script to verify the implementation and test coverage:
This script will help verify the proper implementation, access control, event emission, and test coverage for the new TSS address update functionality.
v2/test/ERC20Custody.t.sol (5)
48-48: LGTM: Default admin role constant added correctly.The
DEFAULT_ADMIN_ROLEconstant is correctly defined with the appropriate value (0x00) for OpenZeppelin's AccessControl. This addition aligns with the existing role constants in the contract and will be useful for access control checks in the new test functions.
84-91: LGTM: Comprehensive test for TSS address update.This test function effectively verifies the TSS address update functionality:
- It correctly sets the caller context to the owner.
- It checks for the expected event emission.
- It verifies the state change by asserting the new TSS address.
The test covers both the behavior and the state change, ensuring the
updateTSSAddressfunction works as expected.
93-99: LGTM: Proper access control test for TSS address update.This test function effectively verifies that the TSS address update is protected by access control:
- It correctly sets an unauthorized caller context (TSS address).
- It expects the appropriate
AccessControlUnauthorizedAccounterror with the correct parameters.- It attempts to update the TSS address, which should fail.
The test ensures that only authorized accounts (with the
DEFAULT_ADMIN_ROLE) can update the TSS address.
101-105: LGTM: Proper input validation test for TSS address update.This test function effectively verifies that the TSS address cannot be set to a zero address:
- It correctly sets the caller context to the owner to isolate the test to input validation.
- It expects the appropriate
ZeroAddresserror.- It attempts to update the TSS address to a zero address, which should fail.
The test ensures that the
updateTSSAddressfunction properly validates its input, preventing the TSS address from being set to an invalid value.
Line range hint
48-105: LGTM: Comprehensive test coverage for TSS address update functionality.The additions to this test file significantly improve the test coverage for the TSS address update feature:
- The
DEFAULT_ADMIN_ROLEconstant is correctly defined for use in access control tests.testTSSUpgradeverifies the successful updating of the TSS address.testTSSUpgradeFailsIfSenderIsNotTSSUpdaterensures proper access control.testTSSUpgradeFailsIfZeroAddressvalidates input to prevent setting an invalid TSS address.These tests collectively cover the happy path, access control, and input validation aspects of the TSS address update functionality, enhancing the overall robustness of the ERC20Custody contract tests.
v2/test/GatewayEVM.t.sol
Outdated
| vm.expectEmit(true, true, true, true, address(gateway)); | ||
| emit UpdatedGatewayTSSAddress(tssAddress); | ||
| gateway.updateTSSAddress(tssAddress); | ||
| address newTssAddress = gateway.tssAddress(); |
There was a problem hiding this comment.
I might think we're missing assertion for checking the roles on previous TSS have been revoked? Same for the other contract
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
v2/contracts/evm/GatewayEVM.sol (1)
74-83: LGTM! Consider adding more detailed documentation.The
_executefunction is well-implemented as an internal utility for executing arbitrary calls. It correctly handles the return value and reverts on failure.Consider adding more detailed documentation, especially regarding:
- The purpose of this internal function within the contract's architecture.
- The security implications of executing arbitrary calls.
- How the
msg.valueis handled and potential risks associated with it.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (76)
v2/docs/src/contracts/Revert.sol/interface.Revertable.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/struct.RevertContext.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/struct.RevertOptions.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ERC20Custody.sol/contract.ERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/GatewayEVM.sol/contract.GatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorBase.sol/abstract.ZetaConnectorBase.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorNative.sol/contract.ZetaConnectorNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorNonNative.sol/contract.ZetaConnectorNonNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20CustodyErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20CustodyEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.Callable.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/struct.MessageContext.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IZetaConnector.sol/interface.IZetaConnectorEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IZetaNonEthNew.sol/interface.IZetaNonEthNew.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/GatewayZEVM.sol/contract.GatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/SystemContract.sol/contract.SystemContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/SystemContract.sol/interface.SystemContractErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/ZRC20.sol/contract.ZRC20.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/ZRC20.sol/interface.ZRC20Errors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/struct.CallOptions.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/ISystem.sol/interface.ISystem.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IWZETA.sol/interface.IWETH9.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/enum.CoinType.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.IZRC20.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.IZRC20Metadata.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.ZRC20Events.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.UniversalContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.zContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/struct.zContext.mdis excluded by!v2/docs/**v2/pkg/erc20custody.sol/erc20custody.gois excluded by!v2/pkg/**v2/pkg/erc20custody.t.sol/erc20custodytest.gois excluded by!v2/pkg/**v2/pkg/erc20custodyechidnatest.sol/erc20custodyechidnatest.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.sol/gatewayevm.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevmtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmechidnatest.sol/gatewayevmechidnatest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmupgrade.t.sol/gatewayevmuupsupgradetest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmupgradetest.sol/gatewayevmupgradetest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmzevm.t.sol/gatewayevmzevmtest.gois excluded by!v2/pkg/**v2/pkg/ierc20custody.sol/ierc20custody.gois excluded by!v2/pkg/**v2/pkg/ierc20custody.sol/ierc20custodyevents.gois excluded by!v2/pkg/**v2/pkg/igatewayevm.sol/igatewayevm.gois excluded by!v2/pkg/**v2/pkg/igatewayevm.sol/igatewayevmevents.gois excluded by!v2/pkg/**v2/pkg/receiverevm.sol/receiverevm.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornative.sol/zetaconnectornative.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornative.t.sol/zetaconnectornativetest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.sol/zetaconnectornonnative.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.t.sol/zetaconnectornonnativetest.gois excluded by!v2/pkg/**v2/types/ERC20Custody.tsis excluded by!v2/types/**v2/types/ERC20CustodyEchidnaTest.tsis excluded by!v2/types/**v2/types/GatewayEVM.tsis excluded by!v2/types/**v2/types/GatewayEVMEchidnaTest.tsis excluded by!v2/types/**v2/types/GatewayEVMUpgradeTest.tsis excluded by!v2/types/**v2/types/IERC20Custody.sol/IERC20Custody.tsis excluded by!v2/types/**v2/types/IERC20Custody.sol/IERC20CustodyEvents.tsis excluded by!v2/types/**v2/types/IGatewayEVM.sol/IGatewayEVM.tsis excluded by!v2/types/**v2/types/IGatewayEVM.sol/IGatewayEVMEvents.tsis excluded by!v2/types/**v2/types/factories/ERC20CustodyEchidnaTest__factory.tsis excluded by!v2/types/**v2/types/factories/ERC20Custody__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVMEchidnaTest__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVMUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVM__factory.tsis excluded by!v2/types/**v2/types/factories/IERC20Custody.sol/IERC20CustodyEvents__factory.tsis excluded by!v2/types/**v2/types/factories/IERC20Custody.sol/IERC20Custody__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayEVM.sol/IGatewayEVMEvents__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayEVM.sol/IGatewayEVM__factory.tsis excluded by!v2/types/**v2/types/factories/ReceiverEVM__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNative__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNonNative__factory.tsis excluded by!v2/types/**
📒 Files selected for processing (4)
- v2/contracts/evm/GatewayEVM.sol (1 hunks)
- v2/contracts/evm/interfaces/IGatewayEVM.sol (1 hunks)
- v2/test/ERC20Custody.t.sol (2 hunks)
- v2/test/GatewayEVM.t.sol (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- v2/contracts/evm/interfaces/IGatewayEVM.sol
- v2/test/ERC20Custody.t.sol
- v2/test/GatewayEVM.t.sol
🔇 Additional comments (2)
v2/contracts/evm/GatewayEVM.sol (2)
85-96: LGTM! Event emission added as requested.The
updateTSSAddressfunction is well-implemented with proper access control and role management. The zero address check adds an extra layer of security.The event emission (
UpdatedGatewayTSSAddress) has been added as requested in the previous review, which improves the contract's observability.
74-96: Overall improvements to contract flexibility and maintainabilityThe additions of the
_executeandupdateTSSAddressfunctions enhance the contract in the following ways:
- The
_executefunction centralizes the logic for executing calls, potentially improving maintainability.- The
updateTSSAddressfunction adds necessary flexibility to update the TSS address, addressing the main objective of this PR.These changes align well with the PR objectives and don't introduce apparent security vulnerabilities.
skosito
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
v2/contracts/evm/ZetaConnectorBase.sol (2)
55-67: Add a Check to Prevent Unnecessary UpdatesThe
updateTSSAddressfunction does not check if thenewTSSAddressis different from the currenttssAddress. Updating to the same address unnecessarily revokes and grants roles.Include a condition to verify the new address is different:
function updateTSSAddress(address newTSSAddress) external onlyRole(DEFAULT_ADMIN_ROLE) { if (newTSSAddress == address(0)) revert ZeroAddress(); + if (newTSSAddress == tssAddress) revert("New TSS address must be different from the current address"); _revokeRole(WITHDRAWER_ROLE, tssAddress); _revokeRole(TSS_ROLE, tssAddress); _grantRole(WITHDRAWER_ROLE, newTSSAddress); _grantRole(TSS_ROLE, newTSSAddress); tssAddress = newTSSAddress; emit UpdatedZetaConnectorTSSAddress(newTSSAddress); }
58-62: Ensure Atomic Role Transfer to Prevent Access GapsThere is a brief moment after revoking roles from the old
tssAddressand before granting them to thenewTSSAddresswhere no address holds the necessary roles. While this function executes atomically, consider the order of operations to ensure security best practices.Consider assigning the new address before revoking and granting roles:
function updateTSSAddress(address newTSSAddress) external onlyRole(DEFAULT_ADMIN_ROLE) { if (newTSSAddress == address(0)) revert ZeroAddress(); + address oldTssAddress = tssAddress; + tssAddress = newTSSAddress; _revokeRole(WITHDRAWER_ROLE, oldTssAddress); _revokeRole(TSS_ROLE, oldTssAddress); _grantRole(WITHDRAWER_ROLE, newTSSAddress); _grantRole(TSS_ROLE, newTSSAddress); emit UpdatedZetaConnectorTSSAddress(newTSSAddress); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (63)
v2/docs/src/contracts/Revert.sol/interface.Revertable.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/struct.RevertContext.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/struct.RevertOptions.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ERC20Custody.sol/contract.ERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/GatewayEVM.sol/contract.GatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorBase.sol/abstract.ZetaConnectorBase.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorNative.sol/contract.ZetaConnectorNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorNonNative.sol/contract.ZetaConnectorNonNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20CustodyErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20CustodyEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.Callable.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/struct.MessageContext.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IZetaConnector.sol/interface.IZetaConnectorEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IZetaNonEthNew.sol/interface.IZetaNonEthNew.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/GatewayZEVM.sol/contract.GatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/SystemContract.sol/contract.SystemContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/SystemContract.sol/interface.SystemContractErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/ZRC20.sol/contract.ZRC20.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/ZRC20.sol/interface.ZRC20Errors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/struct.CallOptions.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/ISystem.sol/interface.ISystem.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IWZETA.sol/interface.IWETH9.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/enum.CoinType.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.IZRC20.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.IZRC20Metadata.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.ZRC20Events.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.UniversalContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.zContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/struct.zContext.mdis excluded by!v2/docs/**v2/pkg/erc20custody.t.sol/erc20custodytest.gois excluded by!v2/pkg/**v2/pkg/erc20custodyechidnatest.sol/erc20custodyechidnatest.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.sol/gatewayevm.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevmtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmechidnatest.sol/gatewayevmechidnatest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmupgrade.t.sol/gatewayevmuupsupgradetest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmupgradetest.sol/gatewayevmupgradetest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmzevm.t.sol/gatewayevmzevmtest.gois excluded by!v2/pkg/**v2/pkg/izetaconnector.sol/izetaconnectorevents.gois excluded by!v2/pkg/**v2/pkg/zetaconnectorbase.sol/zetaconnectorbase.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornative.sol/zetaconnectornative.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornative.t.sol/zetaconnectornativetest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.sol/zetaconnectornonnative.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.t.sol/zetaconnectornonnativetest.gois excluded by!v2/pkg/**v2/types/IZetaConnector.sol/IZetaConnectorEvents.tsis excluded by!v2/types/**v2/types/ZetaConnectorBase.tsis excluded by!v2/types/**v2/types/ZetaConnectorNative.tsis excluded by!v2/types/**v2/types/ZetaConnectorNonNative.tsis excluded by!v2/types/**v2/types/factories/ERC20CustodyEchidnaTest__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVMEchidnaTest__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVMUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVM__factory.tsis excluded by!v2/types/**v2/types/factories/IZetaConnector.sol/IZetaConnectorEvents__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorBase__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNative__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNonNative__factory.tsis excluded by!v2/types/**
📒 Files selected for processing (5)
- v2/contracts/evm/ZetaConnectorBase.sol (2 hunks)
- v2/contracts/evm/interfaces/IZetaConnector.sol (1 hunks)
- v2/test/ERC20Custody.t.sol (2 hunks)
- v2/test/GatewayEVM.t.sol (1 hunks)
- v2/test/ZetaConnectorNative.t.sol (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- v2/test/ERC20Custody.t.sol
- v2/test/GatewayEVM.t.sol
🔇 Additional comments (2)
v2/contracts/evm/interfaces/IZetaConnector.sol (1)
27-29: LGTM: New eventUpdatedZetaConnectorTSSAddressis well-defined and documented.The new event
UpdatedZetaConnectorTSSAddressis a good addition to the interface. It follows the existing coding style and documentation patterns. The event name and parameter are clear and appropriate for its purpose of signaling updates to the TSS address.v2/test/ZetaConnectorNative.t.sol (1)
49-49: Consistency in Role DefinitionsThe addition of
TSS_ROLEaligns with the existing role constants and follows the established pattern, ensuring consistency in role management.
| /// @param destination Address to call. | ||
| /// @param data Calldata to pass to the call. | ||
| /// @return The result of the call. | ||
| function _execute(address destination, bytes calldata data) internal returns (bytes memory) { |
There was a problem hiding this comment.
May I miss something, what is this function added in this PR?
| /// @param destination Address to call. | ||
| /// @param data Calldata to pass to the call. | ||
| /// @return The result of the call. | ||
| function _execute(address destination, bytes calldata data) internal returns (bytes memory) { |
There was a problem hiding this comment.
Also, flagging this here, why was this function added to the PR?
There was a problem hiding this comment.
bad merge, was cleaned up after #368
| @@ -58,6 +58,22 @@ contract ERC20Custody is IERC20Custody, ReentrancyGuard, AccessControl, Pausable | |||
| _unpause(); | |||
| } | |||
|
|
|||
| /// @notice Update tss address | |||
| /// @param newTSSAddress new tss address | |||
| function updateTSSAddress(address newTSSAddress) external onlyRole(DEFAULT_ADMIN_ROLE) { | |||
There was a problem hiding this comment.
Theoretically, here, the same address that is already used could be passed. That alone can lead to no real issues, but it would also be nice to check that. The same goes for the other 2
|
|
||
| /// @notice Emitted when tss address is updated | ||
| /// @param newTSSAddress new tss address | ||
| event UpdatedCustodyTSSAddress(address newTSSAddress); |
There was a problem hiding this comment.
The event should contain both the old and the new values. The same goes for the one in GatewayEVM and Zetaconnector
Summary by CodeRabbit
New Features
Bug Fixes
Tests