refactor: [M04/L03/N16] whitelistedRoutes should distinguish between enabled deposit and poolRebalance paths

contracts/Arbitrum_SpokePool.sol

@@ -77,8 +77,11 @@ contract Arbitrum_SpokePool is SpokePool {
      **************************************/
 
     function _bridgeTokensToHubPool(RelayerRefundLeaf memory relayerRefundLeaf) internal override {
+        // Check that the Ethereum counterpart of the L2 token is stored on this contract.
+        address ethereumTokenToBridge = whitelistedTokens[relayerRefundLeaf.l2TokenAddress];
+        require(ethereumTokenToBridge != address(0), "Uninitialized mainnet token");
         StandardBridgeLike(l2GatewayRouter).outboundTransfer(
-            whitelistedTokens[relayerRefundLeaf.l2TokenAddress], // _l1Token. Address of the L1 token to bridge over.
+            ethereumTokenToBridge, // _l1Token. Address of the L1 token to bridge over.
             hubPool, // _to. Withdraw, over the bridge, to the l1 hub pool contract.
             relayerRefundLeaf.amountToReturn, // _amount.
             "" // _data. We don't need to send any data for the bridging action.

contracts/HubPool.sol

@@ -77,9 +77,13 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
     // Whether the bundle proposal process is paused.
     bool public paused;
 
-    // Whitelist of origin token + ID to destination token routings to be used by off-chain agents. The notion of a
-    // route does not need to include L1; it can be L2->L2 route. i.e USDC on Arbitrum -> USDC on Optimism as a "route".
-    mapping(bytes32 => address) private whitelistedRoutes;
+    // Stores paths from L1 token + destination ID to destination token. Since different tokens on L1 might map to
+    // to the same address on different destinations, we hash (L1 token address, destination ID) to
+    // use as a key that maps to a destination token. This mapping is used to direct pool rebalances from
+    // HubPool to SpokePool, and also is designed to be used as a lookup for off-chain data workers to determine
+    // which L1 tokens to relay to SpokePools to refund relayers. The admin can set the "destination token"
+    // to 0x0 to disable a pool rebalance route and block executeRootBundle() from executing.
+    mapping(bytes32 => address) private poolRebalanceRoutes;
 
     struct PooledToken {
         // LP token given to LPs of a specific L1 token.
@@ -179,14 +183,17 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
         uint256 lpTokensBurnt,
         address indexed liquidityProvider
     );
-    event WhitelistRoute(
+    event SetPoolRebalanceRoute(
+        uint256 indexed destinationChainId,
+        address indexed l1Token,
+        address indexed destinationToken
+    );
+    event SetEnableDepositRoute(
         uint256 indexed originChainId,
         uint256 indexed destinationChainId,
         address indexed originToken,
-        address destinationToken,
-        bool enableRoute
+        bool depositsEnabled
     );
-
     event ProposeRootBundle(
         uint32 requestExpirationTimestamp,
         uint64 unclaimedPoolRebalanceLeafCount,
@@ -360,11 +367,11 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
 
     /**
      * @notice Sets cross chain relay helper contracts for L2 chain ID. Callable only by owner.
-     * @dev We do not block setting the adapter or spokepool to invalid/zero addresses because we want to allow the
+     * @dev We do not block setting the adapter or SpokePool to invalid/zero addresses because we want to allow the
      * admin to block relaying roots to the spoke pool for emergency recovery purposes.
      * @param l2ChainId Chain to set contracts for.
      * @param adapter Adapter used to relay messages and tokens to spoke pool. Deployed on current chain.
-     * @param spokePool Recipient of relayed messages and tokens on SpokePool. Deployed on l2ChainId.
+     * @param spokePool Recipient of relayed messages and tokens on spoke pool. Deployed on l2ChainId.
      */
 
     function setCrossChainContracts(
@@ -377,39 +384,53 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
     }
 
     /**
-     * @notice Whitelist an origin chain ID + token <-> destination token route. Callable only by owner.
-     * @param originChainId Chain where deposit occurs.
-     * @param destinationChainId Chain where depositor wants to receive funds.
-     * @param originToken Deposited token.
-     * @param destinationToken Token that depositor wants to receive on destination chain. Unused if `enableRoute` is
-     * False.
-     * @param enableRoute Set to true to enable route on L2 and whitelist new destination token, or False to disable
-     * route on L2 and delete destination token mapping on this contract.
+     * @notice Store canonical destination token counterpart for l1 token. Callable only by owner.
+     * @dev Admin can set destinationToken to 0x0 to effectively disable executing any root bundles with leaves
+     * containing this l1 token + destination chain ID combination.
+     * @param destinationChainId Destination chain where destination token resides.
+     * @param l1Token Token enabled for liquidity in this pool, and the L1 counterpart to the destination token on the
+     * destination chain ID.
+     * @param destinationToken Destination chain counterpart of L1 token.
      */
-    function whitelistRoute(
-        uint256 originChainId,
+    function setPoolRebalanceRoute(
         uint256 destinationChainId,
-        address originToken,
-        address destinationToken,
-        bool enableRoute
+        address l1Token,
+        address destinationToken
     ) public override onlyOwner nonReentrant {
-        if (enableRoute)
-            whitelistedRoutes[_whitelistedRouteKey(originChainId, originToken, destinationChainId)] = destinationToken;
-        else delete whitelistedRoutes[_whitelistedRouteKey(originChainId, originToken, destinationChainId)];
+        poolRebalanceRoutes[_poolRebalanceRouteKey(l1Token, destinationChainId)] = destinationToken;
+        emit SetPoolRebalanceRoute(destinationChainId, l1Token, destinationToken);
+    }
 
-        // Whitelist the same route on the origin network.
+    /**
+     * @notice Sends cross-chain message to SpokePool on originChainId to enable or disable deposit route from that
+     * SpokePool to another one. Callable only by owner.
+     * @dev Admin is responsible for ensuring that `originToken` is linked to some L1 token on this contract, via
+     * poolRebalanceRoutes(), and that this L1 token also has a counterpart on the destination chain. If either
+     * condition fails, then the deposit will be unrelayable by off-chain relayers because they will not know which
+     * token to relay to recipients on the destination chain, and data workers wouldn't know which L1 token to send
+     * to the destination chain to refund the relayer.
+     * @param originChainId Chain where token deposit occurs.
+     * @param destinationChainId Chain where token depositor wants to receive funds.
+     * @param originToken Token sent in deposit.
+     * @param depositsEnabled Set to true to whitelist this route for deposits, set to false if caller just wants to
+     * map the origin token + destination ID to the destination token address on the origin chain's SpokePool.
+     */
+    function setDepositRoute(
+        uint256 originChainId,
+        uint256 destinationChainId,
+        address originToken,
+        bool depositsEnabled
+    ) public override nonReentrant onlyOwner {
         _relaySpokePoolAdminFunction(
             originChainId,
             abi.encodeWithSignature(
                 "setEnableRoute(address,uint256,bool)",
                 originToken,
                 destinationChainId,
-                enableRoute
+                depositsEnabled
             )
         );
-
-        // @dev Client should ignore `destinationToken` value if `enableRoute == False`.
-        emit WhitelistRoute(originChainId, destinationChainId, originToken, destinationToken, enableRoute);
+        emit SetEnableDepositRoute(originChainId, destinationChainId, originToken, depositsEnabled);
     }
 
     /**
@@ -658,6 +679,11 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
         rootBundleProposal.unclaimedPoolRebalanceLeafCount--;
 
         // Relay each L1 token to destination chain.
+        // Note: if any of the keccak256(l1Tokens, chainId) combinations are not mapped to a destination token address,
+        // then this internal method will revert. In this case the admin will have to associate a destination token
+        // with each l1 token. If the destination token mapping was missing at the time of the proposal, we assume
+        // that the root bundle would have been disputed because the off-chain data worker would have been unable to
+        // determine if the relayers used the correct destination token for a given origin token.
         _sendTokensToChainAndUpdatePooledTokenTrackers(
             adapter,
             spokePool,
@@ -813,19 +839,20 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
     }
 
     /**
-     * @notice Conveniently queries whether an origin chain + token => destination chain ID is whitelisted and returns
-     * the whitelisted destination token.
-     * @param originChainId Deposit chain.
-     * @param originToken Deposited token.
-     * @param destinationChainId Where depositor can receive funds.
-     * @return address Depositor can receive this token on destination chain ID.
+     * @notice Conveniently queries which destination token is mapped to the hash of an l1 token +
+     * destination chain ID.
+     * @param destinationChainId Where destination token is deployed.
+     * @param l1Token Ethereum version token.
+     * @return destinationToken address The destination token that is sent to spoke pools after this contract bridges
+     * the l1Token to the destination chain.
      */
-    function whitelistedRoute(
-        uint256 originChainId,
-        address originToken,
-        uint256 destinationChainId
-    ) public view override returns (address) {
-        return whitelistedRoutes[_whitelistedRouteKey(originChainId, originToken, destinationChainId)];
+    function poolRebalanceRoute(uint256 destinationChainId, address l1Token)
+        external
+        view
+        override
+        returns (address destinationToken)
+    {
+        return poolRebalanceRoutes[_poolRebalanceRouteKey(l1Token, destinationChainId)];
     }
 
     /**
@@ -880,9 +907,9 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
     ) internal {
         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
+            // Validate the L1 -> L2 token route is stored. If it is not then the output of the bridging action
             // could send tokens to the 0x0 address on the L2.
-            address l2Token = whitelistedRoutes[_whitelistedRouteKey(block.chainid, l1Token, chainId)];
+            address l2Token = poolRebalanceRoutes[_poolRebalanceRouteKey(l1Token, chainId)];
             require(l2Token != address(0), "Route not whitelisted");
 
             // If the net send amount for this token is positive then: 1) send tokens from L1->L2 to facilitate the L2
@@ -1017,6 +1044,10 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
         emit SpokePoolAdminFunctionTriggered(chainId, functionData);
     }
 
+    function _poolRebalanceRouteKey(address l1Token, uint256 destinationChainId) internal pure returns (bytes32) {
+        return keccak256(abi.encode(l1Token, destinationChainId));
+    }
+
     function _getInitializedCrossChainContracts(uint256 chainId)
         internal
         view
@@ -1028,14 +1059,6 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable {
         require(adapter.isContract(), "Adapter not initialized");
     }
 
-    function _whitelistedRouteKey(
-        uint256 originChainId,
-        address originToken,
-        uint256 destinationChainId
-    ) internal pure returns (bytes32) {
-        return keccak256(abi.encode(originChainId, originToken, destinationChainId));
-    }
-
     function _activeRequest() internal view returns (bool) {
         return rootBundleProposal.unclaimedPoolRebalanceLeafCount != 0;
     }

contracts/HubPoolInterface.sol

@@ -61,14 +61,6 @@ interface HubPoolInterface {
         address spokePool
     ) external;
 
-    function whitelistRoute(
-        uint256 originChainId,
-        uint256 destinationChainId,
-        address originToken,
-        address destinationToken,
-        bool enableRoute
-    ) external;
-
     function enableL1TokenForLiquidityProvision(address l1Token) external;
 
     function disableL1TokenForLiquidityProvision(address l1Token) external;
@@ -114,11 +106,23 @@ interface HubPoolInterface {
 
     function getRootBundleProposalAncillaryData() external view returns (bytes memory ancillaryData);
 
-    function whitelistedRoute(
+    function setPoolRebalanceRoute(
+        uint256 destinationChainId,
+        address l1Token,
+        address destinationToken
+    ) external;
+
+    function setDepositRoute(
         uint256 originChainId,
+        uint256 destinationChainId,
         address originToken,
-        uint256 destinationChainId
-    ) external view returns (address);
+        bool depositsEnabled
+    ) external;
+
+    function poolRebalanceRoute(uint256 destinationChainId, address l1Token)
+        external
+        view
+        returns (address destinationToken);
 
     function loadEthForL2Calls() external payable;
 }

contracts/SpokePool.sol

@@ -49,8 +49,6 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall
     uint32 public numberOfDeposits;
 
     // Origin token to destination token routings can be turned on or off, which can enable or disable deposits.
-    // A reverse mapping is stored on the L1 HubPool to enable or disable rebalance transfers from the HubPool to this
-    // contract.
     mapping(address => mapping(uint256 => bool)) public enabledDepositRoutes;
 
     // Stores collection of merkle roots that can be published to this contract from the HubPool, which are referenced
@@ -156,11 +154,6 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall
      *               MODIFIERS              *
      ****************************************/
 
-    modifier onlyEnabledRoute(address originToken, uint256 destinationId) {
-        require(enabledDepositRoutes[originToken][destinationId], "Disabled route");
-        _;
-    }
-
     // Implementing contract needs to override _requireAdminSender() to ensure that admin functions are protected
     // appropriately.
     modifier onlyAdmin() {
@@ -267,7 +260,10 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall
         uint256 destinationChainId,
         uint64 relayerFeePct,
         uint32 quoteTimestamp
-    ) public payable override onlyEnabledRoute(originToken, destinationChainId) nonReentrant {
+    ) public payable override nonReentrant {
+        // Check that deposit route is enabled.
+        require(enabledDepositRoutes[originToken][destinationChainId], "Disabled route");
+
         // We limit the relay fees to prevent the user spending all their funds on fees.
         require(relayerFeePct < 0.5e18, "invalid relayer fee");
         // This function assumes that L2 timing cannot be compared accurately and consistently to L1 timing. Therefore,

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@across-protocol/contracts-v2",
-  "version": "0.0.27",
+  "version": "0.0.28",
   "author": "UMA Team",
   "license": "AGPL-3.0",
   "repository": {

test/HubPool.Admin.ts

@@ -81,21 +81,21 @@ describe("HubPool Admin functions", function () {
   });
   it("Only owner can whitelist route for deposits and rebalances", async function () {
     await hubPool.setCrossChainContracts(destinationChainId, mockAdapter.address, mockSpoke.address);
-    await expect(
-      hubPool.connect(other).whitelistRoute(originChainId, destinationChainId, weth.address, usdc.address, true)
-    ).to.be.reverted;
-    await expect(hubPool.whitelistRoute(originChainId, destinationChainId, weth.address, usdc.address, true))
-      .to.emit(hubPool, "WhitelistRoute")
-      .withArgs(originChainId, destinationChainId, weth.address, usdc.address, true);
-
-    expect(await hubPool.whitelistedRoute(originChainId, weth.address, destinationChainId)).to.equal(usdc.address);
+    await expect(hubPool.connect(other).setPoolRebalanceRoute(destinationChainId, weth.address, usdc.address)).to.be
+      .reverted;
+    await expect(hubPool.setPoolRebalanceRoute(destinationChainId, weth.address, usdc.address))
+      .to.emit(hubPool, "SetPoolRebalanceRoute")
+      .withArgs(destinationChainId, weth.address, usdc.address);
 
-    // Can disable a route.
-    await hubPool.whitelistRoute(originChainId, destinationChainId, weth.address, usdc.address, false);
-    expect(await hubPool.whitelistedRoute(originChainId, weth.address, destinationChainId)).to.equal(ZERO_ADDRESS);
+    // Relay deposit route to spoke pool. Check content of messages sent to mock spoke pool.
+    await expect(hubPool.connect(other).setDepositRoute(originChainId, destinationChainId, weth.address, true)).to.be
+      .reverted;
+    await expect(hubPool.setDepositRoute(originChainId, destinationChainId, weth.address, true))
+      .to.emit(hubPool, "SetEnableDepositRoute")
+      .withArgs(originChainId, destinationChainId, weth.address, true);
 
-    // Check content of messages sent to mock spoke pool. The last call should have "disabled" a route, and the call
-    // right before should have enabled the route.
+    // Disable deposit route on SpokePool right after:
+    await hubPool.setDepositRoute(originChainId, destinationChainId, weth.address, false);
 
     // Since the mock adapter is delegatecalled, when querying, its address should be the hubPool address.
     const mockAdapterAtHubPool = mockAdapter.attach(hubPool.address);
@@ -106,14 +106,14 @@ describe("HubPool Admin functions", function () {
       mockSpoke.interface.encodeFunctionData("setEnableRoute", [
         weth.address,
         destinationChainId,
-        false, // Should be set to false to disable route on SpokePool
+        false, // Latest call disabled the route
       ])
     );
     expect(relayMessageEvents[relayMessageEvents.length - 2].args?.message).to.equal(
       mockSpoke.interface.encodeFunctionData("setEnableRoute", [
         weth.address,
         destinationChainId,
-        true, // Should be set to true because destination token wasn't 0x0
+        true, // Second to last call enabled the route
       ])
     );
   });