improve: [L02] validate function inputs

contracts/HubPool.sol

@@ -5,7 +5,6 @@ import "./MerkleLib.sol";
 import "./HubPoolInterface.sol";
 import "./Lockable.sol";
 
-import "./interfaces/AdapterInterface.sol";
 import "./interfaces/LpTokenFactoryInterface.sol";
 import "./interfaces/WETH9.sol";
 
@@ -99,7 +98,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
 
     // Heler contracts to facilitate cross chain actions between HubPool and SpokePool for a specific network.
     struct CrossChainContract {
-        AdapterInterface adapter;
+        address adapter;
         address spokePool;
     }
     // Mapping of chainId to the associated adapter and spokePool contracts.
@@ -301,6 +300,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
         onlyOwner
     {
         require(newProtocolFeeCapturePct <= 1e18, "Bad protocolFeeCapturePct");
+        require(newProtocolFeeCaptureAddress != address(0), "Bad protocolFeeCaptureAddress");
         protocolFeeCaptureAddress = newProtocolFeeCaptureAddress;
         protocolFeeCapturePct = newProtocolFeeCapturePct;
         emit ProtocolFeeCaptureSet(newProtocolFeeCaptureAddress, newProtocolFeeCapturePct);
@@ -365,7 +365,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
         address adapter,
         address spokePool
     ) public override onlyOwner {
-        crossChainContracts[l2ChainId] = CrossChainContract(AdapterInterface(adapter), spokePool);
+        crossChainContracts[l2ChainId] = CrossChainContract(adapter, spokePool);
         emit CrossChainContractsSet(l2ChainId, adapter, spokePool);
     }
 
@@ -632,9 +632,6 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
             "Bad Proof"
         );
 
-        // Before interacting with a particular chain's adapter, ensure that the adapter is set.
-        require(address(crossChainContracts[chainId].adapter) != address(0), "No adapter for chain");
-
         // Make sure SpokePool address is initialized since _sendTokensToChainAndUpdatePooledTokenTrackers() will not
         // revert if its accidentally set to address(0). We don't make the same check on the adapter for this
         // chainId because the internal method's delegatecall() to the adapter will revert if its address is set
@@ -648,8 +645,33 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
         // Decrement the unclaimedPoolRebalanceLeafCount.
         rootBundleProposal.unclaimedPoolRebalanceLeafCount--;
 
-        _sendTokensToChainAndUpdatePooledTokenTrackers(spokePool, chainId, l1Tokens, netSendAmounts, bundleLpFees);
-        _relayRootBundleToSpokePool(spokePool, chainId);
+        // Relay each L1 token to destination chain.
+        // Note: We don't check that the adapter is initialized since if its the zero address or invalid otherwise,
+        // then the delegate call should revert.
+        address adapter = crossChainContracts[chainId].adapter;
+        _sendTokensToChainAndUpdatePooledTokenTrackers(
+            adapter,
+            spokePool,
+            chainId,
+            l1Tokens,
+            netSendAmounts,
+            bundleLpFees
+        );
+
+        // Relay root bundles to spoke pool on destination chain by
+        // performing delegatecall to use the adapter's code with this contract's context.
+        (bool success, ) = adapter.delegatecall(
+            abi.encodeWithSignature(
+                "relayMessage(address,bytes)",
+                spokePool, // target. This should be the spokePool on the L2.
+                abi.encodeWithSignature(
+                    "relayRootBundle(bytes32,bytes32)",
+                    rootBundleProposal.relayerRefundRoot,
+                    rootBundleProposal.slowRelayRoot
+                ) // message
+            )
+        );
+        require(success, "delegatecall failed");
 
         // Transfer the bondAmount to back to the proposer, if this the last executed leaf. Only sending this once all
         // leafs have been executed acts to force the data worker to execute all bundles or they wont receive their bond.
@@ -851,14 +873,13 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
     // Note this method does a lot and wraps together the sending of tokens and updating the pooled token trackers. This
     // is done as a gas saving so we don't need to iterate over the l1Tokens multiple times.
     function _sendTokensToChainAndUpdatePooledTokenTrackers(
+        address adapter,
         address spokePool,
         uint256 chainId,
         address[] memory l1Tokens,
         int256[] memory netSendAmounts,
         uint256[] memory bundleLpFees
     ) internal {
-        AdapterInterface adapter = crossChainContracts[chainId].adapter;
-
         for (uint32 i = 0; i < l1Tokens.length; i++) {
             address l1Token = l1Tokens[i];
             // Validate the L1 -> L2 token route is whitelisted. If it is not then the output of the bridging action
@@ -871,7 +892,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
             if (netSendAmounts[i] > 0) {
                 // Perform delegatecall to use the adapter's code with this contract's context. Opt for delegatecall's
                 // complexity in exchange for lower gas costs.
-                (bool success, ) = address(adapter).delegatecall(
+                (bool success, ) = adapter.delegatecall(
                     abi.encodeWithSignature(
                         "relayTokens(address,address,uint256,address)",
                         l1Token, // l1Token.
@@ -892,24 +913,6 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
         }
     }
 
-    function _relayRootBundleToSpokePool(address spokePool, uint256 chainId) internal {
-        AdapterInterface adapter = crossChainContracts[chainId].adapter;
-
-        // Perform delegatecall to use the adapter's code with this contract's context.
-        (bool success, ) = address(adapter).delegatecall(
-            abi.encodeWithSignature(
-                "relayMessage(address,bytes)",
-                spokePool, // target. This should be the spokePool on the L2.
-                abi.encodeWithSignature(
-                    "relayRootBundle(bytes32,bytes32)",
-                    rootBundleProposal.relayerRefundRoot,
-                    rootBundleProposal.slowRelayRoot
-                ) // message
-            )
-        );
-        require(success, "delegatecall failed");
-    }
-
     function _exchangeRateCurrent(address l1Token) internal returns (uint256) {
         PooledToken storage pooledToken = pooledTokens[l1Token]; // Note this is storage so the state can be modified.
         uint256 lpTokenTotalSupply = IERC20(pooledToken.lpToken).totalSupply();
@@ -1002,14 +1005,15 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
     }
 
     function _relaySpokePoolAdminFunction(uint256 chainId, bytes memory functionData) internal {
-        AdapterInterface adapter = crossChainContracts[chainId].adapter;
-        require(address(adapter) != address(0), "Adapter not initialized");
+        address adapter = crossChainContracts[chainId].adapter;
+        address spokePool = crossChainContracts[chainId].spokePool;
+        require(spokePool != address(0), "SpokePool not initialized");
 
         // Perform delegatecall to use the adapter's code with this contract's context.
-        (bool success, ) = address(adapter).delegatecall(
+        (bool success, ) = adapter.delegatecall(
             abi.encodeWithSignature(
                 "relayMessage(address,bytes)",
-                crossChainContracts[chainId].spokePool, // target. This should be the spokePool on the L2.
+                spokePool, // target. This should be the spokePool on the L2.
                 functionData
             )
         );

contracts/MerkleLib.sol

@@ -82,21 +82,21 @@ library MerkleLib {
     /**
      * @notice Tests whether a claim is contained within a 1D claimedBitMap mapping.
      * @param claimedBitMap a simple uint256 value, encoding a 1D bitmap.
-     * @param index the index to check in the bitmap.
-     \* @return bool indicating if the index within the claimedBitMap has been marked as claimed.
+     * @param index the index to check in the bitmap. Uint8 type enforces that index can't be > 255.
+     * @return bool indicating if the index within the claimedBitMap has been marked as claimed.
      */
-    function isClaimed1D(uint256 claimedBitMap, uint256 index) internal pure returns (bool) {
+    function isClaimed1D(uint256 claimedBitMap, uint8 index) internal pure returns (bool) {
         uint256 mask = (1 << index);
         return claimedBitMap & mask == mask;
     }
 
     /**
      * @notice Marks an index in a claimedBitMap as claimed.
-     * @param claimedBitMap a simple uint256 mapping in storage used as a bitmap.
+     * @param claimedBitMap a simple uint256 mapping in storage used as a bitmap. Uint8 type enforces that index
+     * can't be > 255.
      * @param index the index to mark in the bitmap.
      */
-    function setClaimed1D(uint256 claimedBitMap, uint256 index) internal pure returns (uint256) {
-        require(index <= 255, "Index out of bounds");
+    function setClaimed1D(uint256 claimedBitMap, uint8 index) internal pure returns (uint256) {
         return claimedBitMap | (1 << index % 256);
     }
 }

contracts/PolygonTokenBridger.sol

@@ -54,20 +54,20 @@ contract PolygonTokenBridger is Lockable {
      * @notice Called by Polygon SpokePool to send tokens over bridge to contract with the same address as this.
      * @param token Token to bridge.
      * @param amount Amount to bridge.
-     * @param isMatic True if token is MATIC.
+     * @param isWrappedMatic True if token is WMATIC.
      */
     function send(
         PolygonIERC20 token,
         uint256 amount,
-        bool isMatic
+        bool isWrappedMatic
     ) public nonReentrant {
         token.safeTransferFrom(msg.sender, address(this), amount);
 
         // In the wMatic case, this unwraps. For other ERC20s, this is the burn/send action.
         token.withdraw(amount);
 
         // This takes the token that was withdrawn and calls withdraw on the "native" ERC20.
-        if (isMatic) maticToken.withdraw{ value: amount }(amount);
+        if (isWrappedMatic) maticToken.withdraw{ value: amount }(amount);
     }
 
     /**

contracts/SpokePool.sol

@@ -41,9 +41,6 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall
     // instruct this contract to wrap ETH when depositing.
     WETH9 public weth;
 
-    // Timestamp when contract was constructed. Relays cannot have a quote time before this.
-    uint32 public deploymentTime;
-
     // Any deposit quote times greater than or less than this value to the current contract time is blocked. Forces
     // caller to use an approximately "current" realized fee. Defaults to 10 minutes.
     uint32 public depositQuoteTimeBuffer = 600;
@@ -152,7 +149,6 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall
     ) Testable(timerAddress) {
         _setCrossDomainAdmin(_crossDomainAdmin);
         _setHubPool(_hubPool);
-        deploymentTime = uint32(getCurrentTime());
         weth = WETH9(_wethAddress);
     }
 
@@ -333,6 +329,8 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall
         uint32 depositId,
         bytes memory depositorSignature
     ) public override nonReentrant {
+        require(newRelayerFeePct < 0.5e18, "invalid relayer fee");
+
         _verifyUpdateRelayerFeeMessage(depositor, chainId(), newRelayerFeePct, depositId, depositorSignature);
 
         // Assuming the above checks passed, a relayer can take the signature and the updated relayer fee information

contracts/test/MerkleLibTest.sol

@@ -45,11 +45,11 @@ contract MerkleLibTest {
         MerkleLib.setClaimed(claimedBitMap, index);
     }
 
-    function isClaimed1D(uint256 index) public view returns (bool) {
+    function isClaimed1D(uint8 index) public view returns (bool) {
         return MerkleLib.isClaimed1D(claimedBitMap1D, index);
     }
 
-    function setClaimed1D(uint256 index) public {
+    function setClaimed1D(uint8 index) public {
         claimedBitMap1D = MerkleLib.setClaimed1D(claimedBitMap1D, index);
     }
 }

test/HubPool.Admin.ts

@@ -47,6 +47,22 @@ describe("HubPool Admin functions", function () {
       hubPool.connect(other).setCrossChainContracts(destinationChainId, mockAdapter.address, mockSpoke.address)
     ).to.be.reverted;
   });
+  it("Only owner can relay spoke pool admin message", async function () {
+    const functionData = mockSpoke.interface.encodeFunctionData("setEnableRoute", [
+      weth.address,
+      destinationChainId,
+      false,
+    ]);
+    await expect(hubPool.connect(other).relaySpokePoolAdminFunction(destinationChainId, functionData)).to.be.reverted;
+
+    // Cannot relay admin function if spoke pool is set to zero address.
+    await hubPool.setCrossChainContracts(destinationChainId, mockAdapter.address, ZERO_ADDRESS);
+    await expect(hubPool.relaySpokePoolAdminFunction(destinationChainId, functionData)).to.be.reverted;
+    await hubPool.setCrossChainContracts(destinationChainId, mockAdapter.address, mockSpoke.address);
+    await expect(hubPool.relaySpokePoolAdminFunction(destinationChainId, functionData))
+      .to.emit(hubPool, "SpokePoolAdminFunctionTriggered")
+      .withArgs(destinationChainId, functionData);
+  });
   it("Only owner can whitelist route for deposits and rebalances", async function () {
     await hubPool.setCrossChainContracts(destinationChainId, mockAdapter.address, mockSpoke.address);
     await expect(

test/HubPool.ProtocolFees.ts

@@ -2,6 +2,7 @@ import { toWei, toBNWei, SignerWithAddress, seedWallet, expect, Contract, ethers
 import { mockTreeRoot, finalFee, bondAmount, amountToLp, refundProposalLiveness } from "./constants";
 import { hubPoolFixture, enableTokensForLP } from "./fixtures/HubPool.Fixture";
 import { constructSingleChainTree } from "./MerkleLib.utils";
+import { ZERO_ADDRESS } from "@uma/common";
 
 let hubPool: Contract, weth: Contract, timer: Contract;
 let owner: SignerWithAddress, dataWorker: SignerWithAddress, liquidityProvider: SignerWithAddress;
@@ -31,6 +32,10 @@ describe("HubPool Protocol fees", function () {
     expect(await hubPool.callStatic.protocolFeeCaptureAddress()).to.equal(owner.address);
     expect(await hubPool.callStatic.protocolFeeCapturePct()).to.equal(initialProtocolFeeCapturePct);
     const newPct = toWei("0.1");
+
+    // Can't set to 0 address
+    await expect(hubPool.connect(owner).setProtocolFeeCapture(ZERO_ADDRESS, newPct)).to.be.reverted;
+
     await hubPool.connect(owner).setProtocolFeeCapture(liquidityProvider.address, newPct);
     expect(await hubPool.callStatic.protocolFeeCaptureAddress()).to.equal(liquidityProvider.address);
     expect(await hubPool.callStatic.protocolFeeCapturePct()).to.equal(newPct);

test/MerkleLib.Claims.ts

@@ -60,7 +60,7 @@ describe("MerkleLib Claims", async function () {
 
     // Setting right at the max should revert.
     await expect(merkleLibTest.setClaimed1D(256)).to.be.reverted;
-    expect(await merkleLibTest.isClaimed1D(255)).to.equal(false);
+    await expect(merkleLibTest.isClaimed1D(256)).to.be.reverted;
 
     // Should be able to set right below the max.
     expect(await merkleLibTest.isClaimed1D(255)).to.equal(false);

test/SpokePool.Relay.ts

@@ -176,6 +176,12 @@ describe("SpokePool Relayer Logic", async function () {
       spokePoolChainId.toString(),
       depositor
     );
+
+    // Cannot set new relayer fee pct >= 50%
+    await expect(
+      spokePool.connect(relayer).speedUpDeposit(depositor.address, toWei("0.5"), consts.firstDepositId, signature)
+    ).to.be.revertedWith("invalid relayer fee");
+
     await expect(
       spokePool
         .connect(relayer)