fix: use nonReentrant only on functions interacting with arbitrary contracts #395
Conversation
skosito
requested review from
andresaiello,
brewmaster012,
lumtis,
CharlieMc0,
fadeev and
fbac
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #395 +/- ##
=======================================
Coverage 84.23% 84.23%
=======================================
Files 8 8
Lines 387 387
Branches 122 122
=======================================
Hits 326 326
Misses 61 61 ☔ View full report in Codecov by Sentry. |
skosito
changed the title
yes this was initial PR description, will update |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- v2/contracts/evm/GatewayEVM.sol (4 hunks)
- v2/contracts/zevm/GatewayZEVM.sol (5 hunks)
🧰 Additional context used
🔇 Additional comments (8)
v2/contracts/evm/GatewayEVM.sol (3)
138-138: Keep nonReentrant modifier for arbitrary contract interactionsThe
executefunction can make arbitrary external calls whenmessageContext.senderisaddress(0). ThenonReentrantmodifier is correctly applied here to protect against potential reentrancy attacks.
175-175: Keep nonReentrant modifier for arbitrary contract interactionsThe
executeWithERC20function can make arbitrary external calls whenmessageContext.senderisaddress(0). ThenonReentrantmodifier is correctly applied here to protect against potential reentrancy attacks.
Line range hint
1-500: Verify PR objective implementationThe current implementation appears to be the opposite of what's described in the PR objectives. The PR aims to "remove the
nonReentrantmodifier from the entrypoint functions" but the code shows it being added to various functions.Let's verify the previous state of these modifiers:
v2/contracts/zevm/GatewayZEVM.sol (5)
Line range hint
341-345: Appropriate use ofnonReentrantmodifier onexecutefunctionAdding the
nonReentrantmodifier to theexecutefunction enhances security by preventing reentrancy attacks when interacting with external contracts throughUniversalContract(target).onCall(). This is appropriate sinceexecutecan be called with arbitrarytargetaddresses.
Line range hint
364-369: Prudent addition ofnonReentranttodepositAndCallfunctionIncluding the
nonReentrantmodifier on thedepositAndCallfunction is a good security measure. This function handles deposits and invokes external contracts, which could be targets for reentrancy exploits.
Line range hint
388-393: Validation ofnonReentrantondepositAndCallwith ZETAAdding the
nonReentrantmodifier to thedepositAndCallfunction that handles ZETA is appropriate. Since this function transfers ZETA and callsUniversalContract(target).onCall(), it is susceptible to reentrancy attacks, and the modifier enhances security.
403-406: SecuringexecuteRevertfunction withnonReentrantApplying the
nonReentrantmodifier to theexecuteRevertfunction is wise. It interacts with external contracts viaRevertable(target).onRevert(revertContext), and protecting against reentrancy here strengthens the contract's security posture.
Line range hint
421-426: Enhanced security fordepositAndRevertfunctionIncluding
nonReentranton thedepositAndRevertfunction is appropriate. This function deposits tokens and callsRevertable(target).onRevert(revertContext), interacting with external contracts and potentially vulnerable to reentrancy attacks.
| @@ -217,9 +217,9 @@ contract GatewayEVM is | |||
| RevertContext calldata revertContext | |||
| ) | |||
| external | |||
| nonReentrant | |||
There was a problem hiding this comment.
Remove unnecessary nonReentrant modifier
The revertWithERC20 function only interacts with contracts implementing the Revertable interface and doesn't make arbitrary external calls. The nonReentrant modifier is not necessary here as the function enforces a specific interface.
function revertWithERC20(
address token,
address to,
uint256 amount,
bytes calldata data,
RevertContext calldata revertContext
)
external
- nonReentrant
onlyRole(ASSET_HANDLER_ROLE)
whenNotPaused📝 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.
| nonReentrant | |
| onlyRole(ASSET_HANDLER_ROLE) |
| @@ -110,9 +110,9 @@ | |||
| ) | |||
| public | |||
| payable | |||
| nonReentrant | |||
There was a problem hiding this comment.
Remove unnecessary nonReentrant modifier
The executeRevert function only interacts with contracts implementing the Revertable interface and doesn't make arbitrary external calls. The nonReentrant modifier is not necessary here as the function enforces a specific interface.
function executeRevert(
address destination,
bytes calldata data,
RevertContext calldata revertContext
)
public
payable
- nonReentrant
onlyRole(TSS_ROLE)
whenNotPaused📝 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.
| nonReentrant | |
| function executeRevert( | |
| address destination, | |
| bytes calldata data, | |
| RevertContext calldata revertContext | |
| ) | |
| public | |
| payable | |
| onlyRole(TSS_ROLE) | |
| whenNotPaused |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
v2/contracts/evm/GatewayEVM.sol (1)
Line range hint
1-500: Refine reentrancy protection strategyThe current implementation applies
nonReentranttoo broadly. Consider this pattern instead:
- Keep
nonReentranton functions that make arbitrary external calls (execute,executeWithERC20)- Remove
nonReentrantfrom functions that:
- Only interact with trusted contracts implementing known interfaces (
executeRevert,revertWithERC20)- Only emit events or interact with trusted system addresses (
deposit,depositAndCall,call)This approach provides necessary protection while allowing legitimate reentrant calls where safe.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (15)
v2/docs/src/contracts/evm/GatewayEVM.sol/contract.GatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/GatewayZEVM.sol/contract.GatewayZEVM.mdis excluded by!v2/docs/**v2/pkg/erc20custody.t.sol/erc20custodytest.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.sol/gatewayevm.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevmtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmzevm.t.sol/gatewayevmzevmtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.sol/gatewayzevm.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.t.sol/gatewayzevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.t.sol/gatewayzevmoutboundtest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornative.t.sol/zetaconnectornativetest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.t.sol/zetaconnectornonnativetest.gois excluded by!v2/pkg/**v2/pkg/zrc20.t.sol/zrc20test.gois excluded by!v2/pkg/**v2/types/factories/GatewayEVM__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayZEVM__factory.tsis excluded by!v2/types/**
📒 Files selected for processing (2)
- v2/contracts/evm/GatewayEVM.sol (5 hunks)
- v2/contracts/zevm/GatewayZEVM.sol (5 hunks)
🧰 Additional context used
🔇 Additional comments (10)
v2/contracts/evm/GatewayEVM.sol (5)
113-113: Remove unnecessary nonReentrant modifierThe
executeRevertfunction only interacts with contracts implementing theRevertableinterface and doesn't make arbitrary external calls. ThenonReentrantmodifier is not necessary here.
138-138: Keep nonReentrant modifier for arbitrary contract interactionsThe
executefunction makes arbitrary external calls through_executeArbitraryCall, which could potentially lead to reentrancy attacks. ThenonReentrantmodifier should be retained here.
175-175: Keep nonReentrant modifier for token interactionsThe
executeWithERC20function interacts with arbitrary contracts and handles ERC20 token transfers. ThenonReentrantmodifier is necessary to prevent potential reentrancy attacks through malicious tokens or contracts.
220-220: Remove unnecessary nonReentrant modifierThe
revertWithERC20function only interacts with contracts implementing theRevertableinterface and usessafeTransfer. ThenonReentrantmodifier is not necessary here.
236-236:⚠️ Potential issueRemove unnecessary nonReentrant modifier from deposit functions
The deposit functions only transfer ETH to TSS address and emit events. They don't make any arbitrary external calls or interact with untrusted contracts. The
nonReentrantmodifier is not necessary here.Let's verify the deposit function interactions:
✅ Verification successful
Let me analyze the results and generate one more verification script to be completely certain about the deposit function in GatewayEVM.sol.
This will help us see the complete implementation of all deposit functions in GatewayEVM.sol and verify that they only interact with the tssAddress.
Let me try one more verification with a slightly modified pattern to ensure we capture the complete deposit function implementation.
Based on the verification results, I can now provide a final response:
Confirmed: nonReentrant modifier can be safely removed from deposit functions
The verification confirms that the deposit function in GatewayEVM.sol only:
- Performs basic input validation
- Makes a single ETH transfer to the trusted tssAddress using .call
- Emits an event
- Has no complex interactions or external calls to untrusted contracts
There are no reentrancy risks present as:
- The only external call is to transfer ETH to a trusted contract (tssAddress)
- The state changes and event emission happen after the ETH transfer
- No other contract functions are called that could create reentrancy vectors
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that deposit functions only interact with trusted contracts # Expected: Only find calls to tssAddress and event emissions # Search for all deposit function implementations rg -A 10 "function deposit.*\{" # Search for external calls within deposit functions ast-grep --pattern 'function deposit($_) { $$$ $DST.call($_) $$$ }'Length of output: 5852
Script:
#!/bin/bash # Let's verify the complete implementation of deposit functions in GatewayEVM.sol # to ensure there are no other external calls or interactions we might have missed rg -U "function deposit.*\{.*?\}" v2/contracts/evm/GatewayEVM.sol -A 20 --multiline # Also check for any other deposit functions in the same contract ast-grep --pattern 'contract GatewayEVM { $$$ function deposit($_) { $$$ } $$$ }'Length of output: 181
Script:
#!/bin/bash # Search for the complete deposit function implementation in GatewayEVM.sol rg -A 15 "^\s*function deposit.*\{" v2/contracts/evm/GatewayEVM.sol # Also check for any other functions that might be called within deposit rg "function\s+\w+.*\{" v2/contracts/evm/GatewayEVM.sol | grep -v "deposit" # Look for any modifiers applied to the deposit function rg "^\s*function deposit.*" v2/contracts/evm/GatewayEVM.solLength of output: 2187
v2/contracts/zevm/GatewayZEVM.sol (5)
341-341: Appropriate Use ofnonReentrantModifier inexecuteFunctionAdding the
nonReentrantmodifier to theexecutefunction enhances security by preventing reentrancy attacks when interacting with arbitrary contracts via theonCallmethod.
364-364: Proper Application ofnonReentrantModifier indepositAndCallFunctionIncluding the
nonReentrantmodifier in thedepositAndCallfunction is appropriate, as it involves token deposits and external contract calls, which could be vulnerable to reentrancy exploits.
388-388: Secure Enhancement withnonReentrantin ZETAdepositAndCallThe addition of the
nonReentrantmodifier to the ZETA version ofdepositAndCallstrengthens the contract against reentrancy attacks during ZETA token transfers and external contract interactions.
403-411: Validation ofnonReentrantAddition toexecuteRevertFunctionApplying the
nonReentrantmodifier toexecuteRevertis a sound decision, as it callsonReverton an external contract, mitigating potential reentrancy risks.
429-429: Correct Use ofnonReentrantindepositAndRevertFunctionAdding the
nonReentrantmodifier todepositAndRevertis appropriate, ensuring reentrancy protection when depositing tokens and executing theonRevertcallback on external contracts.
Use case:
Call from zetachain to evm chain reaches
gatewayEVM.executefunction which is calling arbitrary function on provided contract. That arbitrary function can call back gateway, eg:gatewayEVM.callgatewayEVM.depositAndCall. Because bothexecuteandcall/depositAndCallhavenonReentrantmodifier, this is not possible currently.Solution can be to remove
nonReentrantfrom either of those - it is better to remove from entrypoint functions since they do not interact with arbitrary contract, just interact with system contracts or addresses (eg. tss, custody, zrc20), and just emit events. On the other side, execute functions interact with arbitrary contracts so leaving it there makes more sense.Summary by CodeRabbit
New Features
Bug Fixes
These updates improve overall contract security and ensure safer handling of transactions.