improve: [L06] Add reentrancy guards to all public methods #92
Conversation
| // Since _exchangeRateCurrent() reads this contract's balance and updates contract state using it, it must be | ||
| // first before transferring any tokens to this contract to ensure synchronization. | ||
| uint256 lpTokensToMint = (l1TokenAmount * 1e18) / _exchangeRateCurrent(l1Token); | ||
| ExpandedIERC20(pooledTokens[l1Token].lpToken).mint(msg.sender, lpTokensToMint); |
There was a problem hiding this comment.
Moved token transfer "interaction" after any effects to follow CEI
There was a problem hiding this comment.
nice. doing this + having the guard is best posible. importantly, you are doing this after the calls that modify tracking state so no risk in this change.
contracts/HubPool.sol
Outdated
| // If they try access more funds than available (i.e l1TokensToReturn > liquidReserves) this will underflow. | ||
| pooledTokens[l1Token].liquidReserves -= l1TokensToReturn; | ||
|
|
||
| ExpandedIERC20(pooledTokens[l1Token].lpToken).burnFrom(msg.sender, lpTokenAmount); |
| }); | ||
|
|
||
| // Finally, delete the state pertaining to the active proposal so that another proposer can submit a new bundle. | ||
| delete rootBundleProposal; |
nicholaspai
changed the title
contracts/Arbitrum_SpokePool.sol
Outdated
|
|
||
| // Apply AVM-specific transformation to cross domain admin address on L1. | ||
| function _requireAdminSender() internal override onlyFromCrossDomainAdmin {} | ||
| function _requireAdminSender() internal override onlyFromCrossDomainAdmin nonReentrant {} |
There was a problem hiding this comment.
so this is an internal method. why add it here? dont we only need it on the external/public functions?
There was a problem hiding this comment.
Ah yes i'll add a comment. Basically we don't add the nonReentrant modifier to onlyAdmin functions in the base SpokePool contract because the Polygon_SpokePool will call these methods internally via the processMessageFromRoot. The other spoke pools like Optimism_SpokePool and Arbitrum_SpokePool have their admin functions triggered by an external contract so we should be reentrancy guarding those methods. However, in the Polygon_SpokePool case we need to reentrancy guard at the processMessageFromRoot method instead of at the admin functions.
There was a problem hiding this comment.
But this isn't equivalent to adding a re-entrancy guard to the admin function, right? It guarantees that this function wasn't re-entered, but it doesn't help us if that admin function re-enters a different function since it doesn't wrap the entire method, no?
mrice32
left a comment
mrice32
left a comment
There was a problem hiding this comment.
I think it's going to be difficult to make all the spoke pool admin methods non-reentrant when they're all called in such different ways.
The issue raised by OZ specifically applies to the HubPool, not the SpokePool right? So are these changes beyond the scope of the changes being requested?
contracts/Arbitrum_SpokePool.sol
Outdated
|
|
||
| // Apply AVM-specific transformation to cross domain admin address on L1. | ||
| function _requireAdminSender() internal override onlyFromCrossDomainAdmin {} | ||
| function _requireAdminSender() internal override onlyFromCrossDomainAdmin nonReentrant {} |
There was a problem hiding this comment.
But this isn't equivalent to adding a re-entrancy guard to the admin function, right? It guarantees that this function wasn't re-entered, but it doesn't help us if that admin function re-enters a different function since it doesn't wrap the entire method, no?
No description provided.