diff --git a/contracts/Arbitrum_SpokePool.sol b/contracts/Arbitrum_SpokePool.sol index 5a0ef4a97..5d7dccb2f 100644 --- a/contracts/Arbitrum_SpokePool.sol +++ b/contracts/Arbitrum_SpokePool.sol @@ -59,7 +59,7 @@ contract Arbitrum_SpokePool is SpokePool { * @notice Change L2 gateway router. Callable only by admin. * @param newL2GatewayRouter New L2 gateway router. */ - function setL2GatewayRouter(address newL2GatewayRouter) public onlyAdmin { + function setL2GatewayRouter(address newL2GatewayRouter) public onlyAdmin nonReentrant { _setL2GatewayRouter(newL2GatewayRouter); } @@ -68,7 +68,7 @@ contract Arbitrum_SpokePool is SpokePool { * @param l2Token Arbitrum token. * @param l1Token Ethereum version of l2Token. */ - function whitelistToken(address l2Token, address l1Token) public onlyAdmin { + function whitelistToken(address l2Token, address l1Token) public onlyAdmin nonReentrant { _whitelistToken(l2Token, l1Token); } diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index c793d094e..d84b1057b 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -310,6 +310,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { public override onlyOwner + nonReentrant { require(newProtocolFeeCapturePct <= 1e18, "Bad protocolFeeCapturePct"); require(newProtocolFeeCaptureAddress != address(0), "Bad protocolFeeCaptureAddress"); @@ -351,7 +352,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { * @notice Sets root bundle proposal liveness period. Callable only by owner. * @param newLiveness New liveness period. */ - function setLiveness(uint32 newLiveness) public override onlyOwner { + function setLiveness(uint32 newLiveness) public override onlyOwner nonReentrant { require(newLiveness > 10 minutes, "Liveness too short"); liveness = newLiveness; emit LivenessSet(newLiveness); @@ -383,7 +384,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { uint256 l2ChainId, address adapter, address spokePool - ) public override onlyOwner { + ) public override onlyOwner nonReentrant { crossChainContracts[l2ChainId] = CrossChainContract(adapter, spokePool); emit CrossChainContractsSet(l2ChainId, adapter, spokePool); } @@ -461,7 +462,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { * @notice Disables LPs from providing liquidity for L1 token. Callable only by owner. * @param l1Token Token to disable liquidity provision for. */ - function disableL1TokenForLiquidityProvision(address l1Token) public override onlyOwner { + function disableL1TokenForLiquidityProvision(address l1Token) public override onlyOwner nonReentrant { pooledTokens[l1Token].isEnabled = false; emit L2TokenDisabledForLiquidityProvision(l1Token, pooledTokens[l1Token].lpToken); } @@ -490,8 +491,8 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { // 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); pooledTokens[l1Token].liquidReserves += l1TokenAmount; + ExpandedIERC20(pooledTokens[l1Token].lpToken).mint(msg.sender, lpTokensToMint); if (address(weth) == l1Token && msg.value > 0) WETH9(address(l1Token)).deposit{ value: msg.value }(); else IERC20(l1Token).safeTransferFrom(msg.sender, address(this), l1TokenAmount); @@ -518,7 +519,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { ExpandedIERC20(pooledTokens[l1Token].lpToken).burnFrom(msg.sender, lpTokenAmount); // Note this method does not make any liquidity utilization checks before letting the LP redeem their LP tokens. - // If they try access more funds that available (i.e l1TokensToReturn > liquidReserves) this will underflow. + // If they try access more funds than available (i.e l1TokensToReturn > liquidReserves) this will underflow. pooledTokens[l1Token].liquidReserves -= l1TokensToReturn; if (sendEth) { @@ -808,6 +809,9 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { customLiveness: liveness }); + // Finally, delete the state pertaining to the active proposal so that another proposer can submit a new bundle. + delete rootBundleProposal; + bondToken.safeTransferFrom(msg.sender, address(this), bondAmount); bondToken.safeIncreaseAllowance(address(optimisticOracle), bondAmount); optimisticOracle.disputePriceFor( @@ -820,9 +824,6 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { ); emit RootBundleDisputed(msg.sender, currentTime, requestAncillaryData); - - // Finally, delete the state pertaining to the active proposal so that another proposer can submit a new bundle. - delete rootBundleProposal; } /** @@ -830,9 +831,10 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { * @param l1Token Token whose protocol fees the caller wants to disburse. */ function claimProtocolFeesCaptured(address l1Token) public override nonReentrant { - IERC20(l1Token).safeTransfer(protocolFeeCaptureAddress, unclaimedAccumulatedProtocolFees[l1Token]); - emit ProtocolFeesCapturedClaimed(l1Token, unclaimedAccumulatedProtocolFees[l1Token]); + uint256 _unclaimedAccumulatedProtocolFees = unclaimedAccumulatedProtocolFees[l1Token]; unclaimedAccumulatedProtocolFees[l1Token] = 0; + IERC20(l1Token).safeTransfer(protocolFeeCaptureAddress, _unclaimedAccumulatedProtocolFees); + emit ProtocolFeesCapturedClaimed(l1Token, _unclaimedAccumulatedProtocolFees); } /** @@ -848,7 +850,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { * data in this ancillary data that is already included in the ProposeRootBundle event. * @return ancillaryData Ancillary data that can be decoded into UTF8. */ - function getRootBundleProposalAncillaryData() public view override returns (bytes memory ancillaryData) { + function getRootBundleProposalAncillaryData() public pure override returns (bytes memory ancillaryData) { return ""; } diff --git a/contracts/HubPoolInterface.sol b/contracts/HubPoolInterface.sol index 3d6400455..9b2c2a565 100644 --- a/contracts/HubPoolInterface.sol +++ b/contracts/HubPoolInterface.sol @@ -104,7 +104,7 @@ interface HubPoolInterface { function claimProtocolFeesCaptured(address l1Token) external; - function getRootBundleProposalAncillaryData() external view returns (bytes memory ancillaryData); + function getRootBundleProposalAncillaryData() external pure returns (bytes memory ancillaryData); function setPoolRebalanceRoute( uint256 destinationChainId, diff --git a/contracts/Optimism_SpokePool.sol b/contracts/Optimism_SpokePool.sol index 2be41a32b..a38297964 100644 --- a/contracts/Optimism_SpokePool.sol +++ b/contracts/Optimism_SpokePool.sol @@ -51,7 +51,7 @@ contract Optimism_SpokePool is CrossDomainEnabled, SpokePool { * @notice Change L1 gas limit. Callable only by admin. * @param newl1Gas New L1 gas limit to set. */ - function setL1GasLimit(uint32 newl1Gas) public onlyAdmin { + function setL1GasLimit(uint32 newl1Gas) public onlyAdmin nonReentrant { l1Gas = newl1Gas; emit SetL1Gas(newl1Gas); } @@ -61,7 +61,7 @@ contract Optimism_SpokePool is CrossDomainEnabled, SpokePool { * @dev If this mapping isn't set for an L2 token, then the standard bridge will be used to bridge this token. * @param tokenBridge Address of token bridge */ - function setTokenBridge(address l2Token, address tokenBridge) public onlyAdmin { + function setTokenBridge(address l2Token, address tokenBridge) public onlyAdmin nonReentrant { tokenBridges[l2Token] = tokenBridge; emit SetL2TokenBridge(l2Token, tokenBridge); } diff --git a/contracts/Polygon_SpokePool.sol b/contracts/Polygon_SpokePool.sol index 142456afb..dfbe4d12f 100644 --- a/contracts/Polygon_SpokePool.sol +++ b/contracts/Polygon_SpokePool.sol @@ -44,6 +44,10 @@ contract Polygon_SpokePool is IFxMessageProcessor, SpokePool { // through validation where the sender is checked and the root (mainnet) sender is also validated. // This modifier sets the callValidated variable so this condition can be checked in _requireAdminSender(). modifier validateInternalCalls() { + // Make sure callValidated is set to True only once at beginning of processMessageFromRoot, which prevents + // processMessageFromRoot from being re-entered. + require(!callValidated, "callValidated already set"); + // This sets a variable indicating that we're now inside a validated call. // Note: this is used by other methods to ensure that this call has been validated by this method and is not // spoofed. See @@ -85,7 +89,7 @@ contract Polygon_SpokePool is IFxMessageProcessor, SpokePool { * @notice Change FxChild address. Callable only by admin via processMessageFromRoot. * @param newFxChild New FxChild. */ - function setFxChild(address newFxChild) public onlyAdmin { + function setFxChild(address newFxChild) public onlyAdmin nonReentrant { fxChild = newFxChild; emit SetFxChild(fxChild); } @@ -94,7 +98,7 @@ contract Polygon_SpokePool is IFxMessageProcessor, SpokePool { * @notice Change polygonTokenBridger address. Callable only by admin via processMessageFromRoot. * @param newPolygonTokenBridger New Polygon Token Bridger contract. */ - function setPolygonTokenBridger(address payable newPolygonTokenBridger) public onlyAdmin { + function setPolygonTokenBridger(address payable newPolygonTokenBridger) public onlyAdmin nonReentrant { polygonTokenBridger = PolygonTokenBridger(newPolygonTokenBridger); emit SetPolygonTokenBridger(address(polygonTokenBridger)); } @@ -113,7 +117,7 @@ contract Polygon_SpokePool is IFxMessageProcessor, SpokePool { uint256, /*stateId*/ address rootMessageSender, bytes calldata data - ) public validateInternalCalls nonReentrant { + ) public validateInternalCalls { // Validation logic. require(msg.sender == fxChild, "Not from fxChild"); require(rootMessageSender == crossDomainAdmin, "Not from mainnet admin"); @@ -143,6 +147,10 @@ contract Polygon_SpokePool is IFxMessageProcessor, SpokePool { emit PolygonTokensBridged(relayerRefundLeaf.l2TokenAddress, address(this), relayerRefundLeaf.amountToReturn); } + // @dev: This contract will trigger admin functions internally via the `processMessageFromRoot`, which is why + // the `callValidated` check is made below and why we use the `validateInternalCalls` modifier on + // `processMessageFromRoot`. This prevents calling the admin functions from any other method besides + // `processMessageFromRoot`. function _requireAdminSender() internal view override { require(callValidated, "Must call processMessageFromRoot"); } diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index b2f5f9375..19a40dd8a 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -169,7 +169,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall * @notice Change cross domain admin address. Callable by admin only. * @param newCrossDomainAdmin New cross domain admin. */ - function setCrossDomainAdmin(address newCrossDomainAdmin) public override onlyAdmin { + function setCrossDomainAdmin(address newCrossDomainAdmin) public override onlyAdmin nonReentrant { _setCrossDomainAdmin(newCrossDomainAdmin); } @@ -177,7 +177,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall * @notice Change L1 hub pool address. Callable by admin only. * @param newHubPool New hub pool. */ - function setHubPool(address newHubPool) public override onlyAdmin { + function setHubPool(address newHubPool) public override onlyAdmin nonReentrant { _setHubPool(newHubPool); } @@ -191,7 +191,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall address originToken, uint256 destinationChainId, bool enabled - ) public override onlyAdmin { + ) public override onlyAdmin nonReentrant { enabledDepositRoutes[originToken][destinationChainId] = enabled; emit EnabledDepositRoute(originToken, destinationChainId, enabled); } @@ -200,7 +200,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall * @notice Change allowance for deposit quote time to differ from current block time. Callable by admin only. * @param newDepositQuoteTimeBuffer New quote time buffer. */ - function setDepositQuoteTimeBuffer(uint32 newDepositQuoteTimeBuffer) public override onlyAdmin { + function setDepositQuoteTimeBuffer(uint32 newDepositQuoteTimeBuffer) public override onlyAdmin nonReentrant { depositQuoteTimeBuffer = newDepositQuoteTimeBuffer; emit SetDepositQuoteTimeBuffer(newDepositQuoteTimeBuffer); } @@ -214,7 +214,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall * @param slowRelayRoot Merkle root containing slow relay fulfillment leaves that can be individually executed via * executeSlowRelayRoot(). */ - function relayRootBundle(bytes32 relayerRefundRoot, bytes32 slowRelayRoot) public override onlyAdmin { + function relayRootBundle(bytes32 relayerRefundRoot, bytes32 slowRelayRoot) public override onlyAdmin nonReentrant { uint32 rootBundleId = uint32(rootBundles.length); RootBundle storage rootBundle = rootBundles.push(); rootBundle.relayerRefundRoot = relayerRefundRoot; @@ -228,7 +228,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall * @param rootBundleId Index of the root bundle that needs to be deleted. Note: this is intentionally a uint256 * to ensure that a small input range doesn't limit which indices this method is able to reach. */ - function emergencyDeleteRootBundle(uint256 rootBundleId) public override onlyAdmin { + function emergencyDeleteRootBundle(uint256 rootBundleId) public override onlyAdmin nonReentrant { delete rootBundles[rootBundleId]; emit EmergencyDeleteRootBundle(rootBundleId); } diff --git a/test/chain-specific-spokepools/Polygon_SpokePool.ts b/test/chain-specific-spokepools/Polygon_SpokePool.ts index a22f2544d..02ad7f711 100644 --- a/test/chain-specific-spokepools/Polygon_SpokePool.ts +++ b/test/chain-specific-spokepools/Polygon_SpokePool.ts @@ -28,6 +28,9 @@ describe("Polygon Spoke Pool", function () { }); it("Only correct caller can set the cross domain admin", async function () { + // Cannot call directly + await expect(polygonSpokePool.setCrossDomainAdmin(rando.address)).to.be.reverted; + const setCrossDomainAdminData = polygonSpokePool.interface.encodeFunctionData("setCrossDomainAdmin", [ rando.address, ]); @@ -45,6 +48,9 @@ describe("Polygon Spoke Pool", function () { }); it("Only correct caller can set the hub pool address", async function () { + // Cannot call directly + await expect(polygonSpokePool.setHubPool(rando.address)).to.be.reverted; + const setHubPoolData = polygonSpokePool.interface.encodeFunctionData("setHubPool", [rando.address]); // Wrong rootMessageSender address. @@ -60,6 +66,9 @@ describe("Polygon Spoke Pool", function () { }); it("Only correct caller can enable a route", async function () { + // Cannot call directly + await expect(polygonSpokePool.setEnableRoute(l2Dai, 1, true)).to.be.reverted; + const setEnableRouteData = polygonSpokePool.interface.encodeFunctionData("setEnableRoute", [l2Dai, 1, true]); // Wrong rootMessageSender address. @@ -75,6 +84,9 @@ describe("Polygon Spoke Pool", function () { }); it("Only correct caller can set the quote time buffer", async function () { + // Cannot call directly + await expect(polygonSpokePool.setDepositQuoteTimeBuffer(12345)).to.be.reverted; + const setDepositQuoteTimeBufferData = polygonSpokePool.interface.encodeFunctionData("setDepositQuoteTimeBuffer", [ 12345, ]); @@ -94,6 +106,9 @@ describe("Polygon Spoke Pool", function () { }); it("Only correct caller can initialize a relayer refund", async function () { + // Cannot call directly + await expect(polygonSpokePool.relayRootBundle(mockTreeRoot, mockTreeRoot)).to.be.reverted; + const relayRootBundleData = polygonSpokePool.interface.encodeFunctionData("relayRootBundle", [ mockTreeRoot, mockTreeRoot, @@ -113,6 +128,21 @@ describe("Polygon Spoke Pool", function () { expect((await polygonSpokePool.rootBundles(0)).relayerRefundRoot).to.equal(mockTreeRoot); }); + it("Cannot re-enter processMessageFromRoot", async function () { + const relayRootBundleData = polygonSpokePool.interface.encodeFunctionData("relayRootBundle", [ + mockTreeRoot, + mockTreeRoot, + ]); + const processMessageFromRootData = polygonSpokePool.interface.encodeFunctionData("processMessageFromRoot", [ + 0, + owner.address, + relayRootBundleData, + ]); + + await expect(polygonSpokePool.connect(fxChild).processMessageFromRoot(0, owner.address, processMessageFromRootData)) + .to.be.reverted; + }); + it("Only owner can delete a relayer refund", async function () { const relayRootBundleData = polygonSpokePool.interface.encodeFunctionData("relayRootBundle", [ mockTreeRoot, @@ -121,6 +151,9 @@ describe("Polygon Spoke Pool", function () { await polygonSpokePool.connect(fxChild).processMessageFromRoot(0, owner.address, relayRootBundleData); + // Cannot call directly + await expect(polygonSpokePool.emergencyDeleteRootBundle(0)).to.be.reverted; + const emergencyDeleteRelayRootBundleData = polygonSpokePool.interface.encodeFunctionData( "emergencyDeleteRootBundle", [0]