feat: v2 contracts access control #247
Conversation
skosito
requested review from
andresaiello,
brewmaster012,
lumtis,
CharlieMc0,
fadeev and
fbac
as code owners
|
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 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 for further information. WalkthroughThe 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. Changes
Sequence Diagram(s)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
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -56,24 +72,24 @@ | |||
|
|
|||
| // Called by the TSS | |||
| // Calling onRevert directly | |||
| function executeRevert(address destination, bytes calldata data) public payable { | |||
| function executeRevert(address destination, bytes calldata data) public payable onlyTSS { | |||
Check notice
Code scanning / Slither
Missing zero address validation Low
|
|
||
| IGatewayEVM public immutable gateway; | ||
| address public immutable zetaToken; | ||
| address public tssAddress; |
Check warning
Code scanning / Slither
State variables that could be declared immutable Warning
|
|
||
| IGatewayEVM public gateway; | ||
| address public tssAddress; |
Check warning
Code scanning / Slither
State variables that could be declared immutable Warning
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #247 +/- ##
==========================================
- Coverage 61.08% 60.91% -0.17%
==========================================
Files 16 16
Lines 388 394 +6
Branches 104 103 -1
==========================================
+ Hits 237 240 +3
- Misses 151 154 +3 ☔ 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, codebase verification and nitpick comments (3)
contracts/prototypes/evm/GatewayEVM.sol (3)
Line range hint
75-81:
Address potential reentrancy vulnerabilities.The
executeRevertfunction may be vulnerable to reentrancy attacks. Consider using thenonReentrantmodifier.- function executeRevert(address destination, bytes calldata data) public payable onlyTSS { + function executeRevert(address destination, bytes calldata data) public payable onlyTSS nonReentrant {Tools
GitHub Check: Slither
[notice] 75-75: Missing zero address validation
GatewayEVM.executeRevert(address,bytes).destination (contracts/prototypes/evm/GatewayEVM.sol#75) lacks a zero-check on :
- (success,result) = destination.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVM.sol#76)
Line range hint
75-81:
Avoid low-level calls.Low-level calls using
callare error-prone and should be avoided if possible. Consider using a more secure method.- (bool success, bytes memory result) = destination.call{value: msg.value}(""); + (bool success, bytes memory result) = Address.functionCallWithValue(destination, "", msg.value);Tools
GitHub Check: Slither
[notice] 75-75: Missing zero address validation
GatewayEVM.executeRevert(address,bytes).destination (contracts/prototypes/evm/GatewayEVM.sol#75) lacks a zero-check on :
- (success,result) = destination.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVM.sol#76)
Line range hint
86-92:
Address potential reentrancy vulnerabilities.The
executefunction may be vulnerable to reentrancy attacks. Consider using thenonReentrantmodifier.- function execute(address destination, bytes calldata data) external payable onlyTSS returns (bytes memory) { + function execute(address destination, bytes calldata data) external payable onlyTSS nonReentrant returns (bytes memory) {
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (39)
pkg/contracts/prototypes/evm/erc20custodynew.sol/erc20custodynew.gois excluded by!pkg/**pkg/contracts/prototypes/evm/gatewayevm.sol/gatewayevm.gois excluded by!pkg/**pkg/contracts/prototypes/evm/gatewayevmupgradetest.sol/gatewayevmupgradetest.gois excluded by!pkg/**pkg/contracts/prototypes/evm/ierc20custodynew.sol/ierc20custodynewerrors.gois excluded by!pkg/**pkg/contracts/prototypes/evm/ierc20custodynew.sol/ierc20custodynewevents.gois excluded by!pkg/**pkg/contracts/prototypes/evm/igatewayevm.sol/igatewayevmerrors.gois excluded by!pkg/**pkg/contracts/prototypes/evm/izetaconnector.sol/izetaconnectorevents.gois excluded by!pkg/**pkg/contracts/prototypes/evm/zetaconnectornative.sol/zetaconnectornative.gois excluded by!pkg/**pkg/contracts/prototypes/evm/zetaconnectornewbase.sol/zetaconnectornewbase.gois excluded by!pkg/**pkg/contracts/prototypes/evm/zetaconnectornonnative.sol/zetaconnectornonnative.gois excluded by!pkg/**pkg/contracts/prototypes/zevm/gatewayzevm.sol/gatewayzevm.gois excluded by!pkg/**pkg/contracts/zevm/zrc20new.sol/zrc20new.gois excluded by!pkg/**typechain-types/contracts/prototypes/evm/ERC20CustodyNew.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/evm/IERC20CustodyNew.sol/IERC20CustodyNewErrors.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/evm/IERC20CustodyNew.sol/IERC20CustodyNewEvents.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/evm/IERC20CustodyNew.sol/index.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/evm/IZetaConnector.sol/IZetaConnectorEvents.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/evm/IZetaConnector.sol/index.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/evm/ZetaConnectorNative.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/evm/ZetaConnectorNewBase.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/evm/ZetaConnectorNonNative.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/evm/index.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/ERC20CustodyNew__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/GatewayEVMUpgradeTest__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/GatewayEVM__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/IERC20CustodyNew.sol/IERC20CustodyNewErrors__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/IERC20CustodyNew.sol/IERC20CustodyNewEvents__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/IERC20CustodyNew.sol/index.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/IGatewayEVM.sol/IGatewayEVMErrors__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/IZetaConnector.sol/IZetaConnectorEvents__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/IZetaConnector.sol/index.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/ZetaConnectorNative__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/ZetaConnectorNewBase__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/ZetaConnectorNonNative__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/index.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/zevm/GatewayZEVM__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/zevm/ZRC20New.sol/ZRC20New__factory.tsis excluded by!typechain-types/**typechain-types/hardhat.d.tsis excluded by!typechain-types/**typechain-types/index.tsis excluded by!typechain-types/**
Files selected for processing (17)
- contracts/prototypes/evm/ERC20CustodyNew.sol (2 hunks)
- contracts/prototypes/evm/GatewayEVM.sol (5 hunks)
- contracts/prototypes/evm/IERC20CustodyNew.sol (1 hunks)
- contracts/prototypes/evm/IGatewayEVM.sol (1 hunks)
- contracts/prototypes/evm/IZetaConnector.sol (1 hunks)
- contracts/prototypes/evm/ZetaConnectorNative.sol (3 hunks)
- contracts/prototypes/evm/ZetaConnectorNewBase.sol (1 hunks)
- contracts/prototypes/evm/ZetaConnectorNonNative.sol (2 hunks)
- contracts/prototypes/zevm/GatewayZEVM.sol (2 hunks)
- contracts/zevm/ZRC20New.sol (4 hunks)
- test/fuzz/GatewayEVMEchidnaTest.sol (1 hunks)
- test/prototypes/GatewayEVMUniswap.spec.ts (2 hunks)
- testFoundry/GatewayEVM.t.sol (17 hunks)
- testFoundry/GatewayEVMUpgrade.t.sol (1 hunks)
- testFoundry/GatewayEVMZEVM.t.sol (6 hunks)
- testFoundry/ZetaConnectorNative.t.sol (9 hunks)
- testFoundry/ZetaConnectorNonNative.t.sol (10 hunks)
Files skipped from review due to trivial changes (1)
- contracts/zevm/ZRC20New.sol
Additional context used
Path-based instructions (11)
contracts/prototypes/evm/IZetaConnector.sol (1)
Pattern
contracts/**: Review the Solidity contracts for security vulnerabilities and best practices.contracts/prototypes/evm/IERC20CustodyNew.sol (1)
Pattern
contracts/**: Review the Solidity contracts for security vulnerabilities and best practices.test/fuzz/GatewayEVMEchidnaTest.sol (1)
Pattern
test/**: Review the test files for proper coverage, edge cases, and best practices.contracts/prototypes/evm/IGatewayEVM.sol (1)
Pattern
contracts/**: Review the Solidity contracts for security vulnerabilities and best practices.contracts/prototypes/evm/ZetaConnectorNewBase.sol (1)
Pattern
contracts/**: Review the Solidity contracts for security vulnerabilities and best practices.contracts/prototypes/evm/ZetaConnectorNonNative.sol (1)
Pattern
contracts/**: Review the Solidity contracts for security vulnerabilities and best practices.contracts/prototypes/evm/ZetaConnectorNative.sol (1)
Pattern
contracts/**: Review the Solidity contracts for security vulnerabilities and best practices.contracts/prototypes/evm/ERC20CustodyNew.sol (1)
Pattern
contracts/**: Review the Solidity contracts for security vulnerabilities and best practices.test/prototypes/GatewayEVMUniswap.spec.ts (1)
Pattern
test/**: Review the test files for proper coverage, edge cases, and best practices.contracts/prototypes/zevm/GatewayZEVM.sol (1)
Pattern
contracts/**: Review the Solidity contracts for security vulnerabilities and best practices.contracts/prototypes/evm/GatewayEVM.sol (1)
Pattern
contracts/**: Review the Solidity contracts for security vulnerabilities and best practices.
GitHub Check: Slither
contracts/prototypes/evm/ZetaConnectorNewBase.sol
[warning] 19-19: State variables that could be declared immutable
ZetaConnectorNewBase.tssAddress (contracts/prototypes/evm/ZetaConnectorNewBase.sol#19) should be immutablecontracts/prototypes/evm/ERC20CustodyNew.sol
[warning] 18-18: State variables that could be declared immutable
ERC20CustodyNew.tssAddress (contracts/prototypes/evm/ERC20CustodyNew.sol#18) should be immutablecontracts/prototypes/evm/GatewayEVM.sol
[notice] 75-75: Missing zero address validation
GatewayEVM.executeRevert(address,bytes).destination (contracts/prototypes/evm/GatewayEVM.sol#75) lacks a zero-check on :
- (success,result) = destination.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVM.sol#76)
[notice] 75-81: Reentrancy vulnerabilities
Reentrancy in GatewayEVM.executeRevert(address,bytes) (contracts/prototypes/evm/GatewayEVM.sol#75-81):
External calls:
- (success,result) = destination.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVM.sol#76)
- Revertable(destination).onRevert(data) (contracts/prototypes/evm/GatewayEVM.sol#78)
External calls sending eth:
- (success,result) = destination.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVM.sol#76)
Event emitted after the call(s):
- Reverted(destination,msg.value,data) (contracts/prototypes/evm/GatewayEVM.sol#80)
[warning] 75-81: Low-level calls
Low level call in GatewayEVM.executeRevert(address,bytes) (contracts/prototypes/evm/GatewayEVM.sol#75-81):
- (success,result) = destination.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVM.sol#76)
[notice] 86-92: Reentrancy vulnerabilities
Reentrancy in GatewayEVM.execute(address,bytes) (contracts/prototypes/evm/GatewayEVM.sol#86-92):
External calls:
- result = _execute(destination,data) (contracts/prototypes/evm/GatewayEVM.sol#87)
- (success,result) = destination.call{value: msg.value}(data) (contracts/prototypes/evm/GatewayEVM.sol#67)
Event emitted after the call(s):
- Executed(destination,msg.value,data) (contracts/prototypes/evm/GatewayEVM.sol#89)
Additional comments not posted (67)
contracts/prototypes/evm/IZetaConnector.sol (3)
5-5: LGTM!The
Withdrawevent declaration follows Solidity best practices.
6-6: LGTM!The
WithdrawAndCallevent declaration follows Solidity best practices.
7-7: LGTM!The
WithdrawAndRevertevent declaration follows Solidity best practices.contracts/prototypes/evm/IERC20CustodyNew.sol (5)
5-5: LGTM!The
Withdrawevent declaration follows Solidity best practices.
6-6: LGTM!The
WithdrawAndCallevent declaration follows Solidity best practices.
7-7: LGTM!The
WithdrawAndRevertevent declaration follows Solidity best practices.
11-11: LGTM!The
ZeroAddresserror declaration follows Solidity best practices.
12-12: LGTM!The
InvalidSendererror declaration follows Solidity best practices.test/fuzz/GatewayEVMEchidnaTest.sol (2)
19-20: LGTM!The constructor changes enhance the initialization of the
ERC20CustodyNewcontract by passing thetssAddressparameter.
Line range hint
22-31:
LGTM!The
testExecuteWithERC20function follows best practices for testing ERC20 token transactions.contracts/prototypes/evm/IGatewayEVM.sol (1)
21-21: LGTM!The new error declaration
InvalidSenderis consistent with other error declarations in the interface.contracts/prototypes/evm/ZetaConnectorNewBase.sol (3)
15-15: LGTM!The new error declaration
InvalidSenderis consistent with other error declarations in the contract.
22-27: LGTM!The new modifier
onlyTSScorrectly restricts access to functions to the TSS address and reverts with an appropriate error if the sender is not the TSS address.
29-35: LGTM!The constructor correctly initializes the state variables and checks for zero addresses, ensuring secure and correct initialization.
contracts/prototypes/evm/ZetaConnectorNonNative.sol (4)
9-10: LGTM!The constructor correctly passes the parameters to the base contract, ensuring secure and correct initialization.
14-17: LGTM!The
withdrawfunction correctly restricts access to the TSS address and performs the withdrawal operation securely.
Line range hint
20-28:
LGTM!The
withdrawAndCallfunction correctly restricts access to the TSS address and performs the withdrawal and call operation securely.
Line range hint
31-39:
LGTM!The
withdrawAndRevertfunction correctly restricts access to the TSS address and performs the withdrawal and revert operation securely.contracts/prototypes/evm/ZetaConnectorNative.sol (4)
16-18: Verify the correctness of theonlyTSSmodifier.The
withdrawfunction now includes theonlyTSSmodifier. Ensure that the modifier correctly restricts access to the function.Verification successful
The
onlyTSSmodifier correctly restricts access to thewithdrawfunction.The modifier ensures that only the address stored in
tssAddresscan call the function, reverting withInvalidSenderif the check fails.
contracts/prototypes/evm/ZetaConnectorNewBase.solScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the `onlyTSS` modifier. # Test: Search for the `onlyTSS` modifier in the contract. Expect: Correct implementation of the modifier. rg --type solidity -A 5 $'modifier onlyTSS'Length of output: 77
Script:
#!/bin/bash # Description: Verify the implementation of the `onlyTSS` modifier. # Test: Search for the `onlyTSS` modifier in Solidity files. Expect: Correct implementation of the modifier. rg --glob '*.sol' -A 5 'modifier onlyTSS'Length of output: 1895
Line range hint
22-28:
Verify the correctness of theonlyTSSmodifier.The
withdrawAndCallfunction now includes theonlyTSSmodifier. Ensure that the modifier correctly restricts access to the function.Verification successful
The
onlyTSSmodifier is correctly implemented.The
onlyTSSmodifier inZetaConnectorNewBase.solensures that the function can only be called by the address stored intssAddress. This correctly restricts access to thewithdrawAndCallfunction inZetaConnectorNative.sol.
contracts/prototypes/evm/ZetaConnectorNewBase.sol:modifier onlyTSS() { if (msg.sender != tssAddress) { revert InvalidSender(); } _; }Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the `onlyTSS` modifier. # Test: Search for the `onlyTSS` modifier in the contract. Expect: Correct implementation of the modifier. rg --type solidity -A 5 $'modifier onlyTSS'Length of output: 77
Script:
#!/bin/bash # Description: Verify the implementation of the `onlyTSS` modifier. # Locate all Solidity files and search for the `onlyTSS` modifier within those files fd -e sol | xargs rg 'modifier onlyTSS'Length of output: 547
Script:
#!/bin/bash # Description: Verify the implementation of the `onlyTSS` modifier in ZetaConnectorNative.sol. # Search for the `onlyTSS` modifier definition within ZetaConnectorNative.sol rg 'modifier onlyTSS' contracts/prototypes/evm/ZetaConnectorNative.sol # If not found, check for inheritance from other contracts rg 'is ' contracts/prototypes/evm/ZetaConnectorNative.solLength of output: 555
Script:
#!/bin/bash # Description: Verify the implementation of the `onlyTSS` modifier in ZetaConnectorNewBase.sol. # Search for the `onlyTSS` modifier definition within ZetaConnectorNewBase.sol rg 'modifier onlyTSS' -A 5 contracts/prototypes/evm/ZetaConnectorNewBase.solLength of output: 206
Line range hint
33-39:
Verify the correctness of theonlyTSSmodifier.The
withdrawAndRevertfunction now includes theonlyTSSmodifier. Ensure that the modifier correctly restricts access to the function.Verification successful
Verification of the
onlyTSSModifier ImplementationThe
withdrawAndRevertfunction inZetaConnectorNative.solcorrectly includes theonlyTSSmodifier. TheZetaConnectorNativecontract inherits fromZetaConnectorNewBase, which defines theonlyTSSmodifier. Additionally, thetssAddressis properly set in the constructor ofZetaConnectorNative, ensuring that the modifier functions as intended.
contracts/prototypes/evm/ZetaConnectorNative.solinherits fromZetaConnectorNewBaseand uses theonlyTSSmodifier.- The
tssAddressis correctly set in the constructor.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the `onlyTSS` modifier. # Test: Search for the `onlyTSS` modifier in the contract. Expect: Correct implementation of the modifier. rg --type solidity -A 5 $'modifier onlyTSS'Length of output: 77
Script:
#!/bin/bash # Description: Verify the implementation of the `onlyTSS` modifier. # Test: Search for the `onlyTSS` modifier in the contract. Expect: Correct implementation of the modifier. rg --type sol -A 5 $'modifier onlyTSS'Length of output: 67
Script:
#!/bin/bash # Description: Verify the implementation of the `onlyTSS` modifier. # Test: Search for the `onlyTSS` modifier in the contract. Expect: Correct implementation of the modifier. rg 'modifier onlyTSS' -A 5 --glob '**/*.sol'Length of output: 1898
Script:
#!/bin/bash # Description: Verify the relationship between `ZetaConnectorNative.sol` and `ZetaConnectorNewBase.sol` and the setting of `tssAddress`. # Check if `ZetaConnectorNative.sol` inherits from `ZetaConnectorNewBase.sol` or uses the `onlyTSS` modifier. rg 'ZetaConnectorNewBase' contracts/prototypes/evm/ZetaConnectorNative.sol rg 'onlyTSS' contracts/prototypes/evm/ZetaConnectorNative.sol # Verify the setting of `tssAddress` in `ZetaConnectorNative.sol`. rg 'tssAddress' contracts/prototypes/evm/ZetaConnectorNative.solLength of output: 905
11-12: Verify the correctness of the constructor parameter handling.The constructor now accepts an additional parameter
_tssAddressand passes it to the parent constructor. Ensure that_tssAddressis correctly handled and passed.Verification successful
Verified the correctness of the constructor parameter handling.
The constructor in
ZetaConnectorNativecorrectly passes the_tssAddressparameter to the parent constructor inZetaConnectorNewBase, which appropriately handles it.
ZetaConnectorNewBaseconstructor:
- Checks for zero addresses.
- Assigns
_tssAddresstotssAddress.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the constructor parameter handling in the parent contract. # Test: Search for the constructor in the parent contract. Expect: Correct handling of the additional parameter. rg --type solidity -A 5 $'constructor(address _gateway, address _zetaToken, address _tssAddress)'Length of output: 131
Script:
#!/bin/bash # Description: Verify the constructor parameter handling in the parent contract. # Find all Solidity files and search for the constructor in the parent contract. fd -e sol | xargs rg 'constructor(address _gateway, address _zetaToken, address _tssAddress)'Length of output: 95
Script:
#!/bin/bash # Description: Locate the parent contract and inspect its constructor for the additional parameter. # Find the Solidity file containing the parent contract definition. fd -e sol | xargs rg -l 'contract ZetaConnectorNewBase' | xargs rg -A 10 'constructor(address _gateway, address _zetaToken, address _tssAddress)'Length of output: 147
Script:
#!/bin/bash # Description: Locate the parent contract and inspect its constructors. # Find the Solidity file containing the parent contract definition. fd -e sol | xargs rg -l 'contract ZetaConnectorNewBase' | xargs rg -A 10 'constructor('Length of output: 164
Script:
#!/bin/bash # Description: Locate the parent contract and inspect its constructors. # Find the Solidity file containing the parent contract definition. fd -e sol | xargs rg -l 'contract ZetaConnectorNewBase' | xargs rg -A 10 'constructor\('Length of output: 515
contracts/prototypes/evm/ERC20CustodyNew.sol (4)
37-40: Verify the correctness of theonlyTSSmodifier.The
withdrawfunction now includes theonlyTSSmodifier. Ensure that the modifier correctly restricts access to the function.Verification successful
Let's correct the file type and re-run the search for the
onlyTSSmodifier.
The
onlyTSSmodifier is correctly implemented.The
onlyTSSmodifier in thewithdrawfunction correctly restricts access by ensuring that only the TSS address can call the function. The modifier checks ifmsg.senderis equal totssAddressand reverts withInvalidSenderif the check fails.
contracts/prototypes/evm/ERC20CustodyNew.sol:modifier onlyTSS() { if (msg.sender != tssAddress) { revert InvalidSender(); } _; }Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the `onlyTSS` modifier. # Test: Search for the `onlyTSS` modifier in the contract. Expect: Correct implementation of the modifier. rg --type solidity -A 5 $'modifier onlyTSS'Length of output: 77
Script:
#!/bin/bash # Description: Verify the implementation of the `onlyTSS` modifier. # Test: Search for the `onlyTSS` modifier in Solidity files. Expect: Correct implementation of the modifier. rg --type-add 'solidity:*.sol' --type solidity -A 5 'modifier onlyTSS'Length of output: 1924
28-33: Verify the correctness of the constructor parameter handling and validation.The constructor now accepts an additional parameter
_tssAddressand validates it to ensure it is not a zero address. Ensure that_tssAddressis correctly handled and validated.Verification successful
The constructor parameter handling and validation are correct.
The constructor in
ERC20CustodyNew.solcorrectly handles the_tssAddressparameter by validating it to ensure it is not a zero address.
ERC20CustodyNew.sol: Lines 28-33Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the constructor parameter handling and validation. # Test: Search for the constructor in the contract. Expect: Correct handling and validation of the additional parameter. rg --type solidity -A 5 $'constructor(address _gateway, address _tssAddress)'Length of output: 111
Script:
#!/bin/bash # Description: Verify the constructor parameter handling and validation. # Find all Solidity files and search for the constructor within those files fd -e sol -x rg 'constructor(address _gateway, address _tssAddress)' -A 5 {}Length of output: 80
Script:
#!/bin/bash # Description: Verify the constructor parameter handling and validation in ERC20CustodyNew.sol # Locate the ERC20CustodyNew.sol file and search for the constructor within it fd 'ERC20CustodyNew.sol' -x rg 'constructor(address _gateway, address _tssAddress)' -A 5 {}Length of output: 93
Script:
#!/bin/bash # Description: Verify the existence of ERC20CustodyNew.sol and list its contents # Locate the ERC20CustodyNew.sol file and list its contents fd 'ERC20CustodyNew.sol' -x cat {}Length of output: 3450
Line range hint
57-63:
Verify the correctness of theonlyTSSmodifier.The
withdrawAndRevertfunction now includes theonlyTSSmodifier. Ensure that the modifier correctly restricts access to the function.Verification successful
Verify the correctness of the
onlyTSSmodifier.The
onlyTSSmodifier correctly restricts access to thewithdrawAndRevertfunction by ensuring that only the TSS address can call it.
- The
onlyTSSmodifier checks ifmsg.senderis equal totssAddressand reverts withInvalidSenderif not.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the `onlyTSS` modifier. # Test: Search for the `onlyTSS` modifier in the contract. Expect: Correct implementation of the modifier. rg --type solidity -A 5 $'modifier onlyTSS'Length of output: 77
Script:
#!/bin/bash # Description: Verify the implementation of the `onlyTSS` modifier. # Locate all Solidity files and search for the `onlyTSS` modifier within them. fd -e sol -x rg 'modifier onlyTSS' {}Length of output: 354
Script:
#!/bin/bash # Description: Display the implementation of the `onlyTSS` modifier in Solidity files. # Locate all Solidity files and display the implementation of the `onlyTSS` modifier. fd -e sol -x rg 'modifier onlyTSS' -A 5 {}Length of output: 1654
Line range hint
45-51:
Verify the correctness of theonlyTSSmodifier.The
withdrawAndCallfunction now includes theonlyTSSmodifier. Ensure that the modifier correctly restricts access to the function.Verification successful
The
onlyTSSmodifier is correctly implemented.The
onlyTSSmodifier ensures that only thetssAddresscan call functions protected by this modifier by checking ifmsg.senderequalstssAddressand reverting the transaction if it does not.
contracts/prototypes/evm/ERC20CustodyNew.sol:modifier onlyTSS() { if (msg.sender != tssAddress) { revert InvalidSender(); } _; }Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the `onlyTSS` modifier. # Test: Search for the `onlyTSS` modifier in the contract. Expect: Correct implementation of the modifier. rg --type solidity -A 5 $'modifier onlyTSS'Length of output: 77
Script:
#!/bin/bash # Description: Verify the implementation of the `onlyTSS` modifier. # Find all Solidity files and search for the `onlyTSS` modifier. fd -e sol -x rg 'modifier onlyTSS' {}Length of output: 354
Script:
#!/bin/bash # Description: Verify the implementation of the `onlyTSS` modifier. # Search for the full implementation of the `onlyTSS` modifier in Solidity files. rg 'modifier onlyTSS' -A 10 --type-add 'solidity:*.sol'Length of output: 5356
Tools
GitHub Check: Slither
[warning] 18-18: State variables that could be declared immutable
ERC20CustodyNew.tssAddress (contracts/prototypes/evm/ERC20CustodyNew.sol#18) should be immutabletestFoundry/GatewayEVMUpgrade.t.sol (2)
49-49: Verify the correctness of the constructor call forERC20CustodyNew.The constructor call for
ERC20CustodyNewnow includes an additional parametertssAddress. Ensure that the parameter is correctly handled and passed to the constructor.
50-50: Verify the correctness of the constructor call forZetaConnectorNonNative.The constructor call for
ZetaConnectorNonNativenow includes an additional parametertssAddress. Ensure that the parameter is correctly handled and passed to the constructor.Verification successful
Verified the correctness of the constructor call for
ZetaConnectorNonNative.The constructor call in
testFoundry/GatewayEVMUpgrade.t.solcorrectly matches the constructor definition in theZetaConnectorNonNativecontract, including the additional parametertssAddress.
contracts/prototypes/evm/ZetaConnectorNonNative.sol: Constructor definition matches the call.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the constructor call for `ZetaConnectorNonNative`. # Test: Search for the constructor call in the test file. Expect: Correct handling and passing of the additional parameter. rg --type solidity -A 5 $'ZetaConnectorNonNative(address gateway, address zeta, address tssAddress)'Length of output: 134
Script:
#!/bin/bash # Description: Verify the constructor call for `ZetaConnectorNonNative`. # Find all Solidity files and search for the constructor pattern within them. rg --glob '*.sol' -A 5 'ZetaConnectorNonNative(address gateway, address zeta, address tssAddress)'Length of output: 100
Script:
#!/bin/bash # Description: Verify the constructor call for `ZetaConnectorNonNative`. # Find the class definition for `ZetaConnectorNonNative` and inspect its constructor. rg --glob '*.sol' -A 10 'contract ZetaConnectorNonNative'Length of output: 1981
test/prototypes/GatewayEVMUniswap.spec.ts (3)
90-90: EnsurewithdrawAndCallis correctly invoked bytssAddress.The change connects
custodytotssAddressfor invokingwithdrawAndCall. This ensures that the transaction is simulated as initiated by the trusted signer. Verify that this aligns with the intended access control mechanisms.Verification successful
Ensure
withdrawAndCallis correctly invoked bytssAddress.The
withdrawAndCallmethod incontracts/prototypes/evm/ERC20CustodyNew.solis correctly defined and the usage oftssAddressin the test file aligns with the intended access control mechanisms. The transaction is accurately simulated as initiated by the trusted signer.
contracts/prototypes/evm/ERC20CustodyNew.sol: Correct implementation ofwithdrawAndCallmethod.test/prototypes/GatewayEVMUniswap.spec.ts: Proper usage oftssAddressto invokewithdrawAndCall.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `withdrawAndCall` method's implementation and usage of `tssAddress`. # Test: Search for the `withdrawAndCall` method definition and usage of `tssAddress`. Expect: Correct implementation and usage. rg --type solidity -A 5 $'function withdrawAndCall' contracts/prototypes/evm/ERC20CustodyNew.solLength of output: 130
Script:
#!/bin/bash # Description: Verify the `withdrawAndCall` method's implementation and usage of `tssAddress`. # Test: Search for the `withdrawAndCall` method definition and usage of `tssAddress`. Expect: Correct implementation and usage. rg 'function withdrawAndCall' -A 5 rg 'tssAddress'Length of output: 172893
70-70: Ensure correct initialization ofZetaConnectorwithtssAddress.The addition of
tssAddressas a parameter ensures that the connector contract is aware of the trusted signer. Make sure that this address is correctly utilized within the contract.Verification successful
Initialization and usage of
tssAddressare correctly implemented.
- The
ZetaConnectorcontract's constructor correctly initializestssAddress.- The
onlyTSSmodifier ensures that functions are restricted to thetssAddress.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `ZetaConnector` contract's constructor and usage of `tssAddress`. # Test: Search for the constructor definition and usage of `tssAddress`. Expect: Correct initialization and usage. rg --type solidity -A 5 $'constructor(address gateway, address zeta, address tssAddress)' contracts/prototypes/evm/ZetaConnectorNonNative.solLength of output: 175
Script:
#!/bin/bash # Description: Verify the `ZetaConnector` contract's constructor and usage of `tssAddress`. # Test: Search for the constructor definition and usage of `tssAddress`. Expect: Correct initialization and usage. rg 'constructor(address gateway, address zeta, address tssAddress)' -A 10 contracts/prototypes/evm/ZetaConnectorNonNative.sol # Test: Search for the usage of `tssAddress` within the contract. Expect: Correct usage. rg 'tssAddress' -A 5 contracts/prototypes/evm/ZetaConnectorNonNative.solLength of output: 627
Script:
#!/bin/bash # Description: Verify the usage of `tssAddress` in the `ZetaConnector` contract and its base contract. # Test: Search for the definition of `onlyTSS` modifier. Expect: Correct implementation using `tssAddress`. rg 'modifier onlyTSS' -A 5 contracts/prototypes/evm/ZetaConnectorNonNative.sol # Test: Search for the usage of `_tssAddress` in the base contract `ZetaConnectorNewBase`. Expect: Correct usage. rg '_tssAddress' -A 5 contracts/prototypes/evm/ZetaConnectorNewBase.solLength of output: 699
Script:
#!/bin/bash # Description: Verify the implementation of `onlyTSS` modifier in the entire codebase. # Test: Search for the definition of `onlyTSS` modifier. Expect: Correct implementation using `tssAddress`. rg 'modifier onlyTSS' -A 5Length of output: 2806
68-68: Ensure correct initialization ofERC20CustodyNewwithtssAddress.The addition of
tssAddressas a parameter ensures that the custody contract is aware of the trusted signer. Make sure that this address is correctly utilized within the contract.Verification successful
Ensure correct initialization of
ERC20CustodyNewwithtssAddress.The
ERC20CustodyNewcontract's constructor correctly initializes thetssAddressparameter, ensuring that the custody contract is aware of the trusted signer.
contracts/prototypes/evm/ERC20CustodyNew.sol:constructor(address _gateway, address _tssAddress) { if (_gateway == address(0) || _tssAddress == address(0)) { revert ZeroAddress(); } gateway = IGatewayEVM(_gateway); tssAddress = _tssAddress; }The constructor checks for zero addresses and assigns the
tssAddressparameter correctly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `ERC20CustodyNew` contract's constructor and usage of `tssAddress`. # Test: Search for the constructor definition and usage of `tssAddress`. Expect: Correct initialization and usage. rg --type solidity -A 5 $'constructor(address gateway, address tssAddress)' contracts/prototypes/evm/ERC20CustodyNew.solLength of output: 154
Script:
#!/bin/bash # Description: Verify the `ERC20CustodyNew` contract's constructor and usage of `tssAddress`. # Test: Search for the constructor definition and usage of `tssAddress`. Expect: Correct initialization and usage. ast-grep --lang solidity --pattern $'constructor(address gateway, address tssAddress) { $$$ }' contracts/prototypes/evm/ERC20CustodyNew.solLength of output: 257
Script:
#!/bin/bash # Description: Verify the `ERC20CustodyNew` contract's constructor and usage of `tssAddress`. # Test: Search for the constructor definition and usage of `tssAddress`. Expect: Correct initialization and usage. rg 'constructor(address gateway, address tssAddress)' -A 10 contracts/prototypes/evm/ERC20CustodyNew.solLength of output: 106
Script:
#!/bin/bash # Description: Verify the `ERC20CustodyNew` contract's constructor and usage of `tssAddress`. # Test: Search for all constructors in Solidity files to ensure correct initialization and usage of `tssAddress`. rg 'constructor' -A 10 contracts/prototypes/evm/Length of output: 6475
contracts/prototypes/zevm/GatewayZEVM.sol (5)
122-124: Ensureexecutefunction correctly restricts access and handles execution securely.The
executefunction now uses theonlyFungiblemodifier. Verify that the function correctly restricts access and securely handles execution.Verification successful
The
executefunction correctly restricts access and handles execution securely.
- The
onlyFungiblemodifier ensures that only the fungible module can call theexecutefunction by checking ifmsg.senderis equal toFUNGIBLE_MODULE_ADDRESS.- The
executefunction securely handles the cross-chain call by invokingUniversalContract(target).onCrossChainCall(context, zrc20, amount, message).Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `execute` function's implementation and usage of `onlyFungible`. # Test: Search for the `execute` function definition and usage of `onlyFungible`. Expect: Correct implementation and usage. rg --type solidity -A 5 $'function execute' contracts/prototypes/zevm/GatewayZEVM.solLength of output: 119
Script:
#!/bin/bash # Description: Verify the `execute` function's implementation and usage of `onlyFungible`. # Test: Search for the `execute` function definition. rg -A 5 'function execute' contracts/prototypes/zevm/GatewayZEVM.sol # Test: Search for the `onlyFungible` modifier definition. rg -A 5 'modifier onlyFungible' contracts/prototypes/zevm/GatewayZEVM.solLength of output: 636
22-28: EnsureonlyFungiblemodifier correctly restricts access.The
onlyFungiblemodifier restricts access to theFUNGIBLE_MODULE_ADDRESS. Ensure that this address is correctly set and that the modifier is securely implemented.Verification successful
Ensure
onlyFungiblemodifier correctly restricts access.The
onlyFungiblemodifier is correctly implemented and used to restrict access to functions that should only be accessible by theFUNGIBLE_MODULE_ADDRESS. The implementation and usage are consistent and secure.
contracts/prototypes/zevm/GatewayZEVM.solcontracts/zevm/ZRC20New.solcontracts/zevm/ZetaConnectorZEVM.solScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `onlyFungible` modifier's implementation and usage. # Test: Search for the `onlyFungible` modifier definition and usage. Expect: Correct implementation and usage. rg --type solidity -A 5 $'modifier onlyFungible' contracts/prototypes/zevm/GatewayZEVM.solLength of output: 124
Script:
#!/bin/bash # Description: Verify the `onlyFungible` modifier's implementation and usage. # Test: Search for the `onlyFungible` modifier definition and usage. Expect: Correct implementation and usage. rg 'modifier onlyFungible' -A 5 contracts/prototypes/zevm/GatewayZEVM.solLength of output: 233
Script:
#!/bin/bash # Description: Verify the usage of the `onlyFungible` modifier across the codebase. # Test: Search for the usage of the `onlyFungible` modifier. Expect: Correct usage. rg 'onlyFungible' -A 3Length of output: 9131
133-135: EnsuredepositAndCallfunction correctly restricts access and handles deposits and calls securely.The
depositAndCallfunction now uses theonlyFungiblemodifier. Verify that the function correctly restricts access and securely handles deposits and calls.Verification successful
Ensure
depositAndCallfunction correctly restricts access and handles deposits and calls securely.The
depositAndCallfunctions correctly use theonlyFungiblemodifier to restrict access. They securely handle deposits and calls by verifying the target, transferring tokens, and invoking theonCrossChainCallmethod on the target contract.
- Lines 133-135 in
contracts/prototypes/zevm/GatewayZEVM.solScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `depositAndCall` function's implementation and usage of `onlyFungible`. # Test: Search for the `depositAndCall` function definition and usage of `onlyFungible`. Expect: Correct implementation and usage. rg --type solidity -A 5 $'function depositAndCall' contracts/prototypes/zevm/GatewayZEVM.solLength of output: 126
Script:
#!/bin/bash # Description: Verify the `depositAndCall` function's implementation and usage of `onlyFungible`. # Test: Search for the `depositAndCall` function definition and usage of `onlyFungible`. Expect: Correct implementation and usage. rg --type-add 'solidity:*.sol' --type solidity -A 5 'function depositAndCall' contracts/prototypes/zevm/GatewayZEVM.solLength of output: 463
Script:
#!/bin/bash # Description: Verify the complete implementation of the `depositAndCall` function. # Test: Search for the complete implementation of the `depositAndCall` function to ensure it correctly restricts access and handles deposits and calls securely. rg --type-add 'solidity:*.sol' --type solidity -A 20 'function depositAndCall' contracts/prototypes/zevm/GatewayZEVM.solLength of output: 1347
160-162: EnsureexecuteRevertfunction correctly restricts access and handles reverts securely.The
executeRevertfunction now uses theonlyFungiblemodifier. Verify that the function correctly restricts access and securely handles reverts.Verification successful
Ensure
executeRevertfunction correctly restricts access and handles reverts securely.The
executeRevertfunction uses theonlyFungiblemodifier to restrict access. TheonlyFungiblemodifier is correctly implemented to ensure that only the address specified byFUNGIBLE_MODULE_ADDRESScan call the function.
contracts/prototypes/zevm/GatewayZEVM.sol:
executeRevertfunctiononlyFungiblemodifierScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `executeRevert` function's implementation and usage of `onlyFungible`. # Test: Search for the `executeRevert` function definition and usage of `onlyFungible`. Expect: Correct implementation and usage. rg --type solidity -A 5 $'function executeRevert' contracts/prototypes/zevm/GatewayZEVM.solLength of output: 125
Script:
#!/bin/bash # Description: Verify the `executeRevert` function's implementation and usage of `onlyFungible`. # Test: Search for the `executeRevert` function definition and usage of `onlyFungible`. Expect: Correct implementation and usage. rg --type sol -A 5 'function executeRevert' contracts/prototypes/zevm/GatewayZEVM.solLength of output: 115
Script:
#!/bin/bash # Description: Verify the `executeRevert` function's implementation and usage of `onlyFungible`. # Step 1: Find all Solidity files. fd -e sol > sol_files.txt # Step 2: Search for the `executeRevert` function definition and usage of `onlyFungible` in the found files. while read -r file; do echo "Searching in $file" rg -A 5 'function executeRevert' "$file" done < sol_files.txtLength of output: 84812
Script:
#!/bin/bash # Description: Verify the implementation of the `onlyFungible` modifier. # Step 1: Find all Solidity files. fd -e sol > sol_files.txt # Step 2: Search for the `onlyFungible` modifier definition in the found files. while read -r file; do echo "Searching in $file" rg -A 5 'modifier onlyFungible' "$file" done < sol_files.txtLength of output: 77483
109-111: Ensuredepositfunction correctly restricts access and handles deposits securely.The
depositfunction now uses theonlyFungiblemodifier. Verify that the function correctly restricts access and securely handles deposits.Verification successful
The
depositfunction correctly restricts access and handles deposits securely.The function uses the
onlyFungiblemodifier to ensure only theFUNGIBLE_MODULE_ADDRESScan call it and includes checks to prevent depositing to invalid targets.
contracts/prototypes/zevm/GatewayZEVM.sol:
depositfunction implementation is secure and correctly uses theonlyFungiblemodifier.- The function includes necessary checks for invalid targets.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `deposit` function's implementation and usage of `onlyFungible`. # Test: Search for the `deposit` function definition and usage of `onlyFungible`. Expect: Correct implementation and usage. rg --type solidity -A 5 $'function deposit' contracts/prototypes/zevm/GatewayZEVM.solLength of output: 119
Script:
#!/bin/bash # Description: Verify the `deposit` function's implementation and usage of `onlyFungible`. # Find all Solidity files and search for the `deposit` function definition within them. fd -e sol | xargs rg -A 5 'function deposit'Length of output: 76439
Script:
#!/bin/bash # Description: Verify the `onlyFungible` modifier's implementation and review the `deposit` function's logic. # Find the implementation of the `onlyFungible` modifier. fd -e sol | xargs rg -A 5 'modifier onlyFungible' # Review the full implementation of the `deposit` function in `contracts/prototypes/zevm/GatewayZEVM.sol`. rg --type solidity -A 20 'function deposit' contracts/prototypes/zevm/GatewayZEVM.solLength of output: 2524
Script:
#!/bin/bash # Description: Review the full implementation of the `deposit` function in `contracts/prototypes/zevm/GatewayZEVM.sol`. # Search for the `deposit` function definition and display the next 40 lines to capture the full implementation. rg -A 40 'function deposit' contracts/prototypes/zevm/GatewayZEVM.solLength of output: 2504
testFoundry/GatewayEVMZEVM.t.sol (6)
116-116: Ensureexecuteis correctly invoked bytssAddress.The change uses
vm.prank(tssAddress)to simulate the transaction as initiated by the trusted signer. Verify that this aligns with the intended access control mechanisms.Verification successful
Ensure
executeis correctly invoked bytssAddress.The change uses
vm.prank(tssAddress)to simulate the transaction as initiated by the trusted signer. This aligns with theonlyTSSmodifier on theexecutemethod, ensuring correct access control.
contracts/prototypes/evm/GatewayEVM.sol: Theexecutemethod is restricted by theonlyTSSmodifier.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `execute` method's implementation and usage of `tssAddress`. # Test: Search for the `execute` method definition and usage of `tssAddress`. Expect: Correct implementation and usage. rg --type solidity -A 5 $'function execute' contracts/prototypes/evm/GatewayEVM.solLength of output: 117
Script:
#!/bin/bash # Description: Verify the `execute` method's implementation and usage of `tssAddress`. # Test: Search for the `execute` method definition and usage of `tssAddress`. Expect: Correct implementation and usage. rg -A 5 'function execute' --glob '*.sol'Length of output: 5291
203-203: Ensureexecuteis correctly invoked bytssAddress.The change uses
vm.prank(tssAddress)to simulate the transaction as initiated by the trusted signer. Verify that this aligns with the intended access control mechanisms.Verification successful
Ensure
executeis correctly invoked bytssAddress.The change uses
vm.prank(tssAddress)to simulate the transaction as initiated by the trusted signer. This aligns with the access control mechanism enforced by theonlyTSSmodifier in theexecutefunction's implementation.
contracts/prototypes/evm/GatewayEVM.sol:function execute(address destination, bytes calldata data) external payable onlyTSS returns (bytes memory)Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `execute` method's implementation and usage of `tssAddress`. # Test: Search for the `execute` method definition and usage of `tssAddress`. Expect: Correct implementation and usage. rg --type solidity -A 5 $'function execute' contracts/prototypes/evm/GatewayEVM.solLength of output: 117
Script:
#!/bin/bash # Description: Verify the `execute` method's implementation and usage of `tssAddress`. # Find Solidity files and search for the `execute` method definition and usage of `tssAddress`. fd -e sol | xargs rg -A 5 'function execute'Length of output: 33375
65-65: Ensure correct initialization ofERC20CustodyNewwithtssAddress.The addition of
tssAddressas a parameter ensures that the custody contract is aware of the trusted signer. Make sure that this address is correctly utilized within the contract.Verification successful
Ensure correct initialization of
ERC20CustodyNewwithtssAddress.The
tssAddressparameter is correctly utilized within theERC20CustodyNewcontract. It is checked for validity and assigned to a state variable, ensuring proper initialization.
ERC20CustodyNewconstructor correctly initializestssAddress.- Usage of
_tssAddressis verified to be correct.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `ERC20CustodyNew` contract's constructor and usage of `tssAddress`. # Test: Search for the constructor definition and usage of `tssAddress`. Expect: Correct initialization and usage. rg --type solidity -A 5 $'constructor(address gateway, address tssAddress)' contracts/prototypes/evm/ERC20CustodyNew.solLength of output: 154
Script:
#!/bin/bash # Description: Verify the `ERC20CustodyNew` contract's constructor and usage of `tssAddress`. # Test: Search for the constructor definition and usage of `tssAddress`. Expect: Correct initialization and usage. rg -A 5 'constructor(address gateway, address tssAddress)' contracts/prototypes/evm/ERC20CustodyNew.solLength of output: 105
Script:
#!/bin/bash # Description: Verify the `ERC20CustodyNew` contract's constructor definitions. # Test: Search for any constructor definitions. Expect: Identification of constructors. rg 'constructor' contracts/prototypes/evm/ERC20CustodyNew.solLength of output: 118
Script:
#!/bin/bash # Description: Verify the usage of `_tssAddress` in the `ERC20CustodyNew` contract. # Test: Search for the usage of `_tssAddress`. Expect: Correct utilization within the contract. rg '_tssAddress' contracts/prototypes/evm/ERC20CustodyNew.solLength of output: 220
179-179: Ensureexecuteis correctly invoked bytssAddress.The change uses
vm.prank(tssAddress)to simulate the transaction as initiated by the trusted signer. Verify that this aligns with the intended access control mechanisms.Verification successful
Ensure
executeis correctly invoked bytssAddress.The
executemethod incontracts/prototypes/evm/GatewayEVM.soluses theonlyTSSmodifier, ensuring it can only be called by the TSS address. The use ofvm.prank(tssAddress)in the test correctly simulates this behavior.
contracts/prototypes/evm/GatewayEVM.sol:executemethod definition confirms the use ofonlyTSSmodifier.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `execute` method's implementation and usage of `tssAddress`. # Test: Search for the `execute` method definition and usage of `tssAddress`. Expect: Correct implementation and usage. rg --type solidity -A 5 $'function execute' contracts/prototypes/evm/GatewayEVM.solLength of output: 117
Script:
#!/bin/bash # Description: Verify the `execute` method's implementation and usage of `tssAddress`. # Step 1: Find all Solidity files. sol_files=$(fd . --extension sol) # Step 2: Search for the `execute` method definition and usage of `tssAddress` in the found Solidity files. for file in $sol_files; do echo "Searching in $file" rg --type-add 'solidity:*.sol' --type solidity -A 5 'function execute' "$file" doneLength of output: 110002
139-139: Ensureexecuteis correctly invoked bytssAddress.The change uses
vm.prank(tssAddress)to simulate the transaction as initiated by the trusted signer. Verify that this aligns with the intended access control mechanisms.Verification successful
Ensure
executeis correctly invoked bytssAddress.The use of
vm.prank(tssAddress)to simulate the transaction as initiated by the trusted signer (tssAddress) aligns with the intended access control mechanisms. This ensures that only the trusted signer can initiate theexecutemethod, maintaining the integrity of the access control.
- The
executemethod is correctly defined and used in the context oftssAddress.- The
tssAddressis properly initialized and utilized within the contract.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `execute` method's implementation and usage of `tssAddress`. # Test: Search for the `execute` method definition and usage of `tssAddress`. Expect: Correct implementation and usage. rg --type solidity -A 5 $'function execute' contracts/prototypes/evm/GatewayEVM.solLength of output: 117
Script:
#!/bin/bash # Description: Verify the `execute` method's implementation and usage of `tssAddress`. # Test: Search for the `execute` method definition in Solidity files. rg -t sol -A 5 'function execute' # Test: Search for the usage of `tssAddress` in Solidity files. rg -t sol 'tssAddress'Length of output: 114
Script:
#!/bin/bash # Description: Verify the `execute` method's implementation and usage of `tssAddress`. # Find all Solidity files and search for the `execute` method definition. fd -e sol -x rg -A 5 'function execute' {} # Find all Solidity files and search for the usage of `tssAddress`. fd -e sol -x rg 'tssAddress' {}Length of output: 146649
66-66: Ensure correct initialization ofZetaConnectorwithtssAddress.The addition of
tssAddressas a parameter ensures that the connector contract is aware of the trusted signer. Make sure that this address is correctly utilized within the contract.contracts/prototypes/evm/GatewayEVM.sol (4)
30-36: LGTM! TheonlyTSSmodifier enhances security.The
onlyTSSmodifier effectively restricts function execution to the TSS address, improving access control.
38-44: LGTM! TheonlyCustodyOrConnectormodifier enhances security.The
onlyCustodyOrConnectormodifier effectively restricts function execution to the custody address or the zeta connector, improving access control.
Line range hint
103-122:
LGTM! TheonlyCustodyOrConnectormodifier enhances security.The
onlyCustodyOrConnectormodifier effectively restricts function execution to the custody address or the zeta connector, improving access control.
Line range hint
130-136:
LGTM! TheonlyCustodyOrConnectormodifier enhances security.The
onlyCustodyOrConnectormodifier effectively restricts function execution to the custody address or the zeta connector, improving access control.testFoundry/ZetaConnectorNative.t.sol (7)
15-16: LGTM! TheUpgradesimport is necessary for upgrade functionality.The import from
openzeppelin-foundry-upgradesis appropriate for managing contract upgrades.
19-19: LGTM! TheIZetaConnectorimport is necessary for interface implementation.The import is appropriate for implementing the
IZetaConnectorinterface.
21-21: LGTM! Implementing multiple interfaces enhances event handling capabilities.The implementation of
IGatewayEVMErrors,IGatewayEVMEvents,IReceiverEVMEvents, andIZetaConnectorEventsinterfaces improves the contract's event handling.
46-47: LGTM! Initialization ofcustodyandzetaConnectoris necessary for the test setup.The initialization of
custodyandzetaConnectorwith new contract instances is appropriate for the test setup.
56-56: LGTM! Thevm.dealfunction is necessary for testing.The use of
vm.dealto assign 1 Ether to thetssAddressis appropriate for testing purposes.
75-82: LGTM! ThetestWithdrawFailsIfSenderIsNotTSSfunction enhances security.The test function ensures that the
withdrawfunction reverts if called by an address other than the TSS address, improving security.
118-126: LGTM! ThetestWithdrawAndCallReceiveERC20FailsIfSenderIsNotTSSfunction enhances security.The test function ensures that the
withdrawAndCallfunction reverts if called by an address other than the TSS address, improving security.testFoundry/ZetaConnectorNonNative.t.sol (6)
15-16: LGTM! TheUpgradesimport is necessary for upgrade functionality.The import from
openzeppelin-foundry-upgradesis appropriate for managing contract upgrades.
17-17: LGTM! TheZeta.non-ethimport is necessary for non-ETH Zeta token functionality.The import is appropriate for implementing non-ETH Zeta token functionality.
22-22: LGTM! Implementing multiple interfaces enhances event handling capabilities.The implementation of
IGatewayEVMErrors,IGatewayEVMEvents,IReceiverEVMEvents, andIZetaConnectorEventsinterfaces improves the contract's event handling.
47-48: LGTM! Initialization ofcustodyandzetaConnectoris necessary for the test setup.The initialization of
custodyandzetaConnectorwith new contract instances is appropriate for the test setup.
58-58: LGTM! Thevm.dealfunction is necessary for testing.The use of
vm.dealto assign 1 Ether to thetssAddressis appropriate for testing purposes.
77-84: LGTM! ThetestWithdrawFailsIfSenderIsNotTSSfunction enhances security.The test function ensures that the
withdrawfunction reverts if called by an address other than the TSS address, improving security.testFoundry/GatewayEVM.t.sol (8)
Line range hint
21-58:
LGTM!The visibility change to
publicis appropriate for asetUpfunction in testing. The initialization logic appears correct.
79-88: LGTM!The visibility change to
publicis appropriate for a test function. The test logic correctly verifies that theexecutefunction fails if the sender is nottssAddress.
124-131: LGTM!The visibility change to
publicis appropriate for a test function. The test logic correctly verifies that theexecuteWithERC20function fails if the sender is notcustodyorzetaConnector.
133-140: LGTM!The visibility change to
publicis appropriate for a test function. The test logic correctly verifies that therevertWithERC20function fails if the sender is notcustodyorzetaConnector.
303-309: LGTM!The visibility change to
publicis appropriate for a test function. The test logic correctly verifies that thewithdrawfunction fails if the sender is nottssAddress.
347-353: LGTM!The visibility change to
publicis appropriate for a test function. The test logic correctly verifies that thewithdrawAndRevertfunction fails if the sender is nottssAddress.
384-390: LGTM!The visibility change to
publicis appropriate for a test function. The test logic correctly verifies that theexecuteRevertfunction fails if the sender is nottssAddress.
355-360: LGTM!The visibility change to
publicis appropriate for a test function. The test logic correctly verifies that thewithdrawAndRevertfunction fails if the amount is 0.
|
|
||
| IGatewayEVM public immutable gateway; | ||
| address public immutable zetaToken; | ||
| address public tssAddress; |
There was a problem hiding this comment.
Consider marking tssAddress as immutable.
The state variable tssAddress can be marked as immutable since it is only set in the constructor and never changed afterward.
- address public tssAddress;
+ address public immutable tssAddress;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 public tssAddress; | |
| address public immutable tssAddress; |
Tools
GitHub Check: Slither
[warning] 19-19: State variables that could be declared immutable
ZetaConnectorNewBase.tssAddress (contracts/prototypes/evm/ZetaConnectorNewBase.sol#19) should be immutable
| @@ -56,24 +72,24 @@ | |||
|
|
|||
| // Called by the TSS | |||
| // Calling onRevert directly | |||
| function executeRevert(address destination, bytes calldata data) public payable { | |||
| function executeRevert(address destination, bytes calldata data) public payable onlyTSS { | |||
There was a problem hiding this comment.
Add zero address validation for destination.
The destination parameter should be validated to ensure it is not a zero address.
+ if (destination == address(0)) revert ZeroAddress();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.
| function executeRevert(address destination, bytes calldata data) public payable onlyTSS { | |
| function executeRevert(address destination, bytes calldata data) public payable onlyTSS { | |
| + if (destination == address(0)) revert ZeroAddress(); |
Tools
GitHub Check: Slither
[notice] 75-75: Missing zero address validation
GatewayEVM.executeRevert(address,bytes).destination (contracts/prototypes/evm/GatewayEVM.sol#75) lacks a zero-check on :
- (success,result) = destination.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVM.sol#76)
[notice] 75-81: Reentrancy vulnerabilities
Reentrancy in GatewayEVM.executeRevert(address,bytes) (contracts/prototypes/evm/GatewayEVM.sol#75-81):
External calls:
- (success,result) = destination.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVM.sol#76)
- Revertable(destination).onRevert(data) (contracts/prototypes/evm/GatewayEVM.sol#78)
External calls sending eth:
- (success,result) = destination.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVM.sol#76)
Event emitted after the call(s):
- Reverted(destination,msg.value,data) (contracts/prototypes/evm/GatewayEVM.sol#80)
[warning] 75-81: Low-level calls
Low level call in GatewayEVM.executeRevert(address,bytes) (contracts/prototypes/evm/GatewayEVM.sol#75-81):
- (success,result) = destination.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVM.sol#76)
|
@lumtis do we also need tssUpdater? |
| if (custody != address(0)) revert CustodyInitialized(); | ||
| if (_custody == address(0)) revert ZeroAddress(); | ||
|
|
||
| custody = _custody; | ||
| } | ||
|
|
||
| function setConnector(address _zetaConnector) external { | ||
| function setConnector(address _zetaConnector) external onlyTSS { |
Check warning
Code scanning / Slither
Conformance to Solidity naming conventions Warning
| @@ -163,14 +179,14 @@ | |||
| emit Call(msg.sender, receiver, payload); | |||
| } | |||
|
|
|||
| function setCustody(address _custody) external { | |||
| function setCustody(address _custody) external onlyTSS { | |||
Check warning
Code scanning / Slither
Conformance to Solidity naming conventions Warning
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (4)
contracts/prototypes/evm/GatewayEVM.sol (4)
Line range hint
75-81:
Add zero address validation and address reentrancy vulnerabilities.The
destinationparameter should be validated to ensure it is not a zero address. Additionally, consider using the Checks-Effects-Interactions pattern to mitigate reentrancy vulnerabilities.+ if (destination == address(0)) revert ZeroAddress(); (bool success, bytes memory result) = destination.call{value: msg.value}(""); if (!success) revert ExecutionFailed(); Revertable(destination).onRevert(data); emit Reverted(destination, msg.value, data);Tools
GitHub Check: Slither
[notice] 75-75: Missing zero address validation
GatewayEVM.executeRevert(address,bytes).destination (contracts/prototypes/evm/GatewayEVM.sol#75) lacks a zero-check on :
- (success,result) = destination.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVM.sol#76)
Line range hint
86-92:
Address reentrancy vulnerabilities.Consider using the Checks-Effects-Interactions pattern to mitigate reentrancy vulnerabilities.
bytes memory result = _execute(destination, data); emit Executed(destination, msg.value, data); return result;
Line range hint
103-122:
Address reentrancy vulnerabilities and ensure proper approval handling.Consider using the Checks-Effects-Interactions pattern to mitigate reentrancy vulnerabilities. Additionally, ensure that token approvals are handled securely.
if (amount == 0) revert InsufficientERC20Amount(); // Approve the target contract to spend the tokens if(!resetApproval(token, to)) revert ApprovalFailed(); if(!IERC20(token).approve(to, amount)) revert ApprovalFailed(); // Execute the call on the target contract bytes memory result = _execute(to, data); // Reset approval if(!resetApproval(token, to)) revert ApprovalFailed(); // Transfer any remaining tokens back to the custody/connector contract uint256 remainingBalance = IERC20(token).balanceOf(address(this)); if (remainingBalance > 0) { transferToAssetHandler(token, remainingBalance); } emit ExecutedWithERC20(token, to, amount, data);
Line range hint
189-193:
Follow naming conventions.The parameter name should follow Solidity naming conventions.
function setConnector(address _zetaConnector) external onlyTSS { if (zetaConnector != address(0)) revert CustodyInitialized(); if (_zetaConnector == address(0)) revert ZeroAddress(); zetaConnector = _zetaConnector; }Tools
GitHub Check: Slither
[notice] 182-187: Missing events access control
GatewayEVM.setCustody(address) (contracts/prototypes/evm/GatewayEVM.sol#182-187) should emit an event for:
- custody = _custody (contracts/prototypes/evm/GatewayEVM.sol#186)
[warning] 189-189: Conformance to Solidity naming conventions
Parameter GatewayEVM.setConnector(address)._zetaConnector (contracts/prototypes/evm/GatewayEVM.sol#189) is not in mixedCase
[warning] 182-182: Conformance to Solidity naming conventions
Parameter GatewayEVM.setCustody(address)._custody (contracts/prototypes/evm/GatewayEVM.sol#182) is not in mixedCase
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (7)
- contracts/prototypes/evm/GatewayEVM.sol (6 hunks)
- test/prototypes/GatewayEVMUniswap.spec.ts (2 hunks)
- testFoundry/GatewayEVM.t.sol (17 hunks)
- testFoundry/GatewayEVMUpgrade.t.sol (1 hunks)
- testFoundry/GatewayEVMZEVM.t.sol (6 hunks)
- testFoundry/ZetaConnectorNative.t.sol (9 hunks)
- testFoundry/ZetaConnectorNonNative.t.sol (9 hunks)
Files skipped from review as they are similar to previous changes (6)
- test/prototypes/GatewayEVMUniswap.spec.ts
- testFoundry/GatewayEVM.t.sol
- testFoundry/GatewayEVMUpgrade.t.sol
- testFoundry/GatewayEVMZEVM.t.sol
- testFoundry/ZetaConnectorNative.t.sol
- testFoundry/ZetaConnectorNonNative.t.sol
Additional context used
Path-based instructions (1)
contracts/prototypes/evm/GatewayEVM.sol (1)
Pattern
contracts/**: Review the Solidity contracts for security vulnerabilities and best practices.
GitHub Check: Slither
contracts/prototypes/evm/GatewayEVM.sol
[notice] 75-75: Missing zero address validation
GatewayEVM.executeRevert(address,bytes).destination (contracts/prototypes/evm/GatewayEVM.sol#75) lacks a zero-check on :
- (success,result) = destination.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVM.sol#76)
[notice] 75-81: Reentrancy vulnerabilities
Reentrancy in GatewayEVM.executeRevert(address,bytes) (contracts/prototypes/evm/GatewayEVM.sol#75-81):
External calls:
- (success,result) = destination.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVM.sol#76)
- Revertable(destination).onRevert(data) (contracts/prototypes/evm/GatewayEVM.sol#78)
External calls sending eth:
- (success,result) = destination.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVM.sol#76)
Event emitted after the call(s):
- Reverted(destination,msg.value,data) (contracts/prototypes/evm/GatewayEVM.sol#80)
[warning] 75-81: Low-level calls
Low level call in GatewayEVM.executeRevert(address,bytes) (contracts/prototypes/evm/GatewayEVM.sol#75-81):
- (success,result) = destination.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVM.sol#76)
[notice] 86-92: Reentrancy vulnerabilities
Reentrancy in GatewayEVM.execute(address,bytes) (contracts/prototypes/evm/GatewayEVM.sol#86-92):
External calls:
- result = _execute(destination,data) (contracts/prototypes/evm/GatewayEVM.sol#87)
- (success,result) = destination.call{value: msg.value}(data) (contracts/prototypes/evm/GatewayEVM.sol#67)
Event emitted after the call(s):
- Executed(destination,msg.value,data) (contracts/prototypes/evm/GatewayEVM.sol#89)
[notice] 182-187: Missing events access control
GatewayEVM.setCustody(address) (contracts/prototypes/evm/GatewayEVM.sol#182-187) should emit an event for:
- custody = _custody (contracts/prototypes/evm/GatewayEVM.sol#186)
[warning] 189-189: Conformance to Solidity naming conventions
Parameter GatewayEVM.setConnector(address)._zetaConnector (contracts/prototypes/evm/GatewayEVM.sol#189) is not in mixedCase
[warning] 182-182: Conformance to Solidity naming conventions
Parameter GatewayEVM.setCustody(address)._custody (contracts/prototypes/evm/GatewayEVM.sol#182) is not in mixedCase
Additional comments not posted (3)
contracts/prototypes/evm/GatewayEVM.sol (3)
30-36: LGTM! TheonlyTSSmodifier is correctly implemented.The modifier ensures that only the
tssAddresscan execute the functions it is applied to.
38-44: LGTM! TheonlyCustodyOrConnectormodifier is correctly implemented.The modifier ensures that only the
custodyorzetaConnectoraddresses can execute the functions it is applied to.
Line range hint
130-138:
LGTM! But review for reentrancy vulnerabilities.The function appears to be correctly implemented, but consider reviewing for potential reentrancy vulnerabilities.
Verification successful
LGTM! But review for reentrancy vulnerabilities.
The function
revertWithERC20appears to be correctly implemented with thenonReentrantmodifier, which is correctly designed to prevent reentrancy attacks.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify reentrancy vulnerabilities in `revertWithERC20`. # Test: Search for reentrancy patterns. Expect: No reentrancy vulnerabilities. rg --type solidity -A 5 $'revertWithERC20'Length of output: 74
Script:
#!/bin/bash # Description: Verify reentrancy vulnerabilities in `revertWithERC20`. # Test: Search for reentrancy patterns. Expect: No reentrancy vulnerabilities. rg 'function revertWithERC20' -A 10 --glob '*.sol'Length of output: 2379
Script:
#!/bin/bash # Description: Verify the implementation of the `nonReentrant` modifier. # Test: Search for the `nonReentrant` modifier implementation. Expect: Correct implementation. rg 'modifier nonReentrant' -A 10 --glob '*.sol'Length of output: 2786
| function setCustody(address _custody) external onlyTSS { | ||
| if (custody != address(0)) revert CustodyInitialized(); | ||
| if (_custody == address(0)) revert ZeroAddress(); | ||
|
|
||
| custody = _custody; | ||
| } |
There was a problem hiding this comment.
Emit an event and follow naming conventions.
The function should emit an event when the custody address is set. Additionally, the parameter name should follow Solidity naming conventions.
function setCustody(address _custody) external onlyTSS {
if (custody != address(0)) revert CustodyInitialized();
if (_custody == address(0)) revert ZeroAddress();
custody = _custody;
+ emit CustodySet(_custody);
}Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: Slither
[notice] 182-187: Missing events access control
GatewayEVM.setCustody(address) (contracts/prototypes/evm/GatewayEVM.sol#182-187) should emit an event for:
- custody = _custody (contracts/prototypes/evm/GatewayEVM.sol#186)
[warning] 182-182: Conformance to Solidity naming conventions
Parameter GatewayEVM.setCustody(address)._custody (contracts/prototypes/evm/GatewayEVM.sol#182) is not in mixedCase
| // SPDX-License-Identifier: MIT | ||
| pragma solidity ^0.8.0; | ||
|
|
||
| interface IZetaConnectorEvents { |
There was a problem hiding this comment.
In my opinion the interface shouldn't be specific for events or errors.
When importing an interface, as a developer I'd expect to have full access to the interface: functions, errors and events since the logic is coupled, meaning that the errors and events are related to the functions, etc
My suggestion here and in all the interface files would be grouping events, errors and functions under interface Foo { ... }
There was a problem hiding this comment.
agree, one use case for having separate is if we want to import only events and errors in situations like tests, it is nice to be more granular if we are using interfaces
maybe dont have errors and events in interface then?
There was a problem hiding this comment.
will follow up on this one, good point
|
|
||
| IGatewayEVM public immutable gateway; | ||
| address public immutable zetaToken; | ||
| address public tssAddress; |
There was a problem hiding this comment.
As analyzed by slither:
| address public tssAddress; | |
| address public immutable tssAddress; |
tssAddress shouldn't change after setting it at deployment time.
There was a problem hiding this comment.
lets see if we will need a way to update it with tssUpdater
There was a problem hiding this comment.
will follow up here: #255
|
|
||
| IGatewayEVM public gateway; | ||
| address public tssAddress; |
There was a problem hiding this comment.
| address public tssAddress; | |
| address public immutable tssAddress; |
| // As the current version, ERC20CustodyNew hold the ERC20s deposited on ZetaChain | ||
| // This version include a functionality allowing to call a contract | ||
| // ERC20Custody doesn't call smart contract directly, it passes through the Gateway contract | ||
| contract ERC20CustodyNew is ReentrancyGuard{ | ||
| contract ERC20CustodyNew is IERC20CustodyNewEvents, IERC20CustodyNewErrors, ReentrancyGuard { |
There was a problem hiding this comment.
As mentioned in other comment, the ideal imho would be:
contract ERC20CustodyNew is IERC20Custody, ReentrancyGuard
|
|
||
| IGatewayEVM public immutable gateway; | ||
| address public immutable zetaToken; | ||
| address public tssAddress; |
There was a problem hiding this comment.
| address public tssAddress; | |
| address public immutable tssAddress; |
| event WithdrawAndCall(address indexed to, uint256 amount, bytes data); | ||
| event WithdrawAndRevert(address indexed to, uint256 amount, bytes data); | ||
| // @dev Only TSS address allowed modifier. | ||
| modifier onlyTSS() { |
There was a problem hiding this comment.
nit: onlyTSS() is defined two times, we could consider moving it to an external file which defines modifier onlyAddress(address)
There was a problem hiding this comment.
what would be a good solution here, would we need separate contract which defines this modifier and inherit from it?
There was a problem hiding this comment.
We can aim for something in the direction of open-zeppelin Ownable (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol)
This way we can abstract the access control logic into a single contract, something like TSSAccessControl.sol that exposes:
- defines a tssAddress.
- defines the logic to update the tssAddress (more on this later).
- exposes an onlyTSS modifier which can be used from different contracts, and those don't have to even know where the TSS address is pointing to.
Regarding the upgrading logic, bear with me as I still lack some context on when and how the TSS address is updated I see a couple options:
- how is it performed currently?
- we could explore current developed proxy patterns, such as open zeppelin
UpgradeableBeacon. - we could have some proxy custom developed pattern implementation, like:
TSSAddress.sol <- TSSAddressAccessControl.sol <- [contracts implementing RBAC]- TSSAddress.sol has a const tssAddress.
- TSSAddressAccessControl would have a tssAddress variable which is proxied to TSSAddress.sol
- Rest of the contracts would use TSSAddressAccessControl functions and modifiers.
- Upgrade procedure: deploy a new TSSAddress.sol, switch the proxy implementation in TSSAddressAccessControl.
There was a problem hiding this comment.
i like having TssAccessControl.sol, thanks for suggestion , let's tackle that and remaining questions in #255
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- contracts/prototypes/evm/GatewayEVM.sol (6 hunks)
- v2/lib/forge-std (1 hunks)
- v2/lib/openzeppelin-contracts-upgradeable (1 hunks)
- v2/lib/openzeppelin-foundry-upgrades (1 hunks)
Files skipped from review due to trivial changes (3)
- v2/lib/forge-std
- v2/lib/openzeppelin-contracts-upgradeable
- v2/lib/openzeppelin-foundry-upgrades
Files skipped from review as they are similar to previous changes (1)
- contracts/prototypes/evm/GatewayEVM.sol
yes, will add after #255 is done |
closes #197
Summary by CodeRabbit
New Features
tssAddressparameter, allowing only authorized entities to perform critical withdrawal operations.Bug Fixes
Tests
Chores