feat: add zeta handling to v2 contracts #214
Conversation
Co-authored-by: lumtis <[email protected]>
Co-authored-by: lumtis <[email protected]>
Co-authored-by: lumtis <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (10)
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/zetaconnectornew.sol/zetaconnectornew.gois excluded by!pkg/**typechain-types/contracts/prototypes/test/GatewayEVMZEVM.t.sol/GatewayEVMZEVMTest.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/ZetaConnectorNew__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/test/GatewayEVMZEVM.t.sol/GatewayEVMZEVMTest__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 (6)
- contracts/prototypes/evm/GatewayEVM.sol (5 hunks)
- contracts/prototypes/evm/GatewayEVMUpgradeTest.sol (2 hunks)
- foundry.toml (1 hunks)
- test/fuzz/ERC20CustodyNewEchidnaTest.sol (1 hunks)
- test/fuzz/GatewayEVMEchidnaTest.sol (1 hunks)
- testFoundry/GatewayEVM.t.sol (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- foundry.toml
- test/fuzz/ERC20CustodyNewEchidnaTest.sol
- test/fuzz/GatewayEVMEchidnaTest.sol
Additional context used
Path-based instructions (2)
contracts/prototypes/evm/GatewayEVM.sol (1)
Pattern
contracts/**: Review the Solidity contracts for security vulnerabilities and best practices.contracts/prototypes/evm/GatewayEVMUpgradeTest.sol (1)
Pattern
contracts/**: Review the Solidity contracts for security vulnerabilities and best practices.
GitHub Check: Slither
contracts/prototypes/evm/GatewayEVM.sol
[warning] 26-26: Conformance to Solidity naming conventions
Parameter GatewayEVM.initialize(address,address)._zeta (contracts/prototypes/evm/GatewayEVM.sol#26) is not in mixedCase
[warning] 26-26: Conformance to Solidity naming conventions
Parameter GatewayEVM.initialize(address,address)._tssAddress (contracts/prototypes/evm/GatewayEVM.sol#26) is not in mixedCase
[warning] 152-152: Conformance to Solidity naming conventions
Parameter GatewayEVM.setConnector(address)._zetaConnector (contracts/prototypes/evm/GatewayEVM.sol#152) is not in mixedCasecontracts/prototypes/evm/GatewayEVMUpgradeTest.sol
[notice] 27-27: Missing zero address validation
GatewayEVMUpgradeTest.initialize(address,address)._zetaAsset (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#27) lacks a zero-check on :
- zetaAsset = _zetaAsset (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#34)
[warning] 27-27: Conformance to Solidity naming conventions
Parameter GatewayEVMUpgradeTest.initialize(address,address)._zetaAsset (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#27) is not in mixedCase
[warning] 27-27: Conformance to Solidity naming conventions
Parameter GatewayEVMUpgradeTest.initialize(address,address)._tssAddress (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#27) is not in mixedCase
[notice] 150-150: Missing zero address validation
GatewayEVMUpgradeTest.setConnector(address)._zetaConnector (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#150) lacks a zero-check on :
- zetaConnector = _zetaConnector (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#152)
[notice] 118-125: Reentrancy vulnerabilities
Reentrancy in GatewayEVMUpgradeTest.depositAndCall(address,bytes) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#118-125):
External calls:
- (deposited,None) = tssAddress.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#120)
Event emitted after the call(s):
- Deposit(msg.sender,receiver,msg.value,address(0),payload) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#124)
[notice] 128-138: Reentrancy vulnerabilities
Reentrancy in GatewayEVMUpgradeTest.depositAndCall(address,uint256,address,bytes) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#128-138):
External calls:
- IERC20(asset).safeTransferFrom(msg.sender,address(destination),amount) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#135)
Event emitted after the call(s):
- Deposit(msg.sender,receiver,amount,asset,payload) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#137)
[notice] 105-115: Reentrancy vulnerabilities
Reentrancy in GatewayEVMUpgradeTest.deposit(address,uint256,address) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#105-115):
External calls:
- IERC20(asset).safeTransferFrom(msg.sender,address(destination),amount) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#112)
Event emitted after the call(s):
- Deposit(msg.sender,receiver,amount,asset,) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#114)
[notice] 95-102: Reentrancy vulnerabilities
Reentrancy in GatewayEVMUpgradeTest.deposit(address) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#95-102):
External calls:
- (deposited,None) = tssAddress.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#97)
Event emitted after the call(s):
- Deposit(msg.sender,receiver,msg.value,address(0),) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#101)
[warning] 95-102: Boolean equality
GatewayEVMUpgradeTest.deposit(address) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#95-102) compares to a boolean constant:
-deposited == false (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#99)
[warning] 118-125: Boolean equality
GatewayEVMUpgradeTest.depositAndCall(address,bytes) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#118-125) compares to a boolean constant:
-deposited == false (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#122)
[warning] 118-125: Low-level calls
Low level call in GatewayEVMUpgradeTest.depositAndCall(address,bytes) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#118-125):
- (deposited,None) = tssAddress.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#120)
[warning] 95-102: Low-level calls
Low level call in GatewayEVMUpgradeTest.deposit(address) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#95-102):
- (deposited,None) = tssAddress.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#97)
[warning] 150-150: Conformance to Solidity naming conventions
Parameter GatewayEVMUpgradeTest.setConnector(address)._zetaConnector (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#150) is not in mixedCase
Additional comments not posted (5)
contracts/prototypes/evm/GatewayEVM.sol (4)
18-19: New public variables added for zeta handling.The addition of
zetaConnectorandzetaas public variables aligns with the PR's objective to handle zeta in v2 contracts.
79-86: Modified token transfer logic to handle zeta.The logic now differentiates between
custodyandzetaConnectorbased on the token being transferred. This is a significant change to ensure that zeta tokens are handled correctly according to the new specifications.
104-112: Deposit function for ERC20 tokens updated to handle zeta.The function now correctly routes zeta tokens to the
zetaConnectorinstead ofcustody. This change is crucial for the correct handling of zeta tokens as per the new protocol specifications.
127-135: Deposit and call function for ERC20 tokens updated to handle zeta.Similar to the simple deposit function, this function also correctly routes zeta tokens to the
zetaConnector. This ensures that the protocol's behavior is consistent across different types of transactions.testFoundry/GatewayEVM.t.sol (1)
Line range hint
10-53: Confirm correct setup and usage of new instances.The new imports and instances for
ZetaConnectorNewandTestERC20calledzetaare correctly set up. Ensure that these are used appropriately in the test scenarios.
| function initialize(address _tssAddress, address _zeta) public initializer { | ||
| __Ownable_init(); | ||
| __UUPSUpgradeable_init(); | ||
|
|
||
| if (_tssAddress == address(0)) revert ZeroAddress(); | ||
| if (_tssAddress == address(0) || _zeta == address(0)) { | ||
| revert ZeroAddress(); | ||
| } | ||
|
|
||
| tssAddress = _tssAddress; | ||
| zeta = _zeta; |
There was a problem hiding this comment.
Initialization function modified to include zeta handling.
The initialize function now accepts an additional _zeta parameter and includes a check for zero addresses, which is a critical security measure. However, the parameter names _tssAddress and _zeta do not conform to Solidity naming conventions (should be in mixedCase).
- function initialize(address _tssAddress, address _zeta) public initializer {
+ function initialize(address tssAddress, address zeta) public initializer {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 initialize(address _tssAddress, address _zeta) public initializer { | |
| __Ownable_init(); | |
| __UUPSUpgradeable_init(); | |
| if (_tssAddress == address(0)) revert ZeroAddress(); | |
| if (_tssAddress == address(0) || _zeta == address(0)) { | |
| revert ZeroAddress(); | |
| } | |
| tssAddress = _tssAddress; | |
| zeta = _zeta; | |
| function initialize(address tssAddress, address zeta) public initializer { | |
| __Ownable_init(); | |
| __UUPSUpgradeable_init(); | |
| if (tssAddress == address(0) || zeta == address(0)) { | |
| revert ZeroAddress(); | |
| } | |
| tssAddress = tssAddress; | |
| zeta = zeta; |
Tools
GitHub Check: Slither
[warning] 26-26: Conformance to Solidity naming conventions
Parameter GatewayEVM.initialize(address,address)._zeta (contracts/prototypes/evm/GatewayEVM.sol#26) is not in mixedCase
[warning] 26-26: Conformance to Solidity naming conventions
Parameter GatewayEVM.initialize(address,address)._tssAddress (contracts/prototypes/evm/GatewayEVM.sol#26) is not in mixedCase
| function setConnector(address _zetaConnector) external { | ||
| if (zetaConnector != address(0)) revert CustodyInitialized(); | ||
| if (_zetaConnector == address(0)) revert ZeroAddress(); | ||
|
|
||
| zetaConnector = _zetaConnector; | ||
| } |
There was a problem hiding this comment.
New function to set zetaConnector.
The setConnector function allows setting the zetaConnector address. This function includes a check to ensure that zetaConnector is not already set, which is a good practice to prevent accidental overwrites. However, the parameter _zetaConnector should be in mixedCase to adhere to Solidity naming conventions.
- function setConnector(address _zetaConnector) external {
+ function setConnector(address zetaConnector) external {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 setConnector(address _zetaConnector) external { | |
| if (zetaConnector != address(0)) revert CustodyInitialized(); | |
| if (_zetaConnector == address(0)) revert ZeroAddress(); | |
| zetaConnector = _zetaConnector; | |
| } | |
| function setConnector(address zetaConnector) external { | |
| if (zetaConnector != address(0)) revert CustodyInitialized(); | |
| if (zetaConnector == address(0)) revert ZeroAddress(); | |
| zetaConnector = zetaConnector; | |
| } |
Tools
GitHub Check: Slither
[warning] 152-152: Conformance to Solidity naming conventions
Parameter GatewayEVM.setConnector(address)._zetaConnector (contracts/prototypes/evm/GatewayEVM.sol#152) is not in mixedCase
| function initialize(address _tssAddress, address _zetaAsset) public initializer { | ||
| __Ownable_init(); | ||
| __UUPSUpgradeable_init(); | ||
|
|
||
| if (_tssAddress == address(0)) revert ZeroAddress(); | ||
|
|
||
| tssAddress = _tssAddress; | ||
| zetaAsset = _zetaAsset; |
There was a problem hiding this comment.
Implement zero address validation and adjust parameter naming.
The initialize function lacks a zero address check for _zetaAsset, which is crucial for security. Also, parameter names should follow camelCase conventions.
27 function initialize(address _tssAddress, address _zetaAsset) public initializer {
28 __Ownable_init();
29 __UUPSUpgradeable_init();
30
31 if (_tssAddress == address(0) || _zetaAsset == address(0)) revert ZeroAddress();
32
33 tssAddress = _tssAddress;
34 zetaAsset = _zetaAsset;
35 }Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: Slither
[notice] 27-27: Missing zero address validation
GatewayEVMUpgradeTest.initialize(address,address)._zetaAsset (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#27) lacks a zero-check on :
- zetaAsset = _zetaAsset (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#34)
[warning] 27-27: Conformance to Solidity naming conventions
Parameter GatewayEVMUpgradeTest.initialize(address,address)._zetaAsset (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#27) is not in mixedCase
[warning] 27-27: Conformance to Solidity naming conventions
Parameter GatewayEVMUpgradeTest.initialize(address,address)._tssAddress (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#27) is not in mixedCase
| // Deposit ERC20 tokens to custody and call an omnichain smart contract | ||
| function depositAndCall(address receiver, uint256 amount, address asset, bytes calldata payload) external { | ||
| if (amount == 0) revert InsufficientERC20Amount(); | ||
|
|
||
| address destination = address(custody); | ||
| if (asset == zetaAsset) { | ||
| destination = address(zetaConnector); | ||
| } | ||
| IERC20(asset).safeTransferFrom(msg.sender, address(destination), amount); | ||
|
|
||
| if (sent == false) revert SendFailed(); | ||
| emit Deposit(msg.sender, receiver, amount, asset, payload); | ||
| } |
There was a problem hiding this comment.
Address reentrancy concerns in depositAndCall for ERC20.
To prevent reentrancy attacks, consider using the nonReentrant modifier. This is crucial for functions involving external calls.
127 function depositAndCall(address receiver, uint256 amount, address asset, bytes calldata payload) external nonReentrant {
128 if (amount == 0) revert InsufficientERC20Amount();
129
130 address destination = address(custody);
131 if (asset == zetaAsset) {
132 destination = address(zetaConnector);
133 }
134 IERC20(asset).safeTransferFrom(msg.sender, address(destination), amount);
135
136 emit Deposit(msg.sender, receiver, amount, asset, payload);
137 }
138 }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.
| // Deposit ERC20 tokens to custody and call an omnichain smart contract | |
| function depositAndCall(address receiver, uint256 amount, address asset, bytes calldata payload) external { | |
| if (amount == 0) revert InsufficientERC20Amount(); | |
| address destination = address(custody); | |
| if (asset == zetaAsset) { | |
| destination = address(zetaConnector); | |
| } | |
| IERC20(asset).safeTransferFrom(msg.sender, address(destination), amount); | |
| if (sent == false) revert SendFailed(); | |
| emit Deposit(msg.sender, receiver, amount, asset, payload); | |
| } | |
| // Deposit ERC20 tokens to custody and call an omnichain smart contract | |
| function depositAndCall(address receiver, uint256 amount, address asset, bytes calldata payload) external nonReentrant { | |
| if (amount == 0) revert InsufficientERC20Amount(); | |
| address destination = address(custody); | |
| if (asset == zetaAsset) { | |
| destination = address(zetaConnector); | |
| } | |
| IERC20(asset).safeTransferFrom(msg.sender, address(destination), amount); | |
| emit Deposit(msg.sender, receiver, amount, asset, payload); | |
| } |
Tools
GitHub Check: Slither
[notice] 128-138: Reentrancy vulnerabilities
Reentrancy in GatewayEVMUpgradeTest.depositAndCall(address,uint256,address,bytes) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#128-138):
External calls:
- IERC20(asset).safeTransferFrom(msg.sender,address(destination),amount) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#135)
Event emitted after the call(s):
- Deposit(msg.sender,receiver,amount,asset,payload) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#137)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #214 +/- ##
===========================================
- Coverage 58.29% 47.52% -10.78%
===========================================
Files 11 14 +3
Lines 235 303 +68
Branches 61 82 +21
===========================================
+ Hits 137 144 +7
- Misses 98 159 +61 ☔ View full report in Codecov by Sentry. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| function initialize(address _tssAddress) public initializer { | ||
| function initialize(address _tssAddress, address _zeta) public initializer { |
Check notice
Code scanning / Slither
Missing zero address validation Low
|
|
||
| function initialize(address _tssAddress) public initializer { | ||
| function initialize(address _tssAddress, address _zeta) public initializer { |
Check warning
Code scanning / Slither
Conformance to Solidity naming conventions Warning
|
|
||
| function initialize(address _tssAddress) public initializer { | ||
| function initialize(address _tssAddress, address _zeta) public initializer { |
Check warning
Code scanning / Slither
Conformance to Solidity naming conventions Warning
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (3)
pkg/contracts/prototypes/evm/gatewayevmupgradetest.sol/gatewayevmupgradetest.gois excluded by!pkg/**typechain-types/contracts/prototypes/evm/GatewayEVMUpgradeTest.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/GatewayEVMUpgradeTest__factory.tsis excluded by!typechain-types/**
Files selected for processing (5)
- contracts/prototypes/evm/GatewayEVMUpgradeTest.sol (2 hunks)
- testFoundry/GatewayEVM.t.sol (5 hunks)
- testFoundry/GatewayEVMUpgrade.t.sol (3 hunks)
- testFoundry/GatewayEVMZEVM.t.sol (4 hunks)
- testFoundry/GatewayZEVM.t.sol (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- testFoundry/GatewayEVM.t.sol
Additional context used
Path-based instructions (1)
contracts/prototypes/evm/GatewayEVMUpgradeTest.sol (1)
Pattern
contracts/**: Review the Solidity contracts for security vulnerabilities and best practices.
GitHub Check: Slither
contracts/prototypes/evm/GatewayEVMUpgradeTest.sol
[notice] 27-27: Missing zero address validation
GatewayEVMUpgradeTest.initialize(address,address)._zeta (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#27) lacks a zero-check on :
- zeta = _zeta (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#34)
[warning] 27-27: Conformance to Solidity naming conventions
Parameter GatewayEVMUpgradeTest.initialize(address,address)._zeta (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#27) is not in mixedCase
[warning] 27-27: Conformance to Solidity naming conventions
Parameter GatewayEVMUpgradeTest.initialize(address,address)._tssAddress (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#27) is not in mixedCase
[notice] 150-150: Missing zero address validation
GatewayEVMUpgradeTest.setConnector(address)._zetaConnector (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#150) lacks a zero-check on :
- zetaConnector = _zetaConnector (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#152)
[notice] 118-125: Reentrancy vulnerabilities
Reentrancy in GatewayEVMUpgradeTest.depositAndCall(address,bytes) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#118-125):
External calls:
- (deposited,None) = tssAddress.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#120)
Event emitted after the call(s):
- Deposit(msg.sender,receiver,msg.value,address(0),payload) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#124)
[notice] 128-138: Reentrancy vulnerabilities
Reentrancy in GatewayEVMUpgradeTest.depositAndCall(address,uint256,address,bytes) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#128-138):
External calls:
- IERC20(asset).safeTransferFrom(msg.sender,address(destination),amount) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#135)
Event emitted after the call(s):
- Deposit(msg.sender,receiver,amount,asset,payload) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#137)
[notice] 105-115: Reentrancy vulnerabilities
Reentrancy in GatewayEVMUpgradeTest.deposit(address,uint256,address) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#105-115):
External calls:
- IERC20(asset).safeTransferFrom(msg.sender,address(destination),amount) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#112)
Event emitted after the call(s):
- Deposit(msg.sender,receiver,amount,asset,) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#114)
[notice] 95-102: Reentrancy vulnerabilities
Reentrancy in GatewayEVMUpgradeTest.deposit(address) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#95-102):
External calls:
- (deposited,None) = tssAddress.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#97)
Event emitted after the call(s):
- Deposit(msg.sender,receiver,msg.value,address(0),) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#101)
[warning] 95-102: Boolean equality
GatewayEVMUpgradeTest.deposit(address) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#95-102) compares to a boolean constant:
-deposited == false (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#99)
[warning] 118-125: Boolean equality
GatewayEVMUpgradeTest.depositAndCall(address,bytes) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#118-125) compares to a boolean constant:
-deposited == false (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#122)
[warning] 118-125: Low-level calls
Low level call in GatewayEVMUpgradeTest.depositAndCall(address,bytes) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#118-125):
- (deposited,None) = tssAddress.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#120)
[warning] 95-102: Low-level calls
Low level call in GatewayEVMUpgradeTest.deposit(address) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#95-102):
- (deposited,None) = tssAddress.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#97)
[warning] 150-150: Conformance to Solidity naming conventions
Parameter GatewayEVMUpgradeTest.setConnector(address)._zetaConnector (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#150) is not in mixedCase
Additional comments not posted (17)
testFoundry/GatewayEVMUpgrade.t.sol (7)
11-11: New import statements added.The import of
ZetaConnectorNew.solandTestERC20.solintroduces dependencies that are crucial for the new functionalities being tested. Ensure these files are reviewed for security vulnerabilities as well.
27-27: Declaration of newzetaConnectorvariable.This variable is essential for handling Zeta transactions. It's important to ensure that the
ZetaConnectorNewcontract is robust and secure since it plays a critical role in the transaction flow.
29-29: Declaration of newzetavariable.The addition of the
zetavariable, aTestERC20token, is crucial for testing Zeta token interactions within theGatewayEVM. Proper initialization and handling of this token in tests are essential to simulate real-world scenarios accurately.
40-40: Initialization ofzeta.Creating a new instance of
TestERC20named "zeta" is crucial for testing the token handling capabilities of theGatewayEVM. It's important to ensure that theTestERC20contract is correctly implemented and secure.
44-44: Modified initialization call forGatewayEVM.The initialization now includes the
zetaparameter, aligning with the new contract specifications that handle Zeta tokens. This change must be reflected correctly in theGatewayEVMcontract to ensure proper initialization.
49-49: Creation of a newZetaConnectorNewinstance.This step is crucial for integrating the Zeta handling functionality into the
GatewayEVM. It's important to ensure that theZetaConnectorNewcontract is implemented securely and efficiently.
53-53: Setting thezetaConnector.This configuration is key to enabling Zeta transactions through the
GatewayEVM. Proper testing of this configuration is essential to ensure that the connector is set correctly and functions as expected.testFoundry/GatewayZEVM.t.sol (2)
54-54: Updated Withdrawal event to include zrc20 address.The inclusion of the
address(zrc20)in theWithdrawalevent helps ensure that the correct token address is referenced during the withdrawal process.
66-66: Updated Withdrawal event inwithdrawAndCallfunction.Similar to the previous change, this ensures the correct token address is referenced when withdrawals are combined with a call operation.
contracts/prototypes/evm/GatewayEVMUpgradeTest.sol (3)
9-9: Added import for interfaces.This import statement is crucial for the contract to implement the necessary interfaces.
15-21: Expanded contract to implement new interfaces and added new state variables.The contract now implements
IGatewayEVMErrorsandIGatewayEVMEvents, which are essential for error handling and event management. The addition ofzetaConnectorandzetaaddresses supports new functionalities related to Zeta handling.Tools
GitHub Check: Slither
[warning] 15-158: Missing inheritance
GatewayEVMUpgradeTest (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#15-158) should inherit from IGatewayEVM (contracts/prototypes/evm/interfaces.sol#28-38)
27-34: Initialize function now accepts an additionalzetaparameter.This change allows the contract to be initialized with a reference to the Zeta token, which is crucial for operations involving this specific token.
- function initialize(address _tssAddress, address _zeta) public initializer { + function initialize(address _tssAddress, address _zeta) public initializer { + if (_zeta == address(0)) revert ZeroAddress(); // Ensure zeta is not the zero addressEnsure that the
zetaparameter is not the zero address to prevent potential issues with uninitialized or invalid token references.Tools
GitHub Check: Slither
[notice] 27-27: Missing zero address validation
GatewayEVMUpgradeTest.initialize(address,address)._zeta (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#27) lacks a zero-check on :
- zeta = _zeta (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#34)
[warning] 27-27: Conformance to Solidity naming conventions
Parameter GatewayEVMUpgradeTest.initialize(address,address)._zeta (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#27) is not in mixedCase
[warning] 27-27: Conformance to Solidity naming conventions
Parameter GatewayEVMUpgradeTest.initialize(address,address)._tssAddress (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#27) is not in mixedCasetestFoundry/GatewayEVMZEVM.t.sol (5)
10-10: Added import forZetaConnectorNew.sol.This import is necessary for the test contract to utilize the
ZetaConnectorNewfunctionality, which is crucial for handling Zeta-related operations.
33-35: Added new variables for Zeta handling.The introduction of
zetaConnectorandzetavariables allows for more comprehensive testing of Zeta functionalities.
57-58: Initialization of Zeta token.The creation of a new
TestERC20instance for the Zeta token enables detailed testing of token-related functionalities.
65-68: Set up of Zeta connector and custody.The setup of the
zetaConnectorwith the gateway and the setting of the connector in the gateway configuration are crucial steps for ensuring that the Zeta handling functionalities are correctly integrated.
149-149: Updated Withdrawal event to include zrc20 address intestWithdrawAndCallReceiverEVMFromZEVM.This update ensures that the correct token address is used in the event, which is crucial for accurate event logging and tracking.
| // Deposit ETH to tss | ||
| function deposit(address receiver) external payable { | ||
| if (msg.value == 0) revert InsufficientETHAmount(); | ||
| (bool deposited, ) = tssAddress.call{value: msg.value}(""); | ||
|
|
||
| emit SendERC20(recipient, token, amount); | ||
| if (deposited == false) revert DepositFailed(); | ||
|
|
||
| emit Deposit(msg.sender, receiver, msg.value, address(0), ""); |
There was a problem hiding this comment.
New function to deposit ETH.
This function allows for the deposit of ETH to a specified receiver. It includes checks for zero deposits and handles the actual transfer.
However, consider using high-level calls instead of low-level ones to improve security.
- (bool deposited, ) = tssAddress.call{value: msg.value}("");
+ require(tssAddress.send(msg.value), "Deposit failed");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.
| // Deposit ETH to tss | |
| function deposit(address receiver) external payable { | |
| if (msg.value == 0) revert InsufficientETHAmount(); | |
| (bool deposited, ) = tssAddress.call{value: msg.value}(""); | |
| emit SendERC20(recipient, token, amount); | |
| if (deposited == false) revert DepositFailed(); | |
| emit Deposit(msg.sender, receiver, msg.value, address(0), ""); | |
| // Deposit ETH to tss | |
| function deposit(address receiver) external payable { | |
| if (msg.value == 0) revert InsufficientETHAmount(); | |
| require(tssAddress.send(msg.value), "Deposit failed"); | |
| emit Deposit(msg.sender, receiver, msg.value, address(0), ""); |
| // Deposit ETH to tss and call an omnichain smart contract | ||
| function depositAndCall(address receiver, bytes calldata payload) external payable { | ||
| if (msg.value == 0) revert InsufficientETHAmount(); | ||
| (bool deposited, ) = tssAddress.call{value: msg.value}(""); | ||
|
|
||
| (bool sent, ) = tssAddress.call{value: msg.value}(""); | ||
| if (deposited == false) revert DepositFailed(); | ||
|
|
||
| emit Deposit(msg.sender, receiver, msg.value, address(0), payload); | ||
| } | ||
|
|
||
| // Deposit ERC20 tokens to custody and call an omnichain smart contract | ||
| function depositAndCall(address receiver, uint256 amount, address asset, bytes calldata payload) external { | ||
| if (amount == 0) revert InsufficientERC20Amount(); | ||
|
|
||
| address destination = address(custody); | ||
| if (asset == zeta) { | ||
| destination = address(zetaConnector); | ||
| } | ||
| IERC20(asset).safeTransferFrom(msg.sender, address(destination), amount); | ||
|
|
||
| if (sent == false) revert SendFailed(); | ||
| emit Deposit(msg.sender, receiver, amount, asset, payload); | ||
| } |
There was a problem hiding this comment.
New functions for depositing and calling.
These functions allow for the deposit of assets (both ETH and ERC20) and the simultaneous calling of another contract. This is a complex interaction that requires careful handling of state changes and external calls.
However, ensure that reentrancy guards are used here as well.
+ @nonReentrant
function depositAndCall(address receiver, bytes calldata payload) external payable {
+ @nonReentrant
function depositAndCall(address receiver, uint256 amount, address asset, bytes calldata payload) external {Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: Slither
[notice] 118-125: Reentrancy vulnerabilities
Reentrancy in GatewayEVMUpgradeTest.depositAndCall(address,bytes) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#118-125):
External calls:
- (deposited,None) = tssAddress.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#120)
Event emitted after the call(s):
- Deposit(msg.sender,receiver,msg.value,address(0),payload) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#124)
[notice] 128-138: Reentrancy vulnerabilities
Reentrancy in GatewayEVMUpgradeTest.depositAndCall(address,uint256,address,bytes) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#128-138):
External calls:
- IERC20(asset).safeTransferFrom(msg.sender,address(destination),amount) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#135)
Event emitted after the call(s):
- Deposit(msg.sender,receiver,amount,asset,payload) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#137)
[warning] 118-125: Boolean equality
GatewayEVMUpgradeTest.depositAndCall(address,bytes) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#118-125) compares to a boolean constant:
-deposited == false (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#122)
[warning] 118-125: Low-level calls
Low level call in GatewayEVMUpgradeTest.depositAndCall(address,bytes) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#118-125):
- (deposited,None) = tssAddress.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#120)
| // Deposit ERC20 tokens to custody | ||
| function deposit(address receiver, uint256 amount, address asset) external { | ||
| if (amount == 0) revert InsufficientERC20Amount(); | ||
|
|
||
| address destination = address(custody); | ||
| if (asset == zeta) { | ||
| destination = address(zetaConnector); | ||
| } | ||
| IERC20(asset).safeTransferFrom(msg.sender, address(destination), amount); | ||
|
|
||
| emit Deposit(msg.sender, receiver, amount, asset, ""); | ||
| } |
There was a problem hiding this comment.
New function to deposit ERC20 tokens.
This function allows for the deposit of ERC20 tokens to a specified receiver. It includes checks for zero deposits and correct handling of the token transfer.
Consider adding a reentrancy guard to this function as well.
+ @nonReentrant
function deposit(address receiver, uint256 amount, address asset) external {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.
| // Deposit ERC20 tokens to custody | |
| function deposit(address receiver, uint256 amount, address asset) external { | |
| if (amount == 0) revert InsufficientERC20Amount(); | |
| address destination = address(custody); | |
| if (asset == zeta) { | |
| destination = address(zetaConnector); | |
| } | |
| IERC20(asset).safeTransferFrom(msg.sender, address(destination), amount); | |
| emit Deposit(msg.sender, receiver, amount, asset, ""); | |
| } | |
| @nonReentrant | |
| // Deposit ERC20 tokens to custody | |
| function deposit(address receiver, uint256 amount, address asset) external { | |
| if (amount == 0) revert InsufficientERC20Amount(); | |
| address destination = address(custody); | |
| if (asset == zeta) { | |
| destination = address(zetaConnector); | |
| } | |
| IERC20(asset).safeTransferFrom(msg.sender, address(destination), amount); | |
| emit Deposit(msg.sender, receiver, amount, asset, ""); | |
| } |
Tools
GitHub Check: Slither
[notice] 105-115: Reentrancy vulnerabilities
Reentrancy in GatewayEVMUpgradeTest.deposit(address,uint256,address) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#105-115):
External calls:
- IERC20(asset).safeTransferFrom(msg.sender,address(destination),amount) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#112)
Event emitted after the call(s):
- Deposit(msg.sender,receiver,amount,asset,) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#114)
| ) public returns (bytes memory) { | ||
| if (amount == 0) revert InsufficientETHAmount(); | ||
| // Approve the target contract to spend the tokens | ||
| if(!IERC20(token).approve(to, 0)) revert ApprovalFailed(); | ||
| 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(!IERC20(token).approve(to, 0)) revert ApprovalFailed(); | ||
| if(!resetApproval(token, to)) revert ApprovalFailed(); | ||
|
|
||
| // Transfer any remaining tokens back to the custody contract | ||
| // Transfer any remaining tokens back to the custody/connector contract | ||
| uint256 remainingBalance = IERC20(token).balanceOf(address(this)); | ||
| if (remainingBalance > 0) { | ||
| IERC20(token).safeTransfer(address(custody), remainingBalance); | ||
| address destination = address(custody); | ||
| if (token == zeta) { | ||
| destination = address(zetaConnector); | ||
| } | ||
| IERC20(token).safeTransfer(address(destination), remainingBalance); |
There was a problem hiding this comment.
Complex token handling in executeWithERC20.
This function handles the execution of calls with ERC20 token transfers. It includes setting and resetting approvals, which is crucial for security.
However, consider adding a reentrancy guard to prevent potential vulnerabilities associated with external calls.
+ @nonReentrant
function executeWithERC20(address token, address to, uint256 amount, bytes calldata data) public returns (bytes memory) {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.
| ) public returns (bytes memory) { | |
| if (amount == 0) revert InsufficientETHAmount(); | |
| // Approve the target contract to spend the tokens | |
| if(!IERC20(token).approve(to, 0)) revert ApprovalFailed(); | |
| 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(!IERC20(token).approve(to, 0)) revert ApprovalFailed(); | |
| if(!resetApproval(token, to)) revert ApprovalFailed(); | |
| // Transfer any remaining tokens back to the custody contract | |
| // Transfer any remaining tokens back to the custody/connector contract | |
| uint256 remainingBalance = IERC20(token).balanceOf(address(this)); | |
| if (remainingBalance > 0) { | |
| IERC20(token).safeTransfer(address(custody), remainingBalance); | |
| address destination = address(custody); | |
| if (token == zeta) { | |
| destination = address(zetaConnector); | |
| } | |
| IERC20(token).safeTransfer(address(destination), remainingBalance); | |
| @nonReentrant | |
| function executeWithERC20(address token, address to, uint256 amount, bytes calldata data) public returns (bytes memory) { | |
| if (amount == 0) revert InsufficientETHAmount(); | |
| // 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) { | |
| address destination = address(custody); | |
| if (token == zeta) { | |
| destination = address(zetaConnector); | |
| } | |
| IERC20(token).safeTransfer(address(destination), remainingBalance); |
| @@ -15,19 +15,24 @@ contract GatewayEVM is Initializable, OwnableUpgradeable, UUPSUpgradeable, IGate | |||
|
|
|||
| address public custody; | |||
| address public tssAddress; | |||
| address public zetaConnector; | |||
There was a problem hiding this comment.
We have an issue to track adding the right comment to the contracts #204
But since we're starting to introduce many variables, this might make sense to try adding specs as we add the new variable?
| function withdrawAndCall(address to, uint256 amount, bytes calldata data) public nonReentrant { | ||
| // TODO: mint? | ||
| // Transfer zeta to the Gateway contract | ||
| zeta.safeTransfer(address(gateway), amount); | ||
|
|
||
| // Forward the call to the Gateway contract | ||
| gateway.executeWithERC20(address(zeta), to, amount, data); | ||
|
|
||
| emit WithdrawAndCall(to, amount, data); | ||
| } |
There was a problem hiding this comment.
I might think we don't need a return for executeWithERC20.
It would just emit the right event for observer to make the outbound failing
| uint256 remainingBalance = IERC20(token).balanceOf(address(this)); | ||
| if (remainingBalance > 0) { | ||
| IERC20(token).safeTransfer(address(custody), remainingBalance); | ||
| address destination = address(custody); | ||
| if (token == zeta) { |
There was a problem hiding this comment.
Will zetaToken value used specifically for this check in this contract?
Maybe it would make it simpler to remove it and just transfer back to msg.sender?
There was a problem hiding this comment.
also used for:
// Deposit ERC20 tokens to custody/connector
function deposit(address receiver, uint256 amount, address asset) external {
if (amount == 0) revert InsufficientERC20Amount();
IERC20(asset).safeTransferFrom(msg.sender, getAssetHandler(token), amount);
emit Deposit(msg.sender, receiver, amount, asset, "");
}
|
|
||
| /// @custom:oz-upgrades-unsafe-allow constructor | ||
| constructor() { | ||
| _disableInitializers(); | ||
| } | ||
|
|
||
| function initialize() public initializer { | ||
| function initialize(address _zetaToken) public initializer { |
Check notice
Code scanning / Slither
Missing zero address validation Low
There was a problem hiding this comment.
@skosito ideally we'd implment this zero address checks everywhere.
|
|
||
| function initialize(address _tssAddress) public initializer { | ||
| function initialize(address _tssAddress, address _zetaToken) public initializer { |
Check notice
Code scanning / Slither
Missing zero address validation Low
| function _transferZETA(uint256 amount, address to) internal { | ||
| if (!IWETH9(zetaToken).transferFrom(msg.sender, address(this), amount)) revert ZetaTokenTransferFailed(); | ||
| IWETH9(zetaToken).withdraw(amount); | ||
| (bool sent, ) = to.call{value: amount}(""); | ||
| if (!sent) revert FailedZetaSent(); | ||
| } |
Check warning
Code scanning / Slither
Low-level calls Warning
|
|
||
| function initialize(address _tssAddress) public initializer { | ||
| function initialize(address _tssAddress, address _zetaToken) public initializer { |
Check warning
Code scanning / Slither
Conformance to Solidity naming conventions Warning
|
|
||
| /// @custom:oz-upgrades-unsafe-allow constructor | ||
| constructor() { | ||
| _disableInitializers(); | ||
| } | ||
|
|
||
| function initialize() public initializer { | ||
| function initialize(address _zetaToken) public initializer { |
Check warning
Code scanning / Slither
Conformance to Solidity naming conventions Warning
| /// @custom:oz-upgrades-unsafe-allow constructor | ||
| constructor() { | ||
| _disableInitializers(); | ||
| } | ||
|
|
||
| function initialize(address _tssAddress) public initializer { | ||
| function initialize(address _tssAddress, address _zetaToken) public initializer { |
Check warning
Code scanning / Slither
Conformance to Solidity naming conventions Warning
| /// @custom:oz-upgrades-unsafe-allow constructor | ||
| constructor() { | ||
| _disableInitializers(); | ||
| } | ||
|
|
||
| function initialize(address _tssAddress) public initializer { | ||
| function initialize(address _tssAddress, address _zetaToken) public initializer { |
Check warning
Code scanning / Slither
Conformance to Solidity naming conventions Warning
|
|
||
| function initialize(address _tssAddress) public initializer { | ||
| function initialize(address _tssAddress, address _zetaToken) public initializer { |
Check warning
Code scanning / Slither
Conformance to Solidity naming conventions Warning
There was a problem hiding this comment.
Actionable comments posted: 11
Outside diff range, codebase verification and nitpick comments (4)
contracts/prototypes/evm/GatewayEVM.sol (4)
Line range hint
67-71: Addwithdrawfunction for ZETA tokens.The
withdrawfunction facilitates the withdrawal of ZETA tokens to external chains. It correctly uses the_transferZETAfunction and emits appropriate events. Ensure that reentrancy guards are used to prevent potential vulnerabilities.+ modifier nonReentrant() { + require(!locked, "Reentrant call"); + locked = true; + _; + locked = false; + }Tools
GitHub Check: Slither
[warning] 36-36: Conformance to Solidity naming conventions
Parameter GatewayEVM.initialize(address,address)._tssAddress (contracts/prototypes/evm/GatewayEVM.sol#36) is not in mixedCase
[warning] 36-36: Conformance to Solidity naming conventions
Parameter GatewayEVM.initialize(address,address)._zetaToken (contracts/prototypes/evm/GatewayEVM.sol#36) is not in mixedCase
Line range hint
73-76: AddwithdrawAndCallfunction for ZETA tokens.The
withdrawAndCallfunction allows for the withdrawal of ZETA tokens and calling a contract on an external chain. It correctly uses the_transferZETAfunction and emits appropriate events. Ensure that reentrancy guards are used to prevent potential vulnerabilities.+ modifier nonReentrant() { + require(!locked, "Reentrant call"); + locked = true; + _; + locked = false; + }
Line range hint
127-138: AdddepositAndCallfunction for ERC20 tokens.The
depositAndCallfunction allows for depositing ERC20 tokens and calling a contract on an external chain. Ensure that reentrancy guards are used to prevent potential vulnerabilities.+ modifier nonReentrant() { + require(!locked, "Reentrant call"); + locked = true; + _; + locked = false; + }
Line range hint
133-144: AdddepositAndCallfunction for ZETA tokens.The
depositAndCallfunction allows for depositing ZETA tokens and calling a contract on ZEVM. It uses the_transferZETAfunction and correctly handles the transfer and call sequence. Ensure that reentrancy guards are used to prevent potential vulnerabilities.+ modifier nonReentrant() { + require(!locked, "Reentrant call"); + locked = true; + _; + locked = false; + }Tools
GitHub Check: Slither
[warning] 148-148: Conformance to Solidity naming conventions
Parameter GatewayEVM.setConnector(address)._zetaConnector (contracts/prototypes/evm/GatewayEVM.sol#148) is not in mixedCase
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (48)
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/igatewayevm.sol/igatewayevm.gois excluded by!pkg/**pkg/contracts/prototypes/evm/igatewayevm.sol/igatewayevmerrors.gois excluded by!pkg/**pkg/contracts/prototypes/evm/igatewayevm.sol/igatewayevmevents.gois excluded by!pkg/**pkg/contracts/prototypes/evm/ireceiverevm.sol/ireceiverevmevents.gois excluded by!pkg/**pkg/contracts/prototypes/evm/zetaconnectornew.sol/zetaconnectornew.gois excluded by!pkg/**pkg/contracts/prototypes/zevm/gatewayzevm.sol/gatewayzevm.gois excluded by!pkg/**pkg/contracts/prototypes/zevm/igatewayzevm.sol/igatewayzevm.gois excluded by!pkg/**pkg/contracts/prototypes/zevm/igatewayzevm.sol/igatewayzevmerrors.gois excluded by!pkg/**pkg/contracts/prototypes/zevm/igatewayzevm.sol/igatewayzevmevents.gois excluded by!pkg/**pkg/contracts/prototypes/zevm/senderzevm.sol/senderzevm.gois excluded by!pkg/**typechain-types/contracts/prototypes/evm/GatewayEVM.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/evm/GatewayEVMUpgradeTest.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/evm/IGatewayEVM.sol/IGatewayEVM.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/evm/IGatewayEVM.sol/IGatewayEVMErrors.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/evm/IGatewayEVM.sol/IGatewayEVMEvents.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/evm/IGatewayEVM.sol/index.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/evm/IReceiverEVM.sol/IReceiverEVMEvents.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/evm/IReceiverEVM.sol/index.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/evm/ZetaConnectorNew.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/evm/index.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/zevm/GatewayZEVM.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/zevm/IGatewayZEVM.sol/IGatewayZEVM.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/zevm/IGatewayZEVM.sol/IGatewayZEVMErrors.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/zevm/IGatewayZEVM.sol/IGatewayZEVMEvents.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/zevm/IGatewayZEVM.sol/index.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/zevm/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/IGatewayEVM.sol/IGatewayEVMErrors__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/IGatewayEVM.sol/IGatewayEVMEvents__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/IGatewayEVM.sol/IGatewayEVM__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/IGatewayEVM.sol/index.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/IReceiverEVM.sol/IReceiverEVMEvents__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/IReceiverEVM.sol/index.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/ZetaConnectorNew__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/prototypes/zevm/IGatewayZEVM.sol/IGatewayZEVMErrors__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/zevm/IGatewayZEVM.sol/IGatewayZEVMEvents__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/zevm/IGatewayZEVM.sol/IGatewayZEVM__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/zevm/IGatewayZEVM.sol/index.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/zevm/SenderZEVM__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/zevm/index.tsis excluded by!typechain-types/**typechain-types/index.tsis excluded by!typechain-types/**
Files selected for processing (13)
- contracts/prototypes/evm/ERC20CustodyNew.sol (1 hunks)
- contracts/prototypes/evm/GatewayEVM.sol (6 hunks)
- contracts/prototypes/evm/GatewayEVMUpgradeTest.sol (2 hunks)
- contracts/prototypes/evm/IGatewayEVM.sol (1 hunks)
- contracts/prototypes/evm/IReceiverEVM.sol (1 hunks)
- contracts/prototypes/evm/ZetaConnectorNew.sol (1 hunks)
- contracts/prototypes/zevm/GatewayZEVM.sol (3 hunks)
- contracts/prototypes/zevm/IGatewayZEVM.sol (2 hunks)
- contracts/prototypes/zevm/SenderZEVM.sol (1 hunks)
- testFoundry/GatewayEVM.t.sol (5 hunks)
- testFoundry/GatewayEVMUpgrade.t.sol (3 hunks)
- testFoundry/GatewayEVMZEVM.t.sol (5 hunks)
- testFoundry/GatewayZEVM.t.sol (3 hunks)
Files skipped from review as they are similar to previous changes (5)
- contracts/prototypes/evm/ZetaConnectorNew.sol
- testFoundry/GatewayEVM.t.sol
- testFoundry/GatewayEVMUpgrade.t.sol
- testFoundry/GatewayEVMZEVM.t.sol
- testFoundry/GatewayZEVM.t.sol
Additional context used
Path-based instructions (8)
contracts/prototypes/evm/IReceiverEVM.sol (1)
Pattern
contracts/**: Review the Solidity contracts for security vulnerabilities and best practices.contracts/prototypes/evm/IGatewayEVM.sol (1)
Pattern
contracts/**: Review the Solidity contracts for security vulnerabilities and best practices.contracts/prototypes/zevm/SenderZEVM.sol (1)
Pattern
contracts/**: Review the Solidity contracts for security vulnerabilities and best practices.contracts/prototypes/zevm/IGatewayZEVM.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.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.contracts/prototypes/evm/GatewayEVMUpgradeTest.sol (1)
Pattern
contracts/**: Review the Solidity contracts for security vulnerabilities and best practices.
GitHub Check: Slither
contracts/prototypes/zevm/GatewayZEVM.sol
[notice] 25-25: Missing zero address validation
GatewayZEVM.initialize(address)._zetaToken (contracts/prototypes/zevm/GatewayZEVM.sol#25) lacks a zero-check on :
- zetaToken = _zetaToken (contracts/prototypes/zevm/GatewayZEVM.sol#28)
[warning] 25-25: Conformance to Solidity naming conventions
Parameter GatewayZEVM.initialize(address)._zetaToken (contracts/prototypes/zevm/GatewayZEVM.sol#25) is not in mixedCase
[warning] 48-53: Low-level calls
Low level call in GatewayZEVM._transferZETA(uint256,address) (contracts/prototypes/zevm/GatewayZEVM.sol#48-53):
- (sent,None) = to.call{value: amount}() (contracts/prototypes/zevm/GatewayZEVM.sol#51)contracts/prototypes/evm/GatewayEVM.sol
[warning] 36-36: Conformance to Solidity naming conventions
Parameter GatewayEVM.initialize(address,address)._tssAddress (contracts/prototypes/evm/GatewayEVM.sol#36) is not in mixedCase
[warning] 36-36: Conformance to Solidity naming conventions
Parameter GatewayEVM.initialize(address,address)._zetaToken (contracts/prototypes/evm/GatewayEVM.sol#36) is not in mixedCase
[warning] 148-148: Conformance to Solidity naming conventions
Parameter GatewayEVM.setConnector(address)._zetaConnector (contracts/prototypes/evm/GatewayEVM.sol#148) is not in mixedCasecontracts/prototypes/evm/GatewayEVMUpgradeTest.sol
[notice] 27-27: Missing zero address validation
GatewayEVMUpgradeTest.initialize(address,address)._zetaToken (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#27) lacks a zero-check on :
- zetaToken = _zetaToken (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#34)
[warning] 27-27: Conformance to Solidity naming conventions
Parameter GatewayEVMUpgradeTest.initialize(address,address)._zetaToken (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#27) is not in mixedCase
[warning] 27-27: Conformance to Solidity naming conventions
Parameter GatewayEVMUpgradeTest.initialize(address,address)._tssAddress (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#27) is not in mixedCase
[notice] 118-125: Reentrancy vulnerabilities
Reentrancy in GatewayEVMUpgradeTest.depositAndCall(address,bytes) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#118-125):
External calls:
- (deposited,None) = tssAddress.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#120)
Event emitted after the call(s):
- Deposit(msg.sender,receiver,msg.value,address(0),payload) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#124)
[notice] 128-138: Reentrancy vulnerabilities
Reentrancy in GatewayEVMUpgradeTest.depositAndCall(address,uint256,address,bytes) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#128-138):
External calls:
- IERC20(asset).safeTransferFrom(msg.sender,address(destination),amount) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#135)
Event emitted after the call(s):
- Deposit(msg.sender,receiver,amount,asset,payload) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#137)
[notice] 105-115: Reentrancy vulnerabilities
Reentrancy in GatewayEVMUpgradeTest.deposit(address,uint256,address) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#105-115):
External calls:
- IERC20(asset).safeTransferFrom(msg.sender,address(destination),amount) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#112)
Event emitted after the call(s):
- Deposit(msg.sender,receiver,amount,asset,) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#114)
[notice] 95-102: Reentrancy vulnerabilities
Reentrancy in GatewayEVMUpgradeTest.deposit(address) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#95-102):
External calls:
- (deposited,None) = tssAddress.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#97)
Event emitted after the call(s):
- Deposit(msg.sender,receiver,msg.value,address(0),) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#101)
[warning] 95-102: Boolean equality
GatewayEVMUpgradeTest.deposit(address) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#95-102) compares to a boolean constant:
-deposited == false (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#99)
[warning] 118-125: Boolean equality
GatewayEVMUpgradeTest.depositAndCall(address,bytes) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#118-125) compares to a boolean constant:
-deposited == false (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#122)
[warning] 118-125: Low-level calls
Low level call in GatewayEVMUpgradeTest.depositAndCall(address,bytes) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#118-125):
- (deposited,None) = tssAddress.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#120)
[warning] 95-102: Low-level calls
Low level call in GatewayEVMUpgradeTest.deposit(address) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#95-102):
- (deposited,None) = tssAddress.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#97)
[warning] 150-150: Conformance to Solidity naming conventions
Parameter GatewayEVMUpgradeTest.setConnector(address)._zetaConnector (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#150) is not in mixedCase
Additional comments not posted (42)
contracts/prototypes/evm/IReceiverEVM.sol (4)
5-5: LGTM!The event
ReceivedPayableis correctly defined.
6-6: LGTM!The event
ReceivedNonPayableis correctly defined.
7-7: LGTM!The event
ReceivedERC20is correctly defined.
8-8: LGTM!The event
ReceivedNoParamsis correctly defined.contracts/prototypes/evm/IGatewayEVM.sol (13)
Line range hint
4-4:
LGTM!The event
Executedis correctly defined.
Line range hint
5-5:
LGTM!The event
ExecutedWithERC20is correctly defined.
Line range hint
6-6:
LGTM!The event
Depositis correctly defined.
Line range hint
7-7:
LGTM!The event
Callis correctly defined.
Line range hint
10-10:
LGTM!The error
ExecutionFailedis correctly defined.
Line range hint
11-11:
LGTM!The error
DepositFailedis correctly defined.
Line range hint
12-12:
LGTM!The error
InsufficientETHAmountis correctly defined.
Line range hint
13-13:
LGTM!The error
InsufficientERC20Amountis correctly defined.
Line range hint
14-14:
LGTM!The error
ZeroAddressis correctly defined.
Line range hint
15-15:
LGTM!The error
ApprovalFailedis correctly defined.
Line range hint
16-16:
LGTM!The error
CustodyInitializedis correctly defined.
29-29: LGTM!The function
executeis correctly defined.
Line range hint
17-27:
Verify the impact of removing the return type.The
executeWithERC20function no longer returns a value. Ensure that this change does not break existing functionality or expectations.Verification successful
Verification Successful: Removal of Return Type in
executeWithERC20The removal of the return type from the
executeWithERC20function in theIGatewayEVMinterface does not break existing functionality or expectations. This change is consistently reflected across the codebase.
- The function is referenced in TypeScript and Go files, and the updated signature without a return type is correctly represented.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing the return type from `executeWithERC20`. # Test: Search for the function usage. Expect: No broken functionality or expectations. rg --type solidity -A 5 $'executeWithERC20'Length of output: 75
Script:
#!/bin/bash # Description: Verify the impact of removing the return type from `executeWithERC20`. # Find all Solidity files and search for the function usage. fd -e sol -x rg 'executeWithERC20' {}Length of output: 25523
contracts/prototypes/zevm/SenderZEVM.sol (3)
5-5: LGTM!The import statement change improves clarity and consistency.
Line range hint
14-19:
LGTM!The function
callReceiveris correctly defined.
Line range hint
22-31:
LGTM!The function
withdrawAndCallReceiveris correctly defined.contracts/prototypes/zevm/IGatewayZEVM.sol (10)
Line range hint
6-8:
LGTM!The function
withdrawis correctly defined.
Line range hint
10-12:
LGTM!The function
withdrawAndCallis correctly defined.
Line range hint
14-15:
LGTM!The function
callis correctly defined.
Line range hint
17-20:
LGTM!The function
depositis correctly defined.
Line range hint
22-27:
LGTM!The function
executeis correctly defined.
Line range hint
29-34:
LGTM!The function
depositAndCallis correctly defined.
37-37: LGTM!The event
Callis correctly defined.
49-49: LGTM!The error
ZetaTokenTransferFailedis correctly defined.
50-50: LGTM!The error
FailedZetaSentis correctly defined.
38-38: Verify the impact of adding thezrc20parameter.The
Withdrawalevent now includes an additionaladdress zrc20parameter. Ensure that this change is correctly handled in the codebase.contracts/prototypes/evm/ERC20CustodyNew.sol (1)
5-5: Update import statement to reflect new dependency.The import statement has been updated to import
IGatewayEVM.solinstead ofinterfaces.sol. This change likely reflects a shift in the dependency for interfacing with a gateway in the EVM environment. Ensure that the new interface is correctly integrated and used in the contract.contracts/prototypes/zevm/GatewayZEVM.sol (4)
7-12: Update import statements to include new dependencies.The import statements now include
ReentrancyGuard.solandIWZETA.sol. These additions are necessary for the new functionalities introduced in the contract.
18-18: Add public variable for Zeta token address.The
zetaTokenaddress is added as a public variable. This is necessary for handling Zeta tokens within the contract.
Line range hint
33-47: Rename_withdrawfunction to_withdrawZRC20.The
_withdrawfunction has been renamed to_withdrawZRC20to clarify its purpose. This change improves the readability and maintainability of the code.Tools
GitHub Check: Slither
[notice] 25-25: Missing zero address validation
GatewayZEVM.initialize(address)._zetaToken (contracts/prototypes/zevm/GatewayZEVM.sol#25) lacks a zero-check on :
- zetaToken = _zetaToken (contracts/prototypes/zevm/GatewayZEVM.sol#28)
[warning] 25-25: Conformance to Solidity naming conventions
Parameter GatewayZEVM.initialize(address)._zetaToken (contracts/prototypes/zevm/GatewayZEVM.sol#25) is not in mixedCase
67-71: Addwithdrawfunction for ZETA tokens.The
withdrawfunction facilitates the withdrawal of ZETA tokens to external chains. It correctly uses the_transferZETAfunction and emits appropriate events. Ensure that reentrancy guards are used to prevent potential vulnerabilities.+ modifier nonReentrant() { + require(!locked, "Reentrant call"); + locked = true; + _; + locked = false; + }Likely invalid or redundant comment.
contracts/prototypes/evm/GatewayEVM.sol (5)
9-9: Add import statement forIGatewayEVM.sol.The import statement for
IGatewayEVM.solhas been added. This is necessary for the new functionalities introduced in the contract.
26-30: Add public variables for Zeta connector and token addresses.The
zetaConnectorandzetaTokenaddresses are added as public variables. This is necessary for handling Zeta tokens within the contract.
Line range hint
56-58: Updatewithdrawfunction to use_withdrawZRC20.The
withdrawfunction has been updated to use the_withdrawZRC20function and emit appropriate events. This change improves the readability and maintainability of the code.Tools
GitHub Check: Slither
[warning] 36-36: Conformance to Solidity naming conventions
Parameter GatewayEVM.initialize(address,address)._tssAddress (contracts/prototypes/evm/GatewayEVM.sol#36) is not in mixedCase
[warning] 36-36: Conformance to Solidity naming conventions
Parameter GatewayEVM.initialize(address,address)._zetaToken (contracts/prototypes/evm/GatewayEVM.sol#36) is not in mixedCase
Line range hint
62-65: UpdatewithdrawAndCallfunction to use_withdrawZRC20.The
withdrawAndCallfunction has been updated to use the_withdrawZRC20function and emit appropriate events. This change improves the readability and maintainability of the code.Tools
GitHub Check: Slither
[warning] 36-36: Conformance to Solidity naming conventions
Parameter GatewayEVM.initialize(address,address)._tssAddress (contracts/prototypes/evm/GatewayEVM.sol#36) is not in mixedCase
[warning] 36-36: Conformance to Solidity naming conventions
Parameter GatewayEVM.initialize(address,address)._zetaToken (contracts/prototypes/evm/GatewayEVM.sol#36) is not in mixedCase
36-45: Add zero address validation for_zetaToken.The
initializefunction now accepts an address parameter_zetaTokenand setszetaTokento this value. However, it lacks a zero address validation for_zetaToken.+ require(_zetaToken != address(0), "Zero address not allowed");Likely invalid or redundant comment.
Tools
GitHub Check: Slither
[warning] 36-36: Conformance to Solidity naming conventions
Parameter GatewayEVM.initialize(address,address)._tssAddress (contracts/prototypes/evm/GatewayEVM.sol#36) is not in mixedCase
[warning] 36-36: Conformance to Solidity naming conventions
Parameter GatewayEVM.initialize(address,address)._zetaToken (contracts/prototypes/evm/GatewayEVM.sol#36) is not in mixedCasecontracts/prototypes/evm/GatewayEVMUpgradeTest.sol (2)
9-9: Add import statement forIGatewayEVM.sol.The import statement for
IGatewayEVM.solhas been added. This is necessary for the new functionalities introduced in the contract.
20-21: Add public variables for Zeta connector and token addresses.The
zetaConnectorandzetaTokenaddresses are added as public variables. This is necessary for handling Zeta tokens within the contract.
| function initialize(address _zetaToken) public initializer { | ||
| __Ownable_init(); | ||
| __UUPSUpgradeable_init(); | ||
| zetaToken = _zetaToken; |
There was a problem hiding this comment.
Add zero address validation for _zetaToken.
The initialize function now accepts an address parameter _zetaToken and sets zetaToken to this value. However, it lacks a zero address validation for _zetaToken.
+ require(_zetaToken != address(0), "Zero address not allowed");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 initialize(address _zetaToken) public initializer { | |
| __Ownable_init(); | |
| __UUPSUpgradeable_init(); | |
| zetaToken = _zetaToken; | |
| function initialize(address _zetaToken) public initializer { | |
| require(_zetaToken != address(0), "Zero address not allowed"); | |
| __Ownable_init(); | |
| __UUPSUpgradeable_init(); | |
| zetaToken = _zetaToken; |
Tools
GitHub Check: Slither
[notice] 25-25: Missing zero address validation
GatewayZEVM.initialize(address)._zetaToken (contracts/prototypes/zevm/GatewayZEVM.sol#25) lacks a zero-check on :
- zetaToken = _zetaToken (contracts/prototypes/zevm/GatewayZEVM.sol#28)
[warning] 25-25: Conformance to Solidity naming conventions
Parameter GatewayZEVM.initialize(address)._zetaToken (contracts/prototypes/zevm/GatewayZEVM.sol#25) is not in mixedCase
| function depositAndCall( | ||
| zContext calldata context, | ||
| uint256 amount, | ||
| address target, | ||
| bytes calldata message | ||
| ) external { | ||
| if (msg.sender != FUNGIBLE_MODULE_ADDRESS) revert CallerIsNotFungibleModule(); | ||
| if (target == FUNGIBLE_MODULE_ADDRESS || target == address(this)) revert InvalidTarget(); | ||
|
|
||
| _transferZETA(amount, target); | ||
| zContract(target).onCrossChainCall(context, zetaToken, amount, message); | ||
| } |
There was a problem hiding this comment.
Add depositAndCall function for ZETA tokens.
The depositAndCall function allows for depositing ZETA tokens and calling a contract on ZEVM. It uses the _transferZETA function and correctly handles the transfer and call sequence. Ensure that reentrancy guards are used to prevent potential vulnerabilities.
+ modifier nonReentrant() {
+ require(!locked, "Reentrant call");
+ locked = true;
+ _;
+ locked = false;
+ }Committable suggestion was skipped due to low confidence.
| // Withdraw ZETA and call smart contract on external chain | ||
| function withdrawAndCall(uint256 amount, bytes calldata message) external nonReentrant { | ||
| _transferZETA(amount, FUNGIBLE_MODULE_ADDRESS); | ||
| emit Withdrawal(msg.sender, address(0), abi.encodePacked(FUNGIBLE_MODULE_ADDRESS), amount, 0, 0, message); |
There was a problem hiding this comment.
Add withdrawAndCall function for ZETA tokens.
The withdrawAndCall function allows for the withdrawal of ZETA tokens and calling a contract on an external chain. It correctly uses the _transferZETA function and emits appropriate events. Ensure that reentrancy guards are used to prevent potential vulnerabilities.
+ modifier nonReentrant() {
+ require(!locked, "Reentrant call");
+ locked = true;
+ _;
+ locked = false;
+ }Committable suggestion was skipped due to low confidence.
| function _transferZETA(uint256 amount, address to) internal { | ||
| if (!IWETH9(zetaToken).transferFrom(msg.sender, address(this), amount)) revert ZetaTokenTransferFailed(); | ||
| IWETH9(zetaToken).withdraw(amount); | ||
| (bool sent, ) = to.call{value: amount}(""); | ||
| if (!sent) revert FailedZetaSent(); | ||
| } |
There was a problem hiding this comment.
Add _transferZETA function to handle ZETA token transfers.
The _transferZETA function handles the transfer of ZETA tokens. It includes low-level calls which can be prone to security issues. Consider using send or transfer for safer ETH transfers, and ensure that all state changes occur before calling external contracts.
+ require(sent, "Failed to send Zeta");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 _transferZETA(uint256 amount, address to) internal { | |
| if (!IWETH9(zetaToken).transferFrom(msg.sender, address(this), amount)) revert ZetaTokenTransferFailed(); | |
| IWETH9(zetaToken).withdraw(amount); | |
| (bool sent, ) = to.call{value: amount}(""); | |
| if (!sent) revert FailedZetaSent(); | |
| } | |
| function _transferZETA(uint256 amount, address to) internal { | |
| if (!IWETH9(zetaToken).transferFrom(msg.sender, address(this), amount)) revert ZetaTokenTransferFailed(); | |
| IWETH9(zetaToken).withdraw(amount); | |
| (bool sent, ) = to.call{value: amount}(""); | |
| require(sent, "Failed to send Zeta"); | |
| } |
Tools
GitHub Check: Slither
[warning] 48-53: Low-level calls
Low level call in GatewayZEVM._transferZETA(uint256,address) (contracts/prototypes/zevm/GatewayZEVM.sol#48-53):
- (sent,None) = to.call{value: amount}() (contracts/prototypes/zevm/GatewayZEVM.sol#51)
| function setConnector(address _zetaConnector) external { | ||
| if (zetaConnector != address(0)) revert CustodyInitialized(); | ||
| zetaConnector = _zetaConnector; | ||
| } |
There was a problem hiding this comment.
Add zero address validation for _zetaConnector.
The setConnector function sets the zetaConnector. It includes a check to ensure that the connector is not already set, which is a good practice to prevent accidental overwrites. However, it lacks a zero address validation for _zetaConnector.
+ if (_zetaConnector == 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 setConnector(address _zetaConnector) external { | |
| if (zetaConnector != address(0)) revert CustodyInitialized(); | |
| zetaConnector = _zetaConnector; | |
| } | |
| function setConnector(address _zetaConnector) external { | |
| if (zetaConnector != address(0)) revert CustodyInitialized(); | |
| if (_zetaConnector == address(0)) revert ZeroAddress(); | |
| zetaConnector = _zetaConnector; | |
| } |
Tools
GitHub Check: Slither
[warning] 150-150: Conformance to Solidity naming conventions
Parameter GatewayEVMUpgradeTest.setConnector(address)._zetaConnector (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#150) is not in mixedCase
| // Deposit ERC20 tokens to custody and call an omnichain smart contract | ||
| function depositAndCall(address receiver, uint256 amount, address asset, bytes calldata payload) external { | ||
| if (amount == 0) revert InsufficientERC20Amount(); | ||
|
|
||
| address destination = address(custody); | ||
| if (asset == zetaToken) { | ||
| destination = address(zetaConnector); | ||
| } | ||
| IERC20(asset).safeTransferFrom(msg.sender, address(destination), amount); | ||
|
|
||
| if (sent == false) revert SendFailed(); | ||
| emit Deposit(msg.sender, receiver, amount, asset, payload); | ||
| } |
There was a problem hiding this comment.
Add depositAndCall function for ERC20 tokens.
The depositAndCall function allows for depositing ERC20 tokens and calling a contract on an external chain. Ensure that reentrancy guards are used to prevent potential vulnerabilities.
+ modifier nonReentrant() {
+ require(!locked, "Reentrant call");
+ locked = true;
+ _;
+ locked = false;
+ }Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: Slither
[notice] 128-138: Reentrancy vulnerabilities
Reentrancy in GatewayEVMUpgradeTest.depositAndCall(address,uint256,address,bytes) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#128-138):
External calls:
- IERC20(asset).safeTransferFrom(msg.sender,address(destination),amount) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#135)
Event emitted after the call(s):
- Deposit(msg.sender,receiver,amount,asset,payload) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#137)
| ) public returns (bytes memory) { | ||
| if (amount == 0) revert InsufficientETHAmount(); | ||
| // Approve the target contract to spend the tokens | ||
| if(!IERC20(token).approve(to, 0)) revert ApprovalFailed(); | ||
| 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(!IERC20(token).approve(to, 0)) revert ApprovalFailed(); | ||
| if(!resetApproval(token, to)) revert ApprovalFailed(); | ||
|
|
||
| // Transfer any remaining tokens back to the custody contract | ||
| // Transfer any remaining tokens back to the custody/connector contract | ||
| uint256 remainingBalance = IERC20(token).balanceOf(address(this)); | ||
| if (remainingBalance > 0) { | ||
| IERC20(token).safeTransfer(address(custody), remainingBalance); | ||
| address destination = address(custody); | ||
| if (token == zetaToken) { | ||
| destination = address(zetaConnector); | ||
| } | ||
| IERC20(token).safeTransfer(address(destination), remainingBalance); | ||
| } |
There was a problem hiding this comment.
Add executeWithERC20 function for ERC20 tokens.
The executeWithERC20 function handles the execution of calls with ERC20 token transfers. It includes setting and resetting approvals, which is crucial for security. Ensure that reentrancy guards are used to prevent potential vulnerabilities.
+ modifier nonReentrant() {
+ require(!locked, "Reentrant call");
+ locked = true;
+ _;
+ locked = false;
+ }Committable suggestion was skipped due to low confidence.
| // Deposit ETH to tss and call an omnichain smart contract | ||
| function depositAndCall(address receiver, bytes calldata payload) external payable { | ||
| if (msg.value == 0) revert InsufficientETHAmount(); | ||
| (bool deposited, ) = tssAddress.call{value: msg.value}(""); | ||
|
|
||
| (bool sent, ) = tssAddress.call{value: msg.value}(""); | ||
| if (deposited == false) revert DepositFailed(); | ||
|
|
||
| emit Deposit(msg.sender, receiver, msg.value, address(0), payload); | ||
| } |
There was a problem hiding this comment.
Add depositAndCall function for ETH.
The depositAndCall function allows for the deposit of ETH to a specified receiver and calling a contract on an external chain. Ensure that reentrancy guards are used to prevent potential vulnerabilities.
+ modifier nonReentrant() {
+ require(!locked, "Reentrant call");
+ locked = true;
+ _;
+ locked = false;
+ }Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: Slither
[notice] 118-125: Reentrancy vulnerabilities
Reentrancy in GatewayEVMUpgradeTest.depositAndCall(address,bytes) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#118-125):
External calls:
- (deposited,None) = tssAddress.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#120)
Event emitted after the call(s):
- Deposit(msg.sender,receiver,msg.value,address(0),payload) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#124)
[warning] 118-125: Boolean equality
GatewayEVMUpgradeTest.depositAndCall(address,bytes) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#118-125) compares to a boolean constant:
-deposited == false (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#122)
[warning] 118-125: Low-level calls
Low level call in GatewayEVMUpgradeTest.depositAndCall(address,bytes) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#118-125):
- (deposited,None) = tssAddress.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#120)
| function deposit(address receiver) external payable { | ||
| if (msg.value == 0) revert InsufficientETHAmount(); | ||
| (bool deposited, ) = tssAddress.call{value: msg.value}(""); | ||
|
|
||
| emit SendERC20(recipient, token, amount); | ||
| if (deposited == false) revert DepositFailed(); | ||
|
|
||
| emit Deposit(msg.sender, receiver, msg.value, address(0), ""); |
There was a problem hiding this comment.
Add deposit function for ETH.
The deposit function allows for the deposit of ETH to a specified receiver. It includes checks for zero deposits and handles the actual transfer. Consider using high-level calls instead of low-level ones to improve security.
- (bool deposited, ) = tssAddress.call{value: msg.value}("");
+ require(tssAddress.send(msg.value), "Deposit failed");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 deposit(address receiver) external payable { | |
| if (msg.value == 0) revert InsufficientETHAmount(); | |
| (bool deposited, ) = tssAddress.call{value: msg.value}(""); | |
| emit SendERC20(recipient, token, amount); | |
| if (deposited == false) revert DepositFailed(); | |
| emit Deposit(msg.sender, receiver, msg.value, address(0), ""); | |
| function deposit(address receiver) external payable { | |
| if (msg.value == 0) revert InsufficientETHAmount(); | |
| require(tssAddress.send(msg.value), "Deposit failed"); | |
| emit Deposit(msg.sender, receiver, msg.value, address(0), ""); |
Tools
GitHub Check: Slither
[notice] 95-102: Reentrancy vulnerabilities
Reentrancy in GatewayEVMUpgradeTest.deposit(address) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#95-102):
External calls:
- (deposited,None) = tssAddress.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#97)
Event emitted after the call(s):
- Deposit(msg.sender,receiver,msg.value,address(0),) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#101)
[warning] 95-102: Boolean equality
GatewayEVMUpgradeTest.deposit(address) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#95-102) compares to a boolean constant:
-deposited == false (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#99)
[warning] 95-102: Low-level calls
Low level call in GatewayEVMUpgradeTest.deposit(address) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#95-102):
- (deposited,None) = tssAddress.call{value: msg.value}() (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#97)
| // Deposit ERC20 tokens to custody | ||
| function deposit(address receiver, uint256 amount, address asset) external { | ||
| if (amount == 0) revert InsufficientERC20Amount(); | ||
|
|
||
| address destination = address(custody); | ||
| if (asset == zetaToken) { | ||
| destination = address(zetaConnector); | ||
| } | ||
| IERC20(asset).safeTransferFrom(msg.sender, address(destination), amount); | ||
|
|
||
| emit Deposit(msg.sender, receiver, amount, asset, ""); | ||
| } |
There was a problem hiding this comment.
Add deposit function for ERC20 tokens.
The deposit function allows for the deposit of ERC20 tokens to a specified receiver. It includes checks for zero deposits and correct handling of the token transfer. Ensure that reentrancy guards are used to prevent potential vulnerabilities.
+ modifier nonReentrant() {
+ require(!locked, "Reentrant call");
+ locked = true;
+ _;
+ locked = false;
+ }Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: Slither
[notice] 105-115: Reentrancy vulnerabilities
Reentrancy in GatewayEVMUpgradeTest.deposit(address,uint256,address) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#105-115):
External calls:
- IERC20(asset).safeTransferFrom(msg.sender,address(destination),amount) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#112)
Event emitted after the call(s):
- Deposit(msg.sender,receiver,amount,asset,) (contracts/prototypes/evm/GatewayEVMUpgradeTest.sol#114)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (8)
- contracts/prototypes/evm/GatewayEVM.sol (6 hunks)
- contracts/prototypes/evm/IZetaNonEthNew.sol (1 hunks)
- contracts/prototypes/evm/ZetaConnectorNewBase.sol (1 hunks)
- contracts/prototypes/evm/ZetaConnectorNewEth.sol (1 hunks)
- contracts/prototypes/evm/ZetaConnectorNewNonEth.sol (1 hunks)
- testFoundry/GatewayEVM.t.sol (6 hunks)
- testFoundry/GatewayEVMUpgrade.t.sol (3 hunks)
- testFoundry/GatewayEVMZEVM.t.sol (5 hunks)
Files skipped from review as they are similar to previous changes (4)
- contracts/prototypes/evm/GatewayEVM.sol
- testFoundry/GatewayEVM.t.sol
- testFoundry/GatewayEVMUpgrade.t.sol
- testFoundry/GatewayEVMZEVM.t.sol
Additional context used
Path-based instructions (4)
contracts/prototypes/evm/IZetaNonEthNew.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/ZetaConnectorNewNonEth.sol (1)
Pattern
contracts/**: Review the Solidity contracts for security vulnerabilities and best practices.contracts/prototypes/evm/ZetaConnectorNewEth.sol (1)
Pattern
contracts/**: Review the Solidity contracts for security vulnerabilities and best practices.
Additional comments not posted (21)
contracts/prototypes/evm/IZetaNonEthNew.sol (3)
1-3: SPDX license identifier and pragma directive are correctly defined.The SPDX license identifier and pragma directive are correctly defined.
4-5: Import statement is correct.The import statement for
IERC20is correct and necessary for extending theIERC20interface.
6-13: Interface definition is correct.The interface
IZetaNonEthNewis correctly defined and follows best practices. The comments provide clear documentation of the interface purpose.contracts/prototypes/evm/ZetaConnectorNewBase.sol (6)
1-3: SPDX license identifier and pragma directive are correctly defined.The SPDX license identifier and pragma directive are correctly defined.
4-8: Import statements are correct.The import statements for
IERC20,IGatewayEVM,SafeERC20, andReentrancyGuardare correct and necessary for the contract functionality.
9-16: Contract definition and state variables are correct.The contract
ZetaConnectorNewBaseis defined as abstract. It extendsReentrancyGuardand usesSafeERC20for safe token operations. The state variablesgatewayandzetaTokenare defined as immutable.
17-19: Events are correctly defined.The events
WithdrawandWithdrawAndCallare correctly defined and provide necessary information for logging and debugging.
20-26: Constructor is correctly defined.The constructor initializes the
gatewayandzetaTokenstate variables and checks for zero addresses.
28-33: Function declarations are correct.The contract declares three virtual functions:
withdraw,withdrawAndCall, andreceiveTokens. The use ofvirtualkeyword indicates that these functions are meant to be overridden in derived contracts.contracts/prototypes/evm/ZetaConnectorNewNonEth.sol (6)
1-3: SPDX license identifier and pragma directive are correctly defined.The SPDX license identifier and pragma directive are correctly defined.
4-7: Import statements are correct.The import statements for
ZetaConnectorNewBase,IZetaNonEthNew, andERC20Burnableare correct and necessary for the contract functionality.
8-11: Contract definition and constructor are correct.The contract
ZetaConnectorNewNonEthextendsZetaConnectorNewBaseand the constructor calls the base constructor.
13-17: Withdraw function is correctly defined.The
withdrawfunction mintszetaTokento the destination address and emits aWithdrawevent.
19-27: WithdrawAndCall function is correctly defined.The
withdrawAndCallfunction mintszetaTokento the gateway, forwards the call to the gateway, and emits aWithdrawAndCallevent.
30-34: ReceiveTokens function is correctly defined.The
receiveTokensfunction burns the tokens from the sender.contracts/prototypes/evm/ZetaConnectorNewEth.sol (6)
1-3: SPDX license identifier and pragma directive are correctly defined.The SPDX license identifier and pragma directive are correctly defined.
4-7: Import statements are correct.The import statements for
ZetaConnectorNewBase,IERC20, andSafeERC20are correct and necessary for the contract functionality.
8-13: Contract definition and constructor are correct.The contract
ZetaConnectorNewEthextendsZetaConnectorNewBaseand the constructor calls the base constructor.
15-19: Withdraw function is correctly defined.The
withdrawfunction transferszetaTokento the destination address and emits aWithdrawevent.
21-29: WithdrawAndCall function is correctly defined.The
withdrawAndCallfunction transferszetaTokento the gateway, forwards the call to the gateway, and emits aWithdrawAndCallevent.
32-36: ReceiveTokens function is correctly defined.The
receiveTokensfunction transfers tokens from the sender to the contract.
| function transferToAssetHandler(address token, uint256 amount) private { | ||
| if (token == zetaToken) { // transfer to connector | ||
| // approve connector to handle tokens depending on connector version (eg. lock or burn) | ||
| IERC20(token).approve(zetaConnector, amount); | ||
| // send tokens to connector | ||
| ZetaConnectorNewBase(zetaConnector).receiveTokens(amount); | ||
| } else { // transfer to custody | ||
| IERC20(token).safeTransfer(custody, amount); | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Unused return Medium
| function transferFromToAssetHandler(address from, address token, uint256 amount) private { | ||
| if (token == zetaToken) { // transfer to connector | ||
| // transfer amount to gateway | ||
| IERC20(token).safeTransferFrom(from, address(this), amount); | ||
| // approve connector to handle tokens depending on connector version (eg. lock or burn) | ||
| IERC20(token).approve(zetaConnector, amount); | ||
| // send tokens to connector | ||
| ZetaConnectorNewBase(zetaConnector).receiveTokens(amount); | ||
| } else { // transfer to custody | ||
| IERC20(token).safeTransferFrom(from, custody, amount); | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Unused return Medium
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (18)
pkg/contracts/prototypes/evm/gatewayevm.sol/gatewayevm.gois excluded by!pkg/**pkg/contracts/prototypes/evm/izetanonethnew.sol/izetanonethnew.gois excluded by!pkg/**pkg/contracts/prototypes/evm/zetaconnectornewbase.sol/zetaconnectornewbase.gois excluded by!pkg/**pkg/contracts/prototypes/evm/zetaconnectorneweth.sol/zetaconnectorneweth.gois excluded by!pkg/**pkg/contracts/prototypes/evm/zetaconnectornewnoneth.sol/zetaconnectornewnoneth.gois excluded by!pkg/**typechain-types/contracts/prototypes/evm/IZetaNonEthNew.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/evm/ZetaConnectorNewBase.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/evm/ZetaConnectorNewEth.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/evm/ZetaConnectorNewNonEth.tsis excluded by!typechain-types/**typechain-types/contracts/prototypes/evm/index.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/GatewayEVM__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/IZetaNonEthNew__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/ZetaConnectorNewEth__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/ZetaConnectorNewNonEth__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/evm/index.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 (1)
- test/prototypes/GatewayEVMUniswap.spec.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- test/prototypes/GatewayEVMUniswap.spec.ts
|
|
||
| /// @custom:oz-upgrades-unsafe-allow constructor | ||
| constructor() { | ||
| _disableInitializers(); | ||
| } | ||
|
|
||
| function initialize() public initializer { | ||
| function initialize(address _zetaToken) public initializer { |
There was a problem hiding this comment.
@skosito ideally we'd implment this zero address checks everywhere.
|
|
||
| /// @custom:oz-upgrades-unsafe-allow constructor | ||
| constructor() { | ||
| _disableInitializers(); | ||
| } | ||
|
|
||
| function initialize() public initializer { | ||
| function initialize(address _zetaToken) public initializer { |
Check warning
Code scanning / Slither
Conformance to Solidity naming conventions Warning
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (16)
pkg/contracts/prototypes/evm/zetaconnectornative.sol/zetaconnectornative.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/prototypes/zevm/igatewayzevm.sol/igatewayzevmerrors.gois excluded by!pkg/**pkg/contracts/prototypes/zevm/senderzevm.sol/senderzevm.gois excluded by!pkg/**typechain-types/contracts/prototypes/evm/ZetaConnectorNative.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/ZetaConnectorNative__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/prototypes/zevm/IGatewayZEVM.sol/IGatewayZEVMErrors__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/prototypes/zevm/SenderZEVM__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 (8)
- contracts/prototypes/evm/ZetaConnectorNative.sol (1 hunks)
- contracts/prototypes/evm/ZetaConnectorNonNative.sol (1 hunks)
- contracts/prototypes/zevm/GatewayZEVM.sol (3 hunks)
- contracts/prototypes/zevm/IGatewayZEVM.sol (2 hunks)
- test/prototypes/GatewayEVMUniswap.spec.ts (1 hunks)
- testFoundry/GatewayEVM.t.sol (6 hunks)
- testFoundry/GatewayEVMUpgrade.t.sol (3 hunks)
- testFoundry/GatewayEVMZEVM.t.sol (5 hunks)
Files skipped from review as they are similar to previous changes (5)
- contracts/prototypes/zevm/IGatewayZEVM.sol
- test/prototypes/GatewayEVMUniswap.spec.ts
- testFoundry/GatewayEVM.t.sol
- testFoundry/GatewayEVMUpgrade.t.sol
- testFoundry/GatewayEVMZEVM.t.sol
Additional context used
Path-based instructions (3)
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/zevm/GatewayZEVM.sol (1)
Pattern
contracts/**: Review the Solidity contracts for security vulnerabilities and best practices.
GitHub Check: Slither
contracts/prototypes/zevm/GatewayZEVM.sol
[warning] 27-27: Conformance to Solidity naming conventions
Parameter GatewayZEVM.initialize(address)._zetaToken (contracts/prototypes/zevm/GatewayZEVM.sol#27) is not in mixedCase
[warning] 54-59: Low-level calls
Low level call in GatewayZEVM._transferZETA(uint256,address) (contracts/prototypes/zevm/GatewayZEVM.sol#54-59):
- (sent,None) = to.call{value: amount}() (contracts/prototypes/zevm/GatewayZEVM.sol#57)
Additional comments not posted (12)
contracts/prototypes/evm/ZetaConnectorNonNative.sol (4)
9-11: Ensure address validation for_gatewayand_zetaToken.While the constructor initializes the base contract, it is essential to ensure that the
_gatewayand_zetaTokenaddresses are valid and not zero addresses.
14-17: Ensure proper access control for thewithdrawfunction.The
withdrawfunction mints Zeta tokens to the destination address. Ensure that only authorized addresses can call this function to prevent unauthorized minting.
20-28: Ensure proper access control for thewithdrawAndCallfunction.The
withdrawAndCallfunction mints Zeta tokens to the gateway and then forwards the call to the gateway contract. Ensure that only authorized addresses can call this function to prevent unauthorized minting and forwarding.
31-34: Ensure proper access control for thereceiveTokensfunction.The
receiveTokensfunction burns the tokens received from the sender. Ensure that only authorized addresses can call this function to prevent unauthorized burning.contracts/prototypes/evm/ZetaConnectorNative.sol (4)
11-13: Ensure address validation for_gatewayand_zetaToken.While the constructor initializes the base contract, it is essential to ensure that the
_gatewayand_zetaTokenaddresses are valid and not zero addresses.
16-19: Ensure proper access control for thewithdrawfunction.The
withdrawfunction transfers Zeta tokens to the destination address. Ensure that only authorized addresses can call this function to prevent unauthorized transfers.
22-29: Ensure proper access control for thewithdrawAndCallfunction.The
withdrawAndCallfunction transfers Zeta tokens to the gateway and then forwards the call to the gateway contract. Ensure that only authorized addresses can call this function to prevent unauthorized transfers and forwarding.
33-36: Ensure proper access control for thereceiveTokensfunction.The
receiveTokensfunction transfers tokens from the sender to the contract. Ensure that only authorized addresses can call this function to prevent unauthorized transfers.contracts/prototypes/zevm/GatewayZEVM.sol (4)
Line range hint
39-51:
Verify the security of gas fee transfer and token burn operations.The
_withdrawZRC20function handles the withdrawal of ZRC20 tokens and burns them. Ensure that the gas fee transfer and token burn operations are secure and cannot be exploited.Tools
GitHub Check: Slither
[warning] 27-27: Conformance to Solidity naming conventions
Parameter GatewayZEVM.initialize(address)._zetaToken (contracts/prototypes/zevm/GatewayZEVM.sol#27) is not in mixedCase
74-77: Ensure proper access control for thewithdrawfunction.The
withdrawfunction uses_transferZETAto transfer ZETA tokens and emits an event. Ensure that only authorized addresses can call this function to prevent unauthorized transfers.
80-82: Ensure proper access control for thewithdrawAndCallfunction.The
withdrawAndCallfunction uses_transferZETAto transfer ZETA tokens and then calls a contract. Ensure that only authorized addresses can call this function to prevent unauthorized transfers and calls.
139-150: Ensure proper access control for thedepositAndCallfunction.The
depositAndCallfunction deposits ZETA tokens and calls a specified contract on ZEVM. Ensure that only authorized addresses can call this function to prevent unauthorized deposits and calls.
| function initialize(address _zetaToken) public initializer { | ||
| if (_zetaToken == address(0)) { | ||
| revert ZeroAddress(); | ||
| } | ||
|
|
||
| __Ownable_init(); | ||
| __UUPSUpgradeable_init(); | ||
| zetaToken = _zetaToken; |
There was a problem hiding this comment.
Conformance to Solidity naming conventions.
The parameter _zetaToken should follow the mixedCase naming convention.
- function initialize(address _zetaToken) public initializer {
+ function initialize(address zetaTokenAddress) public initializer {
- if (_zetaToken == address(0)) {
+ if (zetaTokenAddress == address(0)) {
- revert ZeroAddress();
+ revert ZeroAddress();
}
- zetaToken = _zetaToken;
+ zetaToken = zetaTokenAddress;
}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 initialize(address _zetaToken) public initializer { | |
| if (_zetaToken == address(0)) { | |
| revert ZeroAddress(); | |
| } | |
| __Ownable_init(); | |
| __UUPSUpgradeable_init(); | |
| zetaToken = _zetaToken; | |
| function initialize(address zetaTokenAddress) public initializer { | |
| if (zetaTokenAddress == address(0)) { | |
| revert ZeroAddress(); | |
| } | |
| __Ownable_init(); | |
| __UUPSUpgradeable_init(); | |
| zetaToken = zetaTokenAddress; |
Tools
GitHub Check: Slither
[warning] 27-27: Conformance to Solidity naming conventions
Parameter GatewayZEVM.initialize(address)._zetaToken (contracts/prototypes/zevm/GatewayZEVM.sol#27) is not in mixedCase
| function _transferZETA(uint256 amount, address to) internal { | ||
| if (!IWETH9(zetaToken).transferFrom(msg.sender, address(this), amount)) revert FailedZetaSent(); | ||
| IWETH9(zetaToken).withdraw(amount); | ||
| (bool sent, ) = to.call{value: amount}(""); | ||
| if (!sent) revert FailedZetaSent(); | ||
| } |
There was a problem hiding this comment.
Avoid low-level calls in _transferZETA function.
The _transferZETA function includes low-level calls which can be prone to security issues. Consider using send or transfer for safer ETH transfers.
- (bool sent, ) = to.call{value: amount}("");
+ (bool sent, ) = to.send(amount);
if (!sent) revert FailedZetaSent();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 _transferZETA(uint256 amount, address to) internal { | |
| if (!IWETH9(zetaToken).transferFrom(msg.sender, address(this), amount)) revert FailedZetaSent(); | |
| IWETH9(zetaToken).withdraw(amount); | |
| (bool sent, ) = to.call{value: amount}(""); | |
| if (!sent) revert FailedZetaSent(); | |
| } | |
| function _transferZETA(uint256 amount, address to) internal { | |
| if (!IWETH9(zetaToken).transferFrom(msg.sender, address(this), amount)) revert FailedZetaSent(); | |
| IWETH9(zetaToken).withdraw(amount); | |
| (bool sent, ) = to.send(amount); | |
| if (!sent) revert FailedZetaSent(); | |
| } |
Tools
GitHub Check: Slither
[warning] 54-59: Low-level calls
Low level call in GatewayZEVM._transferZETA(uint256,address) (contracts/prototypes/zevm/GatewayZEVM.sol#54-59):
- (sent,None) = to.call{value: amount}() (contracts/prototypes/zevm/GatewayZEVM.sol#57)
implements specs for zeta handling
NOTE: unit tests and worker will be in follow up PR once unit test PR is merged and specs are approved
Qs:
zeta connector now looks like custody, i didnt add mint before transfer because according to existing interfaces zeta on evm doesnt have mint method, and this is handled by locking tokens on connector instead of minting?
on gatewayzevm, i used same logic as existing one:
is this ok?
Summary by CodeRabbit
New Features
ZetaConnectorandzetatoken contracts for enhanced token operations.ZetaConnector,zeta, andGatewayEVMcontracts.Bug Fixes
Tests
ZetaConnectorandzetatoken contracts.ZetaConnector,zeta, andGatewayEVM.