improve: [L06] Add reentrancy guards to all public methods

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);
     }
 

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,19 +824,17 @@ 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;
     }
 
     /**
      * @notice Send unclaimed accumulated protocol fees to fee capture address.
      * @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 "";
     }
 

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,

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);
     }

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");
     }

contracts/SpokePool.sol

@@ -169,15 +169,15 @@ 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);
     }
 
     /**
      * @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);
     }

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]