Closed skosito closed 2 months ago
[!WARNING]
Rate limit exceeded
@skosito has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 4 seconds before requesting another review.
How to resolve this issue?
After the wait time has elapsed, a review can be triggered using the `@coderabbitai review` command as a PR comment. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit.How do rate limits work?
CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our [FAQ](https://coderabbit.ai/docs/faq) for further information.Commits
Files that changed from the base of the PR and between 9c324a005dbe49c763d5140eb229f352121ad95a and 77f2bf0bb369b12d5e3f5658a7d458dcd8681613.
The recent changes enhance the security and functionality of various smart contracts by implementing stricter access controls, primarily centered around a new trusted signer (TSS) address. Key functions related to token withdrawals and transactions are now restricted to authorized entities, improving overall contract integrity and user experience through better error handling and event logging.
Files | Change Summary |
---|---|
contracts/prototypes/evm/ERC20CustodyNew.sol , IERC20CustodyNew.sol , GatewayEVM.sol |
Introduced tssAddress for enhanced access control; added onlyTSS modifier for sensitive functions; implemented event and error interfaces for better logging. |
contracts/prototypes/evm/ZetaConnectorNative.sol , ZetaConnectorNonNative.sol , ZetaConnectorNewBase.sol |
Updated constructors to accept tssAddress ; secured withdrawal functions with onlyTSS modifier, restricting access to authorized entities. |
test/fuzz/GatewayEVMEchidnaTest.sol , test/prototypes/GatewayEVMUniswap.spec.ts |
Modified deployment logic to include tssAddress ; adjusted interactions to validate against tssAddress , enhancing test cases for access control. |
testFoundry/GatewayEVM.t.sol , GatewayEVMUpgrade.t.sol , GatewayEVMZEVM.t.sol |
Enhanced initialization of contracts with tssAddress ; added tests to validate access control mechanisms enforcing sender checks against tssAddress . |
testFoundry/ZetaConnectorNative.t.sol , ZetaConnectorNonNative.t.sol |
Implemented TSS checks in withdrawal tests; modified contract structure to align with new event handling requirements, improving security during withdrawal operations. |
sequenceDiagram
participant User
participant TSS
participant ERC20CustodyNew
participant GatewayEVM
User->>ERC20CustodyNew: initiate withdrawal()
ERC20CustodyNew->>TSS: validate TSS permissions
TSS-->>ERC20CustodyNew: permission granted
ERC20CustodyNew->>GatewayEVM: execute withdrawal
GatewayEVM-->>User: withdrawal completed
Objective | Addressed | Explanation |
---|---|---|
Assess and document contract access control ( #197 ) | β | |
Update access control mechanisms for new contracts ( #197 ) | β | |
Ensure TSS address is utilized correctly ( #197 ) | β |
π "In the meadow, hops a bright hare,
With contracts changed, we dance in the air!
TSS now guards, with hops and bounds,
Secure transactions, where joy abounds.
With every withdrawal, a cheer will ring,
For safety and fun, let the blockchain sing!" π°
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?
Attention: Patch coverage is 64.28571%
with 5 lines
in your changes missing coverage. Please review.
Project coverage is 60.91%. Comparing base (
a2da596
) to head (77f2bf0
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@lumtis do we also need tssUpdater?
Can we start write a specs, like a table listing what actors can be calling what contracts? Similar to: https://www.notion.so/zetachain/Admin-Policy-Groups-4e501dd64dfc460180a361f9b8507310 We can do it here: https://www.notion.so/zetachain/Access-Control-1dd47015d1d8401ca5216f82922bcf87?pvs=4 This can be then shared to security team for review (the docs will be more complete later based on comment below)
I had in mind initially for the issue to port also the admin security mechanism of v1 into it. Example:
But I realized, this would be better to address in a further PR Create issue: #255
yes, will add after #255 is done
closes #197
Summary by CodeRabbit
New Features
tssAddress
parameter, allowing only authorized entities to perform critical withdrawal operations.Bug Fixes
Tests
Chores