From 4d1c54bafc2637f86f90473759b7a10ce82c7151 Mon Sep 17 00:00:00 2001
From: nicholaspai <npai.nyc@gmail.com>
Date: Mon, 14 Mar 2022 20:06:45 -0400
Subject: [PATCH 1/9] improve: Add reentrancy guards to all public methods

---
 contracts/Arbitrum_SpokePool.sol |  4 ++--
 contracts/HubPool.sol            |  7 ++++---
 contracts/Optimism_SpokePool.sol |  4 ++--
 contracts/Polygon_SpokePool.sol  |  4 ++--
 contracts/SpokePool.sol          | 12 ++++++------
 5 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/contracts/Arbitrum_SpokePool.sol b/contracts/Arbitrum_SpokePool.sol
index 81a59e718..c8dbca195 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 c10d7fb4a..ab0b8d1ac 100644
--- a/contracts/HubPool.sol
+++ b/contracts/HubPool.sol
@@ -299,6 +299,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
         public
         override
         onlyOwner
+        nonReentrant
     {
         require(newProtocolFeeCapturePct <= 1e18, "Bad protocolFeeCapturePct");
         protocolFeeCaptureAddress = newProtocolFeeCaptureAddress;
@@ -334,7 +335,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);
@@ -364,7 +365,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(AdapterInterface(adapter), spokePool);
         emit CrossChainContractsSet(l2ChainId, adapter, spokePool);
     }
@@ -424,7 +425,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);
     }
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..cca2dac56 100644
--- a/contracts/Polygon_SpokePool.sol
+++ b/contracts/Polygon_SpokePool.sol
@@ -85,7 +85,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 +94,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));
     }
diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol
index e31c73412..2e4a6e5af 100644
--- a/contracts/SpokePool.sol
+++ b/contracts/SpokePool.sol
@@ -180,7 +180,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);
     }
 
@@ -188,7 +188,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);
     }
 
@@ -202,7 +202,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);
     }
@@ -211,7 +211,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);
     }
@@ -225,7 +225,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;
@@ -239,7 +239,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);
     }

From 65ef3f0b6dee6c106f8b8fd059255f933ba75aee Mon Sep 17 00:00:00 2001
From: nicholaspai <npai.nyc@gmail.com>
Date: Mon, 14 Mar 2022 22:04:23 -0400
Subject: [PATCH 2/9] Add reentrancy guard to onlyAdmin override

---
 contracts/Arbitrum_SpokePool.sol |  6 +++---
 contracts/Ethereum_SpokePool.sol |  2 +-
 contracts/HubPool.sol            | 19 ++++++++++---------
 contracts/Optimism_SpokePool.sol |  6 +++---
 contracts/Polygon_SpokePool.sol  |  4 ++--
 contracts/SpokePool.sol          | 16 ++++++++++------
 6 files changed, 29 insertions(+), 24 deletions(-)

diff --git a/contracts/Arbitrum_SpokePool.sol b/contracts/Arbitrum_SpokePool.sol
index c8dbca195..ebf09c56e 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 nonReentrant {
+    function setL2GatewayRouter(address newL2GatewayRouter) public onlyAdmin {
         _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 nonReentrant {
+    function whitelistToken(address l2Token, address l1Token) public onlyAdmin {
         _whitelistToken(l2Token, l1Token);
     }
 
@@ -108,5 +108,5 @@ contract Arbitrum_SpokePool is SpokePool {
     }
 
     // Apply AVM-specific transformation to cross domain admin address on L1.
-    function _requireAdminSender() internal override onlyFromCrossDomainAdmin {}
+    function _requireAdminSender() internal override onlyFromCrossDomainAdmin nonReentrant {}
 }
diff --git a/contracts/Ethereum_SpokePool.sol b/contracts/Ethereum_SpokePool.sol
index 12ac97aee..08ca649cf 100644
--- a/contracts/Ethereum_SpokePool.sol
+++ b/contracts/Ethereum_SpokePool.sol
@@ -35,5 +35,5 @@ contract Ethereum_SpokePool is SpokePool, Ownable {
 
     // Admin is simply owner which should be same account that owns the HubPool deployed on this network. A core
     // assumption of this contract system is that the HubPool is deployed on Ethereum.
-    function _requireAdminSender() internal override onlyOwner {}
+    function _requireAdminSender() internal override onlyOwner nonReentrant {}
 }
diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol
index ab0b8d1ac..967f3d198 100644
--- a/contracts/HubPool.sol
+++ b/contracts/HubPool.sol
@@ -454,8 +454,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);
@@ -477,12 +477,12 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
     ) public override nonReentrant {
         require(address(weth) == l1Token || !sendEth, "Cant send eth");
         uint256 l1TokensToReturn = (lpTokenAmount * _exchangeRateCurrent(l1Token)) / 1e18;
-
-        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;
 
+        ExpandedIERC20(pooledTokens[l1Token].lpToken).burnFrom(msg.sender, lpTokenAmount);
+
         if (sendEth) _unwrapWETHTo(payable(msg.sender), l1TokensToReturn);
         else IERC20(l1Token).safeTransfer(msg.sender, l1TokensToReturn);
 
@@ -726,6 +726,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(
@@ -738,9 +741,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;
     }
 
     /**
@@ -748,9 +748,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);
     }
 
     /**
diff --git a/contracts/Optimism_SpokePool.sol b/contracts/Optimism_SpokePool.sol
index a38297964..de16a4e07 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 nonReentrant {
+    function setL1GasLimit(uint32 newl1Gas) public onlyAdmin {
         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 nonReentrant {
+    function setTokenBridge(address l2Token, address tokenBridge) public onlyAdmin {
         tokenBridges[l2Token] = tokenBridge;
         emit SetL2TokenBridge(l2Token, tokenBridge);
     }
@@ -154,5 +154,5 @@ contract Optimism_SpokePool is CrossDomainEnabled, SpokePool {
     }
 
     // Apply OVM-specific transformation to cross domain admin address on L1.
-    function _requireAdminSender() internal override onlyFromCrossDomainAccount(crossDomainAdmin) {}
+    function _requireAdminSender() internal override onlyFromCrossDomainAccount(crossDomainAdmin) nonReentrant {}
 }
diff --git a/contracts/Polygon_SpokePool.sol b/contracts/Polygon_SpokePool.sol
index cca2dac56..142456afb 100644
--- a/contracts/Polygon_SpokePool.sol
+++ b/contracts/Polygon_SpokePool.sol
@@ -85,7 +85,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 nonReentrant {
+    function setFxChild(address newFxChild) public onlyAdmin {
         fxChild = newFxChild;
         emit SetFxChild(fxChild);
     }
@@ -94,7 +94,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 nonReentrant {
+    function setPolygonTokenBridger(address payable newPolygonTokenBridger) public onlyAdmin {
         polygonTokenBridger = PolygonTokenBridger(newPolygonTokenBridger);
         emit SetPolygonTokenBridger(address(polygonTokenBridger));
     }
diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol
index 2e4a6e5af..6447856ea 100644
--- a/contracts/SpokePool.sol
+++ b/contracts/SpokePool.sol
@@ -24,6 +24,10 @@ import "./SpokePoolInterface.sol";
  * on the destination chain. Locked source chain tokens are later sent over the canonical token bridge to L1 HubPool.
  * Relayers are refunded with destination tokens out of this contract after another off-chain actor, a "data worker",
  * submits a proof that the relayer correctly submitted a relay on this SpokePool.
+ * @dev We don't use reentrancy guards in public onlyAdmin methods because these functions are expected to be called
+ * via the canonical L1 --> L2 bridge. Some bridges, like Polygon's, ultimately call these functions internally, so
+ * we leave it up to the overriding SpokePool contract to add reentrancy to the _requireAdminSender overridden
+ * implementation.
  */
 abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCaller {
     using SafeERC20 for IERC20;
@@ -180,7 +184,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 nonReentrant {
+    function setCrossDomainAdmin(address newCrossDomainAdmin) public override onlyAdmin {
         _setCrossDomainAdmin(newCrossDomainAdmin);
     }
 
@@ -188,7 +192,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 nonReentrant {
+    function setHubPool(address newHubPool) public override onlyAdmin {
         _setHubPool(newHubPool);
     }
 
@@ -202,7 +206,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall
         address originToken,
         uint256 destinationChainId,
         bool enabled
-    ) public override onlyAdmin nonReentrant {
+    ) public override onlyAdmin {
         enabledDepositRoutes[originToken][destinationChainId] = enabled;
         emit EnabledDepositRoute(originToken, destinationChainId, enabled);
     }
@@ -211,7 +215,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 nonReentrant {
+    function setDepositQuoteTimeBuffer(uint32 newDepositQuoteTimeBuffer) public override onlyAdmin {
         depositQuoteTimeBuffer = newDepositQuoteTimeBuffer;
         emit SetDepositQuoteTimeBuffer(newDepositQuoteTimeBuffer);
     }
@@ -225,7 +229,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 nonReentrant {
+    function relayRootBundle(bytes32 relayerRefundRoot, bytes32 slowRelayRoot) public override onlyAdmin {
         uint32 rootBundleId = uint32(rootBundles.length);
         RootBundle storage rootBundle = rootBundles.push();
         rootBundle.relayerRefundRoot = relayerRefundRoot;
@@ -239,7 +243,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 nonReentrant {
+    function emergencyDeleteRootBundle(uint256 rootBundleId) public override onlyAdmin {
         delete rootBundles[rootBundleId];
         emit EmergencyDeleteRootBundle(rootBundleId);
     }

From cc6853a368e19075b0803c77cc0190fdf85e6216 Mon Sep 17 00:00:00 2001
From: nicholaspai <npai.nyc@gmail.com>
Date: Wed, 16 Mar 2022 11:32:16 -0400
Subject: [PATCH 3/9] Add comments

---
 contracts/Arbitrum_SpokePool.sol | 6 ++++++
 contracts/Ethereum_SpokePool.sol | 6 ++++++
 contracts/Optimism_SpokePool.sol | 6 ++++++
 contracts/Polygon_SpokePool.sol  | 4 ++++
 4 files changed, 22 insertions(+)

diff --git a/contracts/Arbitrum_SpokePool.sol b/contracts/Arbitrum_SpokePool.sol
index ebf09c56e..dafd10bae 100644
--- a/contracts/Arbitrum_SpokePool.sol
+++ b/contracts/Arbitrum_SpokePool.sol
@@ -108,5 +108,11 @@ contract Arbitrum_SpokePool is SpokePool {
     }
 
     // Apply AVM-specific transformation to cross domain admin address on L1.
+    // @dev: This is an internal method that we purposefully add a reentrancy guard to. 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.
     function _requireAdminSender() internal override onlyFromCrossDomainAdmin nonReentrant {}
 }
diff --git a/contracts/Ethereum_SpokePool.sol b/contracts/Ethereum_SpokePool.sol
index 08ca649cf..48c044163 100644
--- a/contracts/Ethereum_SpokePool.sol
+++ b/contracts/Ethereum_SpokePool.sol
@@ -35,5 +35,11 @@ contract Ethereum_SpokePool is SpokePool, Ownable {
 
     // Admin is simply owner which should be same account that owns the HubPool deployed on this network. A core
     // assumption of this contract system is that the HubPool is deployed on Ethereum.
+    // @dev: This is an internal method that we purposefully add a reentrancy guard to. 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.
     function _requireAdminSender() internal override onlyOwner nonReentrant {}
 }
diff --git a/contracts/Optimism_SpokePool.sol b/contracts/Optimism_SpokePool.sol
index de16a4e07..b0fb07775 100644
--- a/contracts/Optimism_SpokePool.sol
+++ b/contracts/Optimism_SpokePool.sol
@@ -154,5 +154,11 @@ contract Optimism_SpokePool is CrossDomainEnabled, SpokePool {
     }
 
     // Apply OVM-specific transformation to cross domain admin address on L1.
+    // @dev: This is an internal method that we purposefully add a reentrancy guard to. 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.
     function _requireAdminSender() internal override onlyFromCrossDomainAccount(crossDomainAdmin) nonReentrant {}
 }
diff --git a/contracts/Polygon_SpokePool.sol b/contracts/Polygon_SpokePool.sol
index 142456afb..9230b04ab 100644
--- a/contracts/Polygon_SpokePool.sol
+++ b/contracts/Polygon_SpokePool.sol
@@ -143,6 +143,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`. So, we don't need
+    // to explicitly reentrancy guard admin functions on this spoke pool, but we should ensure that the
+    // `processMessageFromRoot` method is reentrancy guarded which is why the `callValidated` check is made below
+    // and why we use the `validateInternalCalls` modifier on `processMessageFromRoot`.
     function _requireAdminSender() internal view override {
         require(callValidated, "Must call processMessageFromRoot");
     }

From 8f1000623afc29db66de51781979a07b506bff02 Mon Sep 17 00:00:00 2001
From: nicholaspai <npai.nyc@gmail.com>
Date: Wed, 16 Mar 2022 11:35:34 -0400
Subject: [PATCH 4/9] merge

---
 contracts/HubPool.sol          | 2 +-
 contracts/HubPoolInterface.sol | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol
index 6d0f19694..348acfdd1 100644
--- a/contracts/HubPool.sol
+++ b/contracts/HubPool.sol
@@ -814,7 +814,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 692ac40d2..4c7a1639b 100644
--- a/contracts/HubPoolInterface.sol
+++ b/contracts/HubPoolInterface.sol
@@ -112,7 +112,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 whitelistedRoute(
         uint256 originChainId,

From 90e0b70516dcfab59f2360f683b4fffbc9aca7c9 Mon Sep 17 00:00:00 2001
From: nicholaspai <npai.nyc@gmail.com>
Date: Thu, 17 Mar 2022 09:42:55 -0400
Subject: [PATCH 5/9] remove nonReentrant from _requireAdminSender

---
 contracts/Arbitrum_SpokePool.sol | 6 ------
 contracts/Ethereum_SpokePool.sol | 8 +-------
 contracts/Optimism_SpokePool.sol | 8 +-------
 contracts/Polygon_SpokePool.sol  | 8 ++++----
 contracts/SpokePool.sol          | 5 ++---
 5 files changed, 8 insertions(+), 27 deletions(-)

diff --git a/contracts/Arbitrum_SpokePool.sol b/contracts/Arbitrum_SpokePool.sol
index dafd10bae..ebf09c56e 100644
--- a/contracts/Arbitrum_SpokePool.sol
+++ b/contracts/Arbitrum_SpokePool.sol
@@ -108,11 +108,5 @@ contract Arbitrum_SpokePool is SpokePool {
     }
 
     // Apply AVM-specific transformation to cross domain admin address on L1.
-    // @dev: This is an internal method that we purposefully add a reentrancy guard to. 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.
     function _requireAdminSender() internal override onlyFromCrossDomainAdmin nonReentrant {}
 }
diff --git a/contracts/Ethereum_SpokePool.sol b/contracts/Ethereum_SpokePool.sol
index 48c044163..12ac97aee 100644
--- a/contracts/Ethereum_SpokePool.sol
+++ b/contracts/Ethereum_SpokePool.sol
@@ -35,11 +35,5 @@ contract Ethereum_SpokePool is SpokePool, Ownable {
 
     // Admin is simply owner which should be same account that owns the HubPool deployed on this network. A core
     // assumption of this contract system is that the HubPool is deployed on Ethereum.
-    // @dev: This is an internal method that we purposefully add a reentrancy guard to. 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.
-    function _requireAdminSender() internal override onlyOwner nonReentrant {}
+    function _requireAdminSender() internal override onlyOwner {}
 }
diff --git a/contracts/Optimism_SpokePool.sol b/contracts/Optimism_SpokePool.sol
index b0fb07775..2be41a32b 100644
--- a/contracts/Optimism_SpokePool.sol
+++ b/contracts/Optimism_SpokePool.sol
@@ -154,11 +154,5 @@ contract Optimism_SpokePool is CrossDomainEnabled, SpokePool {
     }
 
     // Apply OVM-specific transformation to cross domain admin address on L1.
-    // @dev: This is an internal method that we purposefully add a reentrancy guard to. 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.
-    function _requireAdminSender() internal override onlyFromCrossDomainAccount(crossDomainAdmin) nonReentrant {}
+    function _requireAdminSender() internal override onlyFromCrossDomainAccount(crossDomainAdmin) {}
 }
diff --git a/contracts/Polygon_SpokePool.sol b/contracts/Polygon_SpokePool.sol
index 9230b04ab..799ddaa8d 100644
--- a/contracts/Polygon_SpokePool.sol
+++ b/contracts/Polygon_SpokePool.sol
@@ -143,10 +143,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`. So, we don't need
-    // to explicitly reentrancy guard admin functions on this spoke pool, but we should ensure that the
-    // `processMessageFromRoot` method is reentrancy guarded which is why the `callValidated` check is made below
-    // and why we use the `validateInternalCalls` modifier on `processMessageFromRoot`.
+    // @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 035bf32c3..383b3bd45 100644
--- a/contracts/SpokePool.sol
+++ b/contracts/SpokePool.sol
@@ -25,9 +25,8 @@ import "./SpokePoolInterface.sol";
  * Relayers are refunded with destination tokens out of this contract after another off-chain actor, a "data worker",
  * submits a proof that the relayer correctly submitted a relay on this SpokePool.
  * @dev We don't use reentrancy guards in public onlyAdmin methods because these functions are expected to be called
- * via the canonical L1 --> L2 bridge. Some bridges, like Polygon's, ultimately call these functions internally, so
- * we leave it up to the overriding SpokePool contract to add reentrancy to the _requireAdminSender overridden
- * implementation.
+ * via the canonical L1 --> L2 bridge. Some bridges, like Polygon's, ultimately call these functions internally, which
+ * would always revert with a reentrancy guard.
  */
 abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCaller {
     using SafeERC20 for IERC20;

From 25ed79989da6d344d5d8e9c160eae77393e89faa Mon Sep 17 00:00:00 2001
From: nicholaspai <npai.nyc@gmail.com>
Date: Thu, 17 Mar 2022 09:44:15 -0400
Subject: [PATCH 6/9] Update Arbitrum_SpokePool.sol

---
 contracts/Arbitrum_SpokePool.sol | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contracts/Arbitrum_SpokePool.sol b/contracts/Arbitrum_SpokePool.sol
index ebf09c56e..81a59e718 100644
--- a/contracts/Arbitrum_SpokePool.sol
+++ b/contracts/Arbitrum_SpokePool.sol
@@ -108,5 +108,5 @@ contract Arbitrum_SpokePool is SpokePool {
     }
 
     // Apply AVM-specific transformation to cross domain admin address on L1.
-    function _requireAdminSender() internal override onlyFromCrossDomainAdmin nonReentrant {}
+    function _requireAdminSender() internal override onlyFromCrossDomainAdmin {}
 }

From 6c6f9179294ed55f1ee0af58162aa7a0bb867c19 Mon Sep 17 00:00:00 2001
From: nicholaspai <npai.nyc@gmail.com>
Date: Thu, 17 Mar 2022 09:55:40 -0400
Subject: [PATCH 7/9] add reentrancy guard to spoke pool

---
 contracts/Arbitrum_SpokePool.sol |  4 ++--
 contracts/Optimism_SpokePool.sol |  4 ++--
 contracts/Polygon_SpokePool.sol  | 10 +++++++---
 contracts/SpokePool.sol          | 15 ++++++---------
 4 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/contracts/Arbitrum_SpokePool.sol b/contracts/Arbitrum_SpokePool.sol
index 81a59e718..c8dbca195 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/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 799ddaa8d..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");
diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol
index 383b3bd45..ad4c95254 100644
--- a/contracts/SpokePool.sol
+++ b/contracts/SpokePool.sol
@@ -24,9 +24,6 @@ import "./SpokePoolInterface.sol";
  * on the destination chain. Locked source chain tokens are later sent over the canonical token bridge to L1 HubPool.
  * Relayers are refunded with destination tokens out of this contract after another off-chain actor, a "data worker",
  * submits a proof that the relayer correctly submitted a relay on this SpokePool.
- * @dev We don't use reentrancy guards in public onlyAdmin methods because these functions are expected to be called
- * via the canonical L1 --> L2 bridge. Some bridges, like Polygon's, ultimately call these functions internally, which
- * would always revert with a reentrancy guard.
  */
 abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCaller {
     using SafeERC20 for IERC20;
@@ -179,7 +176,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);
     }
 
@@ -187,7 +184,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);
     }
 
@@ -201,7 +198,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);
     }
@@ -210,7 +207,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);
     }
@@ -224,7 +221,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;
@@ -238,7 +235,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);
     }

From 83a3181abc654b78de59459c6ed25902c1f8ec22 Mon Sep 17 00:00:00 2001
From: nicholaspai <npai.nyc@gmail.com>
Date: Thu, 17 Mar 2022 10:04:55 -0400
Subject: [PATCH 8/9] Update HubPool.sol

---
 contracts/HubPool.sol | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol
index 8fc5ca683..6fa9aecec 100644
--- a/contracts/HubPool.sol
+++ b/contracts/HubPool.sol
@@ -511,6 +511,8 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
     ) public override nonReentrant {
         require(address(weth) == l1Token || !sendEth, "Cant send eth");
         uint256 l1TokensToReturn = (lpTokenAmount * _exchangeRateCurrent(l1Token)) / 1e18;
+
+        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 than available (i.e l1TokensToReturn > liquidReserves) this will underflow.
         pooledTokens[l1Token].liquidReserves -= l1TokensToReturn;

From 7aa2fa8f46f8d40512857f35dd3ac64587c61f18 Mon Sep 17 00:00:00 2001
From: nicholaspai <npai.nyc@gmail.com>
Date: Thu, 17 Mar 2022 14:45:42 -0400
Subject: [PATCH 9/9] Add tests

---
 .../Polygon_SpokePool.ts                      | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)

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]