From 85b944daf0c88c45ca21dc0c3e002ac7fcfa25bc Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Mon, 14 Mar 2022 16:55:57 -0400 Subject: [PATCH 01/20] feat: Refactor whitelistedRoutes struct --- contracts/HubPool.sol | 111 ++++++++++++++------ contracts/HubPoolInterface.sol | 29 +++-- test/HubPool.Admin.ts | 23 ++-- test/HubPool.ExecuteRootBundle.ts | 3 +- test/chain-adapters/Arbitrum_Adapter.ts | 5 +- test/gas-analytics/HubPool.RootExecution.ts | 5 +- 6 files changed, 118 insertions(+), 58 deletions(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index c10d7fb4a..413d526f6 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -74,9 +74,24 @@ 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; + // A Route is mapped to a hash of an origin chain ID, origin token address, and destination chain ID. Those three + // values should point to a unique token address existing on the destination chain. This route is used to determine + // which token to send from this contract to a spoke pool to execute a "pool rebalance" in the case where the origin + // chain ID is this network (i.e. origin chain ID = 1). This route is also used to keep track of which deposit + // routes are enabled/disabled. + struct Route { + address destinationToken; + bool depositsEnabled; + } + + // Stores paths from origin token + ID to destination token + ID. Since different tokens on an origin might map to + // to the same address on a destination, we hash (origin chain ID, origin token address, destination ID) to + // use as a key that maps to a destination token. This means that routes are stored "one-way" and each route will + // likely require its "reverse" route to be stored as well. For example, we would want to store USDC on Ethereum + // to USDC on Optimism. This means that we'll map: keccak256(1, USDC-ethereum, 10) => USDC-optimism, and + // keccak(10, USDC-optimism, 1) => USDC-ethereum. Note that not all routes are enabled, but we might still want + // to know what the destination token address is, for rebalances for example. + mapping(bytes32 => Route) private whitelistedRoutes; struct PooledToken { // LP token given to LPs of a specific L1 token. @@ -181,9 +196,14 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { uint256 indexed destinationChainId, address indexed originToken, address destinationToken, - bool enableRoute + bool depositsEnabled + ); + event RelayWhitelistRoute( + uint256 indexed originChainId, + uint256 indexed destinationChainId, + address indexed originToken, + bool depositsEnabled ); - event ProposeRootBundle( uint32 requestExpirationTimestamp, uint64 unclaimedPoolRebalanceLeafCount, @@ -370,39 +390,61 @@ 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 Stores mapping from origin chain ID + token + destination chain ID to token. This mapping corresponds + * to token transfer routes and can be used for both HubPool => SpokePool rebalances and SpokePool ==> SpokePool + * deposits. Callable only by owner. + * @param originChainId Chain where token transfer originates. + * @param destinationChainId Chain where token transfer ends up. + * @param originToken Token sent from origin chain. + * @param destinationToken Token received at destination chain. + * @param depositsEnabled Set to true to whitelist this route for deposits, set to false if caller just wants to + * map the origin token + origin ID + destination ID to the destination token address and wants to explicitly + * disable this route for deposits. */ + function whitelistRoute( uint256 originChainId, uint256 destinationChainId, address originToken, address destinationToken, - bool enableRoute - ) public override onlyOwner nonReentrant { - if (enableRoute) - whitelistedRoutes[_whitelistedRouteKey(originChainId, originToken, destinationChainId)] = destinationToken; - else delete whitelistedRoutes[_whitelistedRouteKey(originChainId, originToken, destinationChainId)]; + bool depositsEnabled + ) public override onlyOwner { + whitelistedRoutes[_whitelistedRouteKey(originChainId, originToken, destinationChainId)] = Route({ + destinationToken: destinationToken, + depositsEnabled: depositsEnabled + }); + emit WhitelistRoute(originChainId, destinationChainId, originToken, destinationToken, depositsEnabled); + } - // Whitelist the same route on the origin network. + /** + * @notice Send cross-chain message to SpokePool on originChainId to enable or disable mapping from origin + * origin token to destination chain ID. If this path is enabled on the SpokePool, then relays are allowed to be + * sent via that path. The destination token address mapped to the origin token can be set by `whitelistRoute` and + * fetched by off-chain relayers. The SpokePool doesn't need to know which destination tokens are mapped to origin + * tokens as this protocol assumes that relayers use the correct whitelisted route. Callable only by owner, + * who has the responsibility to make sure that calls to `whitelistRoute` are made in concert with these calls, + * otherwise the enabled path information will become out of sync between SpokePool and HubPool. + * @param originChainId Chain where token transfer originates. + * @param destinationChainId Chain where token transfer ends up. + * @param originToken Token sent from origin chain. + * @param depositsEnabled Set to True/False to enable/disable route on SpokePool for deposits. + */ + function relayWhitelistRoute( + uint256 originChainId, + uint256 destinationChainId, + address originToken, + bool depositsEnabled + ) public override onlyOwner nonReentrant { _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 RelayWhitelistRoute(originChainId, destinationChainId, originToken, depositsEnabled); } /** @@ -793,19 +835,22 @@ 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. + * @notice Conveniently queries whether an origin chain + token => destination chain ID is enabled for deposits and + * the mapped 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. + * @param originToken Deposited token. + * @return destinationToken address Depositor can receive this token on destination chain. + * @return depositsEnabled boolean Whether deposits are enabled on SpokePool on origin chain. */ function whitelistedRoute( uint256 originChainId, - address originToken, - uint256 destinationChainId - ) public view override returns (address) { - return whitelistedRoutes[_whitelistedRouteKey(originChainId, originToken, destinationChainId)]; + uint256 destinationChainId, + address originToken + ) public view override returns (address destinationToken, bool depositsEnabled) { + Route memory route = whitelistedRoutes[_whitelistedRouteKey(originChainId, originToken, destinationChainId)]; + destinationToken = route.destinationToken; + depositsEnabled = route.depositsEnabled; } /** @@ -861,9 +906,9 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { 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 = whitelistedRoutes[_whitelistedRouteKey(block.chainid, l1Token, chainId)].destinationToken; 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 diff --git a/contracts/HubPoolInterface.sol b/contracts/HubPoolInterface.sol index a13f61193..a59cdb423 100644 --- a/contracts/HubPoolInterface.sol +++ b/contracts/HubPoolInterface.sol @@ -54,14 +54,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; @@ -106,11 +98,26 @@ interface HubPoolInterface { function getRootBundleProposalAncillaryData() external view returns (bytes memory ancillaryData); - function whitelistedRoute( + function whitelistRoute( uint256 originChainId, + uint256 destinationChainId, address originToken, - uint256 destinationChainId - ) external view returns (address); + address destinationToken, + bool depositsEnabled + ) external; + + function relayWhitelistRoute( + uint256 originChainId, + uint256 destinationChainId, + address originToken, + bool depositsEnabled + ) external; + + function whitelistedRoute( + uint256 originChainId, + uint256 destinationChainId, + address originToken + ) external view returns (address destinationToken, bool depositsEnabled); function loadEthForL2Calls() external payable; } diff --git a/test/HubPool.Admin.ts b/test/HubPool.Admin.ts index a718e591e..8175075b6 100644 --- a/test/HubPool.Admin.ts +++ b/test/HubPool.Admin.ts @@ -3,7 +3,6 @@ import { Contract, ethers, randomAddress, utf8ToHex } from "./utils"; import { originChainId, destinationChainId, bondAmount, zeroAddress, mockTreeRoot } from "./constants"; import { mockSlowRelayRoot, finalFeeUsdc, finalFee, totalBond } from "./constants"; import { hubPoolFixture } from "./fixtures/HubPool.Fixture"; -import { ZERO_ADDRESS } from "@uma/common"; let hubPool: Contract, weth: Contract, @@ -55,15 +54,23 @@ describe("HubPool Admin functions", function () { 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); + let whitelistedRoute = await hubPool.whitelistedRoute(originChainId, destinationChainId, weth.address); + expect(whitelistedRoute.destinationToken).to.equal(usdc.address); + expect(whitelistedRoute.depositsEnabled).to.equal(true); // 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); + whitelistedRoute = await hubPool.whitelistedRoute(originChainId, destinationChainId, weth.address); + expect(whitelistedRoute.destinationToken).to.equal(usdc.address); + expect(whitelistedRoute.depositsEnabled).to.equal(false); - // 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. + // Relay whitelist mapping to spoke pool. Check content of messages sent to mock spoke pool. + await expect(hubPool.connect(other).relayWhitelistRoute(originChainId, destinationChainId, weth.address, true)).to + .be.reverted; + await expect(hubPool.relayWhitelistRoute(originChainId, destinationChainId, weth.address, true)) + .to.emit(hubPool, "RelayWhitelistRoute") + .withArgs(originChainId, destinationChainId, weth.address, true); + await hubPool.relayWhitelistRoute(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); @@ -74,14 +81,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 ]) ); }); diff --git a/test/HubPool.ExecuteRootBundle.ts b/test/HubPool.ExecuteRootBundle.ts index e69c9d47f..89204e804 100644 --- a/test/HubPool.ExecuteRootBundle.ts +++ b/test/HubPool.ExecuteRootBundle.ts @@ -69,8 +69,7 @@ describe("HubPool Root Bundle Execution", function () { const relayMessageEvents = await mockAdapterAtHubPool.queryFilter( mockAdapterAtHubPool.filters.RelayMessageCalled() ); - expect(relayMessageEvents.length).to.equal(7); // Exactly seven message send from L1->L2. 6 for each whitelist route - // and 1 for the initiateRelayerRefund. + expect(relayMessageEvents.length).to.equal(1); // Exactly one message sent from L1->L2. expect(relayMessageEvents[relayMessageEvents.length - 1].args?.target).to.equal(mockSpoke.address); expect(relayMessageEvents[relayMessageEvents.length - 1].args?.message).to.equal( mockSpoke.interface.encodeFunctionData("relayRootBundle", [ diff --git a/test/chain-adapters/Arbitrum_Adapter.ts b/test/chain-adapters/Arbitrum_Adapter.ts index e80e4a03d..6019bad59 100644 --- a/test/chain-adapters/Arbitrum_Adapter.ts +++ b/test/chain-adapters/Arbitrum_Adapter.ts @@ -63,7 +63,7 @@ describe("Arbitrum Chain Adapter", function () { expect(await hubPool.relaySpokePoolAdminFunction(arbitrumChainId, functionCallData)) .to.emit(arbitrumAdapter.attach(hubPool.address), "MessageRelayed") .withArgs(mockSpoke.address, functionCallData); - expect(l1Inbox.createRetryableTicket).to.have.been.calledThrice; + expect(l1Inbox.createRetryableTicket).to.have.been.calledOnce; expect(l1Inbox.createRetryableTicket).to.have.been.calledWith( mockSpoke.address, 0, @@ -94,8 +94,7 @@ describe("Arbitrum Chain Adapter", function () { consts.sampleL2GasPrice, "0x" ); - expect(l1Inbox.createRetryableTicket).to.have.been.calledThrice; // only 1 L1->L2 message sent. Note that the two - // whitelist transactions already sent two messages. + expect(l1Inbox.createRetryableTicket).to.have.been.calledOnce; // only 1 L1->L2 message sent. expect(l1Inbox.createRetryableTicket).to.have.been.calledWith( mockSpoke.address, 0, diff --git a/test/gas-analytics/HubPool.RootExecution.ts b/test/gas-analytics/HubPool.RootExecution.ts index 234fb29cd..b41a5abff 100644 --- a/test/gas-analytics/HubPool.RootExecution.ts +++ b/test/gas-analytics/HubPool.RootExecution.ts @@ -203,7 +203,10 @@ describe("Gas Analytics: HubPool Root Bundle Execution", function () { // Advance time so the request can be executed and execute the request. await timer.setCurrentTime(Number(await timer.getCurrentTime()) + consts.refundProposalLiveness + 1); const multicallData = leaves.map((leaf) => { - return hubPool.interface.encodeFunctionData("executeRootBundle", [leaf, tree.getHexProof(leaf)]); + return hubPool.interface.encodeFunctionData("executeRootBundle", [ + ...Object.values(leaf), + tree.getHexProof(leaf), + ]); }); const receipt = await (await hubPool.connect(dataWorker).multicall(multicallData)).wait(); From 98967d2ca283cab8d49e88c27fd744c40ad2834d Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Mon, 14 Mar 2022 17:59:56 -0400 Subject: [PATCH 02/20] Update HubPool.sol --- contracts/HubPool.sol | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index 413d526f6..b59bffaf7 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -80,7 +80,9 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { // chain ID is this network (i.e. origin chain ID = 1). This route is also used to keep track of which deposit // routes are enabled/disabled. struct Route { + // destinationToken is used for pool rebalances. address destinationToken; + // depositsEnabled enables deposit paths on SpokePools. bool depositsEnabled; } @@ -90,7 +92,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { // likely require its "reverse" route to be stored as well. For example, we would want to store USDC on Ethereum // to USDC on Optimism. This means that we'll map: keccak256(1, USDC-ethereum, 10) => USDC-optimism, and // keccak(10, USDC-optimism, 1) => USDC-ethereum. Note that not all routes are enabled, but we might still want - // to know what the destination token address is, for rebalances for example. + // to know what the destination token address is for rebalances. mapping(bytes32 => Route) private whitelistedRoutes; struct PooledToken { From c02b15ec730c9118c2d9bc83c76aac5f8e623c99 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Mon, 14 Mar 2022 18:10:28 -0400 Subject: [PATCH 03/20] Update HubPool.sol --- contracts/HubPool.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index b59bffaf7..cce4e399f 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -849,7 +849,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { uint256 originChainId, uint256 destinationChainId, address originToken - ) public view override returns (address destinationToken, bool depositsEnabled) { + ) external view override returns (address destinationToken, bool depositsEnabled) { Route memory route = whitelistedRoutes[_whitelistedRouteKey(originChainId, originToken, destinationChainId)]; destinationToken = route.destinationToken; depositsEnabled = route.depositsEnabled; From 3b4e0f6b9c450c4c0b8325139f455cf0aabebb76 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 16 Mar 2022 10:01:08 -0400 Subject: [PATCH 04/20] drop depositsEnabled from HubPool --- contracts/Arbitrum_SpokePool.sol | 28 +--- contracts/HubPool.sol | 144 +++++++++--------- contracts/HubPoolInterface.sol | 15 +- contracts/SpokePool.sol | 41 +++-- contracts/SpokePoolInterface.sol | 1 + test/HubPool.Admin.ts | 43 +++--- test/HubPool.ExecuteRootBundle.ts | 31 +++- test/SpokePool.Admin.ts | 11 +- test/SpokePool.Deposit.ts | 46 +++++- test/SpokePool.SlowRelay.ts | 5 +- test/chain-adapters/Arbitrum_Adapter.ts | 20 +-- test/chain-adapters/Ethereum_Adapter.ts | 4 +- test/chain-adapters/Optimism_Adapter.ts | 8 +- test/chain-adapters/Polygon_Adapter.ts | 8 +- .../Arbitrum_SpokePool.ts | 22 +-- .../Ethereum_SpokePool.ts | 12 +- .../Optimism_SpokePool.ts | 8 +- .../Polygon_SpokePool.ts | 26 +++- test/fixtures/HubPool.Fixture.ts | 10 +- test/fixtures/SpokePool.Fixture.ts | 3 +- test/gas-analytics/HubPool.RootExecution.ts | 2 +- test/gas-analytics/utils.ts | 3 +- 22 files changed, 285 insertions(+), 206 deletions(-) diff --git a/contracts/Arbitrum_SpokePool.sol b/contracts/Arbitrum_SpokePool.sol index 81a59e718..605fc3715 100644 --- a/contracts/Arbitrum_SpokePool.sol +++ b/contracts/Arbitrum_SpokePool.sol @@ -20,13 +20,8 @@ contract Arbitrum_SpokePool is SpokePool { // Address of the Arbitrum L2 token gateway to send funds to L1. address public l2GatewayRouter; - // Admin controlled mapping of arbitrum tokens to L1 counterpart. L1 counterpart addresses - // are necessary params used when bridging tokens to L1. - mapping(address => address) public whitelistedTokens; - event ArbitrumTokensBridged(address indexed l1Token, address target, uint256 numberOfTokensBridged); event SetL2GatewayRouter(address indexed newL2GatewayRouter); - event WhitelistedTokens(address indexed l2Token, address indexed l1Token); /** * @notice Construct the AVM SpokePool. @@ -63,22 +58,20 @@ contract Arbitrum_SpokePool is SpokePool { _setL2GatewayRouter(newL2GatewayRouter); } - /** - * @notice Add L2 -> L1 token mapping. Callable only by admin. - * @param l2Token Arbitrum token. - * @param l1Token Ethereum version of l2Token. - */ - function whitelistToken(address l2Token, address l1Token) public onlyAdmin { - _whitelistToken(l2Token, l1Token); - } - /************************************** * INTERNAL FUNCTIONS * **************************************/ function _bridgeTokensToHubPool(RelayerRefundLeaf memory relayerRefundLeaf) internal override { + // Check that the Ethereum counterpart of the L2 token is stored on this contract. We assume that the HubPool + // is deployed with a chain ID of 1. + require( + enabledDepositRoutes[relayerRefundLeaf.l2TokenAddress][1].destinationToken != address(0), + "Uninitialized mainnet token" + ); StandardBridgeLike(l2GatewayRouter).outboundTransfer( - whitelistedTokens[relayerRefundLeaf.l2TokenAddress], // _l1Token. Address of the L1 token to bridge over. + enabledDepositRoutes[relayerRefundLeaf.l2TokenAddress][1].destinationToken, // _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. @@ -91,11 +84,6 @@ contract Arbitrum_SpokePool is SpokePool { emit SetL2GatewayRouter(l2GatewayRouter); } - function _whitelistToken(address _l2Token, address _l1Token) internal { - whitelistedTokens[_l2Token] = _l1Token; - emit WhitelistedTokens(_l2Token, _l1Token); - } - // L1 addresses are transformed during l1->l2 calls. // See https://developer.offchainlabs.com/docs/l1_l2_messages#address-aliasing for more information. // This cannot be pulled directly from Arbitrum contracts because their contracts are not 0.8.X compatible and diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index cce4e399f..079ad65b9 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -74,26 +74,12 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { // Whether the bundle proposal process is paused. bool public paused; - // A Route is mapped to a hash of an origin chain ID, origin token address, and destination chain ID. Those three - // values should point to a unique token address existing on the destination chain. This route is used to determine - // which token to send from this contract to a spoke pool to execute a "pool rebalance" in the case where the origin - // chain ID is this network (i.e. origin chain ID = 1). This route is also used to keep track of which deposit - // routes are enabled/disabled. - struct Route { - // destinationToken is used for pool rebalances. - address destinationToken; - // depositsEnabled enables deposit paths on SpokePools. - bool depositsEnabled; - } - // Stores paths from origin token + ID to destination token + ID. Since different tokens on an origin might map to // to the same address on a destination, we hash (origin chain ID, origin token address, destination ID) to - // use as a key that maps to a destination token. This means that routes are stored "one-way" and each route will - // likely require its "reverse" route to be stored as well. For example, we would want to store USDC on Ethereum - // to USDC on Optimism. This means that we'll map: keccak256(1, USDC-ethereum, 10) => USDC-optimism, and - // keccak(10, USDC-optimism, 1) => USDC-ethereum. Note that not all routes are enabled, but we might still want - // to know what the destination token address is for rebalances. - mapping(bytes32 => Route) private whitelistedRoutes; + // use as a key that maps to a destination token. This mapping is used to enable pool rebalances from + // HubPool to SpokePool. Note that the value component of the mapping, or the "destination token" can be + // set to 0x0 to disable a pool rebalance route. + mapping(bytes32 => address) private poolRebalanceRoutes; struct PooledToken { // LP token given to LPs of a specific L1 token. @@ -193,17 +179,17 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { uint256 lpTokensBurnt, address indexed liquidityProvider ); - event WhitelistRoute( + event SetPoolRebalanceRoute( uint256 indexed originChainId, uint256 indexed destinationChainId, address indexed originToken, - address destinationToken, - bool depositsEnabled + address destinationToken ); - event RelayWhitelistRoute( + event SetEnableDepositRoute( uint256 indexed originChainId, uint256 indexed destinationChainId, address indexed originToken, + address destinationToken, bool depositsEnabled ); event ProposeRootBundle( @@ -393,15 +379,30 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { /** * @notice Stores mapping from origin chain ID + token + destination chain ID to token. This mapping corresponds - * to token transfer routes and can be used for both HubPool => SpokePool rebalances and SpokePool ==> SpokePool - * deposits. Callable only by owner. + * to token transfer routes and can be used for HubPool => SpokePool rebalances. Callable only by owner. + * @notice Can also be used to send cross-chain message to SpokePool on originChainId to enable or disable + * mapping from origin token to destination chain ID. If this path is enabled on the SpokePool, then relays + * are allowed to be sent via that path. + * @notice The meaning of a "route" is reused in this contract and is possible confusing. What's important to note + * is that token transfers always go from origin chain to destination chain. The HubPool stores a whitelist + * of routes where tokens can be sent from it to SpokePools for "pool rebalances", but it has no control over + * which deposits are enabled on SpokePools. Conversely, the SpokePool whitelists routes that deposited tokens + * can be sent along to execute cross-chain relays. The reason that this function is overloaded is because this + * contract is the only one that can update the SpokePool's whitelist of allowed deposit routes. It is likely + * in practice that the admin would want to simultaneously whitelist pool rebalance routes from the HubPool + * to the Optimism and Arbitrum SpokePools, and whitelist deposits from Optimism to Arbitrum. * @param originChainId Chain where token transfer originates. * @param destinationChainId Chain where token transfer ends up. * @param originToken Token sent from origin chain. * @param destinationToken Token received at destination chain. * @param depositsEnabled Set to true to whitelist this route for deposits, set to false if caller just wants to * map the origin token + origin ID + destination ID to the destination token address and wants to explicitly - * disable this route for deposits. + * disable this route for deposits. This variable is used only if `relayToSpokePool` is True, in which case it is + * relayed as part of the message to the SpokePool. + * @param relayToSpokePool Set to true to relay a message to the SpokePool on the origin chain to enable + * deposits for the origin token to the destination chain ID. Set to false to skip this step + * @param setInHubPool Set to true to store the mapping from origin chain ID + token + destination chain ID to + * the destination token and whether deposits are enabled or not. Set to false to skip this step. */ function whitelistRoute( @@ -409,44 +410,41 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { uint256 destinationChainId, address originToken, address destinationToken, - bool depositsEnabled + bool depositsEnabled, + bool relayToSpokePool, + bool setInHubPool ) public override onlyOwner { - whitelistedRoutes[_whitelistedRouteKey(originChainId, originToken, destinationChainId)] = Route({ - destinationToken: destinationToken, - depositsEnabled: depositsEnabled - }); - emit WhitelistRoute(originChainId, destinationChainId, originToken, destinationToken, depositsEnabled); - } - - /** - * @notice Send cross-chain message to SpokePool on originChainId to enable or disable mapping from origin - * origin token to destination chain ID. If this path is enabled on the SpokePool, then relays are allowed to be - * sent via that path. The destination token address mapped to the origin token can be set by `whitelistRoute` and - * fetched by off-chain relayers. The SpokePool doesn't need to know which destination tokens are mapped to origin - * tokens as this protocol assumes that relayers use the correct whitelisted route. Callable only by owner, - * who has the responsibility to make sure that calls to `whitelistRoute` are made in concert with these calls, - * otherwise the enabled path information will become out of sync between SpokePool and HubPool. - * @param originChainId Chain where token transfer originates. - * @param destinationChainId Chain where token transfer ends up. - * @param originToken Token sent from origin chain. - * @param depositsEnabled Set to True/False to enable/disable route on SpokePool for deposits. - */ - function relayWhitelistRoute( - uint256 originChainId, - uint256 destinationChainId, - address originToken, - bool depositsEnabled - ) public override onlyOwner nonReentrant { - _relaySpokePoolAdminFunction( - originChainId, - abi.encodeWithSignature( - "setEnableRoute(address,uint256,bool)", - originToken, + if (setInHubPool) { + // Store which destination token we need to bridge to SpokePool on origin chain. + // @dev There is an argument we should require that `originChainId == block.chainId` since pool rebalances + // will only ever originate from this contract, but this is an onlyOwner function and we leave it to the + // admin to use this function correctly. + poolRebalanceRoutes[ + _poolRebalanceRouteKey(originChainId, originToken, destinationChainId) + ] = destinationToken; + emit SetPoolRebalanceRoute(originChainId, destinationChainId, originToken, destinationToken); + } + if (relayToSpokePool) { + // Store whether deposit route from origin chain to destination chain should be enabled or disabled on + // spoke pool on origin chain. + _relaySpokePoolAdminFunction( + originChainId, + abi.encodeWithSignature( + "setEnableRoute(address,address,uint256,bool)", + originToken, + destinationToken, + destinationChainId, + depositsEnabled + ) + ); + emit SetEnableDepositRoute( + originChainId, destinationChainId, + originToken, + destinationToken, depositsEnabled - ) - ); - emit RelayWhitelistRoute(originChainId, destinationChainId, originToken, depositsEnabled); + ); + } } /** @@ -837,22 +835,20 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { } /** - * @notice Conveniently queries whether an origin chain + token => destination chain ID is enabled for deposits and - * the mapped destination token. - * @param originChainId Deposit chain. - * @param destinationChainId Where depositor can receive funds. - * @param originToken Deposited token. - * @return destinationToken address Depositor can receive this token on destination chain. - * @return depositsEnabled boolean Whether deposits are enabled on SpokePool on origin chain. + * @notice Conveniently queries which destination token is mapped to the hash of an origin chain + token + + * destination chain ID for use in pool rebalances. + * @param originChainId Origin chain. + * @param destinationChainId Where pool rebalance sends funds. + * @param originToken Rebalance token. + * @return destinationToken address SpokePool can receive this token on destination chain following pool + * rebalance. */ - function whitelistedRoute( + function poolRebalanceRoute( uint256 originChainId, uint256 destinationChainId, address originToken - ) external view override returns (address destinationToken, bool depositsEnabled) { - Route memory route = whitelistedRoutes[_whitelistedRouteKey(originChainId, originToken, destinationChainId)]; - destinationToken = route.destinationToken; - depositsEnabled = route.depositsEnabled; + ) external view override returns (address destinationToken) { + return poolRebalanceRoutes[_poolRebalanceRouteKey(originChainId, originToken, destinationChainId)]; } /** @@ -910,7 +906,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { address l1Token = l1Tokens[i]; // 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)].destinationToken; + address l2Token = poolRebalanceRoutes[_poolRebalanceRouteKey(block.chainid, 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 @@ -1064,7 +1060,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { emit SpokePoolAdminFunctionTriggered(chainId, functionData); } - function _whitelistedRouteKey( + function _poolRebalanceRouteKey( uint256 originChainId, address originToken, uint256 destinationChainId diff --git a/contracts/HubPoolInterface.sol b/contracts/HubPoolInterface.sol index a59cdb423..e77861cb0 100644 --- a/contracts/HubPoolInterface.sol +++ b/contracts/HubPoolInterface.sol @@ -103,21 +103,16 @@ interface HubPoolInterface { uint256 destinationChainId, address originToken, address destinationToken, - bool depositsEnabled + bool depositsEnabled, + bool relayToSpokePool, + bool setInHubPool ) external; - function relayWhitelistRoute( - uint256 originChainId, - uint256 destinationChainId, - address originToken, - bool depositsEnabled - ) external; - - function whitelistedRoute( + function poolRebalanceRoute( uint256 originChainId, uint256 destinationChainId, address originToken - ) external view returns (address destinationToken, bool depositsEnabled); + ) external view returns (address destinationToken); function loadEthForL2Calls() external payable; } diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index e31c73412..f66a0b705 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -51,10 +51,12 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall // Count of deposits is used to construct a unique deposit identifier for this spoke pool. uint32 public numberOfDeposits; + struct DepositDestinationToken { + address destinationToken; + bool enabled; + } // 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; + mapping(address => mapping(uint256 => DepositDestinationToken)) public enabledDepositRoutes; // Stores collection of merkle roots that can be published to this contract from the HubPool, which are referenced // by "data workers" via inclusion proofs to execute leaves in the roots. @@ -81,7 +83,12 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall ****************************************/ event SetXDomainAdmin(address indexed newAdmin); event SetHubPool(address indexed newHubPool); - event EnabledDepositRoute(address indexed originToken, uint256 indexed destinationChainId, bool enabled); + event EnabledDepositRoute( + uint256 indexed destinationChainId, + address indexed originToken, + address indexed destinationToken, + bool enabled + ); event SetDepositQuoteTimeBuffer(uint32 newBuffer); event FundsDeposited( uint256 amount, @@ -91,6 +98,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall uint32 indexed depositId, uint32 quoteTimestamp, address indexed originToken, + address destinationToken, address recipient, address indexed depositor ); @@ -160,11 +168,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() { @@ -195,16 +198,21 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall /** * @notice Enable/Disable an origin token => destination chain ID route for deposits. Callable by admin only. * @param originToken Token that depositor can deposit to this contract. + * @param destinationToken Token that recipient on destination chain receives. * @param destinationChainId Chain ID for where depositor wants to receive funds. * @param enabled True to enable deposits, False otherwise. */ function setEnableRoute( address originToken, + address destinationToken, uint256 destinationChainId, bool enabled ) public override onlyAdmin { - enabledDepositRoutes[originToken][destinationChainId] = enabled; - emit EnabledDepositRoute(originToken, destinationChainId, enabled); + enabledDepositRoutes[originToken][destinationChainId] = DepositDestinationToken({ + destinationToken: destinationToken, + enabled: enabled + }); + emit EnabledDepositRoute(destinationChainId, originToken, destinationToken, enabled); } /** @@ -253,7 +261,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall * tokens in this contract and receive a destination token on the destination chain. The origin => destination * token mapping is stored on the L1 HubPool. * @notice The caller must first approve this contract to spend amount of originToken. - * @notice The originToken => destinationChainId must be enabled. + * @notice The originToken => destinationChainId must be enabled and the destination token must be set. * @notice This method is payable because the caller is able to deposit ETH if the originToken is WETH and this * function will handle wrapping ETH. * @param recipient Address to receive funds at on destination chain. @@ -271,7 +279,11 @@ 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 initialized. + DepositDestinationToken memory destinationToken = enabledDepositRoutes[originToken][destinationChainId]; + require(destinationToken.destinationToken != address(0) && destinationToken.enabled, "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, @@ -302,6 +314,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall numberOfDeposits, quoteTimestamp, originToken, + destinationToken.destinationToken, recipient, msg.sender ); @@ -824,6 +837,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall uint32 depositId, uint32 quoteTimestamp, address originToken, + address destinationToken, address recipient, address depositor ) internal { @@ -835,6 +849,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall depositId, quoteTimestamp, originToken, + destinationToken, recipient, depositor ); diff --git a/contracts/SpokePoolInterface.sol b/contracts/SpokePoolInterface.sol index 5f49b0ca8..eeb22fec3 100644 --- a/contracts/SpokePoolInterface.sol +++ b/contracts/SpokePoolInterface.sol @@ -54,6 +54,7 @@ interface SpokePoolInterface { function setEnableRoute( address originToken, + address destinationToken, uint256 destinationChainId, bool enable ) external; diff --git a/test/HubPool.Admin.ts b/test/HubPool.Admin.ts index 8175075b6..4aa0482d5 100644 --- a/test/HubPool.Admin.ts +++ b/test/HubPool.Admin.ts @@ -3,6 +3,7 @@ import { Contract, ethers, randomAddress, utf8ToHex } from "./utils"; import { originChainId, destinationChainId, bondAmount, zeroAddress, mockTreeRoot } from "./constants"; import { mockSlowRelayRoot, finalFeeUsdc, finalFee, totalBond } from "./constants"; import { hubPoolFixture } from "./fixtures/HubPool.Fixture"; +import { ZERO_ADDRESS } from "@uma/common"; let hubPool: Contract, weth: Contract, @@ -49,28 +50,30 @@ 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) + hubPool + .connect(other) + .whitelistRoute(originChainId, destinationChainId, weth.address, usdc.address, true, false, 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); - let whitelistedRoute = await hubPool.whitelistedRoute(originChainId, destinationChainId, weth.address); - expect(whitelistedRoute.destinationToken).to.equal(usdc.address); - expect(whitelistedRoute.depositsEnabled).to.equal(true); - - // Can disable a route. - await hubPool.whitelistRoute(originChainId, destinationChainId, weth.address, usdc.address, false); - whitelistedRoute = await hubPool.whitelistedRoute(originChainId, destinationChainId, weth.address); - expect(whitelistedRoute.destinationToken).to.equal(usdc.address); - expect(whitelistedRoute.depositsEnabled).to.equal(false); + await expect( + hubPool.whitelistRoute(originChainId, destinationChainId, weth.address, usdc.address, true, false, true) + ) + .to.emit(hubPool, "SetPoolRebalanceRoute") + .withArgs(originChainId, destinationChainId, weth.address, usdc.address); // Relay whitelist mapping to spoke pool. Check content of messages sent to mock spoke pool. - await expect(hubPool.connect(other).relayWhitelistRoute(originChainId, destinationChainId, weth.address, true)).to - .be.reverted; - await expect(hubPool.relayWhitelistRoute(originChainId, destinationChainId, weth.address, true)) - .to.emit(hubPool, "RelayWhitelistRoute") - .withArgs(originChainId, destinationChainId, weth.address, true); - await hubPool.relayWhitelistRoute(originChainId, destinationChainId, weth.address, false); + await expect( + hubPool + .connect(other) + .whitelistRoute(originChainId, destinationChainId, weth.address, usdc.address, true, true, false) + ).to.be.reverted; + await expect( + hubPool.whitelistRoute(originChainId, destinationChainId, weth.address, usdc.address, true, true, false) + ) + .to.emit(hubPool, "SetEnableDepositRoute") + .withArgs(originChainId, destinationChainId, weth.address, usdc.address, true); + + // Disable deposit route on SpokePool right after: + await hubPool.whitelistRoute(originChainId, destinationChainId, weth.address, usdc.address, false, true, true); // Since the mock adapter is delegatecalled, when querying, its address should be the hubPool address. const mockAdapterAtHubPool = mockAdapter.attach(hubPool.address); @@ -80,6 +83,7 @@ describe("HubPool Admin functions", function () { expect(relayMessageEvents[relayMessageEvents.length - 1].args?.message).to.equal( mockSpoke.interface.encodeFunctionData("setEnableRoute", [ weth.address, + usdc.address, destinationChainId, false, // Latest call disabled the route ]) @@ -87,6 +91,7 @@ describe("HubPool Admin functions", function () { expect(relayMessageEvents[relayMessageEvents.length - 2].args?.message).to.equal( mockSpoke.interface.encodeFunctionData("setEnableRoute", [ weth.address, + usdc.address, destinationChainId, true, // Second to last call enabled the route ]) diff --git a/test/HubPool.ExecuteRootBundle.ts b/test/HubPool.ExecuteRootBundle.ts index 89204e804..ad04d249a 100644 --- a/test/HubPool.ExecuteRootBundle.ts +++ b/test/HubPool.ExecuteRootBundle.ts @@ -1,4 +1,4 @@ -import { toBNWei, SignerWithAddress, seedWallet, expect, Contract, ethers } from "./utils"; +import { toBNWei, SignerWithAddress, seedWallet, expect, Contract, ethers, hre } from "./utils"; import * as consts from "./constants"; import { hubPoolFixture, enableTokensForLP } from "./fixtures/HubPool.Fixture"; import { buildPoolRebalanceLeafTree, buildPoolRebalanceLeafs } from "./MerkleLib.utils"; @@ -114,6 +114,35 @@ describe("HubPool Root Bundle Execution", function () { ).to.be.revertedWith("Uninitialized spoke pool"); }); + it("Reverts if destination token is zero address for a pool rebalance route", async function () { + const { leafs, tree } = await constructSimpleTree(); + + await hubPool.connect(dataWorker).proposeRootBundle( + [3117], // bundleEvaluationBlockNumbers used by bots to construct bundles. Length must equal the number of leafs. + 1, // poolRebalanceLeafCount. There is exactly one leaf in the bundle (just sending WETH to one address). + tree.getHexRoot(), // poolRebalanceRoot. Generated from the merkle tree constructed before. + consts.mockRelayerRefundRoot, // Not relevant for this test. + consts.mockSlowRelayRoot // Not relevant for this test. + ); + + // Let's set weth pool rebalance route to zero address. + await hubPool.whitelistRoute( + await hre.getChainId(), + consts.repaymentChainId, + weth.address, + ZERO_ADDRESS, + true, + false, + true + ); + + // Advance time so the request can be executed and check that executing the request reverts. + await timer.setCurrentTime(Number(await timer.getCurrentTime()) + consts.refundProposalLiveness + 1); + await expect( + hubPool.connect(dataWorker).executeRootBundle(...Object.values(leafs[0]), tree.getHexProof(leafs[0])) + ).to.be.revertedWith("Route not whitelisted"); + }); + it("Execution rejects leaf claim before liveness passed", async function () { const { leafs, tree } = await constructSimpleTree(); await hubPool diff --git a/test/SpokePool.Admin.ts b/test/SpokePool.Admin.ts index f9e2126ad..eda3adf1f 100644 --- a/test/SpokePool.Admin.ts +++ b/test/SpokePool.Admin.ts @@ -1,4 +1,4 @@ -import { expect, ethers, Contract, SignerWithAddress } from "./utils"; +import { expect, ethers, Contract, SignerWithAddress, randomAddress } from "./utils"; import { spokePoolFixture } from "./fixtures/SpokePool.Fixture"; import { destinationChainId, mockRelayerRefundRoot, mockSlowRelayRoot } from "./constants"; @@ -11,10 +11,13 @@ describe("SpokePool Admin Functions", async function () { ({ spokePool, erc20 } = await spokePoolFixture()); }); it("Enable token path", async function () { - await expect(spokePool.connect(owner).setEnableRoute(erc20.address, destinationChainId, true)) + const destToken = randomAddress(); + await expect(spokePool.connect(owner).setEnableRoute(erc20.address, destToken, destinationChainId, true)) .to.emit(spokePool, "EnabledDepositRoute") - .withArgs(erc20.address, destinationChainId, true); - expect(await spokePool.enabledDepositRoutes(erc20.address, destinationChainId)).to.equal(true); + .withArgs(destinationChainId, erc20.address, destToken, true); + const destTokenStruct = await spokePool.enabledDepositRoutes(erc20.address, destinationChainId); + expect(destTokenStruct.enabled).to.equal(true); + expect(destTokenStruct.destinationToken).to.equal(destToken); }); it("Change deposit quote buffer", async function () { await expect(spokePool.connect(owner).setDepositQuoteTimeBuffer(60)) diff --git a/test/SpokePool.Deposit.ts b/test/SpokePool.Deposit.ts index b231f9c4a..7c73dfb85 100644 --- a/test/SpokePool.Deposit.ts +++ b/test/SpokePool.Deposit.ts @@ -1,6 +1,7 @@ -import { expect, ethers, Contract, SignerWithAddress, seedWallet, toBN, toWei } from "./utils"; +import { expect, ethers, Contract, SignerWithAddress, seedWallet, toBN, toWei, randomAddress } from "./utils"; import { spokePoolFixture, enableRoutes, getDepositParams } from "./fixtures/SpokePool.Fixture"; import { amountToSeedWallets, amountToDeposit, destinationChainId, depositRelayerFeePct } from "./constants"; +import { ZERO_ADDRESS } from "@uma/common"; let spokePool: Contract, weth: Contract, erc20: Contract, unwhitelistedErc20: Contract; let depositor: SignerWithAddress, recipient: SignerWithAddress; @@ -18,7 +19,10 @@ describe("SpokePool Depositor Logic", async function () { await weth.connect(depositor).approve(spokePool.address, amountToDeposit); // Whitelist origin token => destination chain ID routes: - await enableRoutes(spokePool, [{ originToken: erc20.address }, { originToken: weth.address }]); + await enableRoutes(spokePool, [ + { originToken: erc20.address, destinationToken: weth.address }, + { originToken: weth.address, destinationToken: erc20.address }, + ]); }); it("Depositing ERC20 tokens correctly pulls tokens and changes contract state", async function () { const currentSpokePoolTime = await spokePool.getCurrentTime(); @@ -45,6 +49,7 @@ describe("SpokePool Depositor Logic", async function () { 0, currentSpokePoolTime, erc20.address, + weth.address, recipient.address, depositor.address ); @@ -152,7 +157,7 @@ describe("SpokePool Depositor Logic", async function () { ).to.be.reverted; // Cannot deposit disabled route. - await spokePool.connect(depositor).setEnableRoute(erc20.address, destinationChainId, false); + await spokePool.connect(depositor).setEnableRoute(erc20.address, weth.address, destinationChainId, false); await expect( spokePool .connect(depositor) @@ -167,8 +172,39 @@ describe("SpokePool Depositor Logic", async function () { ) ) ).to.be.reverted; - // Re-enable route. - await spokePool.connect(depositor).setEnableRoute(erc20.address, destinationChainId, true); + // Re-enable route but "forget" to set destination token address + await spokePool.connect(depositor).setEnableRoute(erc20.address, ZERO_ADDRESS, destinationChainId, true); + await expect( + spokePool + .connect(depositor) + .deposit( + ...getDepositParams( + recipient.address, + erc20.address, + amountToDeposit, + destinationChainId, + depositRelayerFeePct, + currentSpokePoolTime + ) + ) + ).to.be.reverted; + + // Re-enable route and demonstrate that call would work. + await spokePool.connect(depositor).setEnableRoute(erc20.address, weth.address, destinationChainId, true); + await expect( + spokePool + .connect(depositor) + .callStatic.deposit( + ...getDepositParams( + recipient.address, + erc20.address, + amountToDeposit, + destinationChainId, + depositRelayerFeePct, + currentSpokePoolTime + ) + ) + ).to.be.ok; // Cannot deposit with invalid relayer fee. await expect( diff --git a/test/SpokePool.SlowRelay.ts b/test/SpokePool.SlowRelay.ts index c749059e2..10d6b4840 100644 --- a/test/SpokePool.SlowRelay.ts +++ b/test/SpokePool.SlowRelay.ts @@ -38,7 +38,10 @@ describe("SpokePool Slow Relay Logic", async function () { await weth.connect(relayer).approve(spokePool.address, fullRelayAmountPostFees); // Whitelist origin token => destination chain ID routes: - await enableRoutes(spokePool, [{ originToken: erc20.address }, { originToken: weth.address }]); + await enableRoutes(spokePool, [ + { originToken: erc20.address, destinationToken: randomAddress() }, + { originToken: weth.address, destinationToken: randomAddress() }, + ]); relays = []; for (let i = 0; i < 99; i++) { diff --git a/test/chain-adapters/Arbitrum_Adapter.ts b/test/chain-adapters/Arbitrum_Adapter.ts index 6019bad59..6f3dd55ca 100644 --- a/test/chain-adapters/Arbitrum_Adapter.ts +++ b/test/chain-adapters/Arbitrum_Adapter.ts @@ -4,13 +4,7 @@ import { getContractFactory, seedWallet, randomAddress } from "../utils"; import { hubPoolFixture, enableTokensForLP } from "../fixtures/HubPool.Fixture"; import { constructSingleChainTree } from "../MerkleLib.utils"; -let hubPool: Contract, - arbitrumAdapter: Contract, - mockAdapter: Contract, - weth: Contract, - dai: Contract, - timer: Contract, - mockSpoke: Contract; +let hubPool: Contract, arbitrumAdapter: Contract, weth: Contract, dai: Contract, timer: Contract, mockSpoke: Contract; let l2Weth: string, l2Dai: string; let owner: SignerWithAddress, dataWorker: SignerWithAddress, liquidityProvider: SignerWithAddress; let l1ERC20Gateway: FakeContract, l1Inbox: FakeContract; @@ -21,7 +15,7 @@ let l1ChainId: number; describe("Arbitrum Chain Adapter", function () { beforeEach(async function () { [owner, dataWorker, liquidityProvider] = await ethers.getSigners(); - ({ weth, dai, l2Weth, l2Dai, hubPool, mockSpoke, timer, mockAdapter } = await hubPoolFixture()); + ({ weth, dai, l2Weth, l2Dai, hubPool, mockSpoke, timer } = await hubPoolFixture()); await seedWallet(dataWorker, [dai], weth, consts.amountToLp); await seedWallet(liquidityProvider, [dai], weth, consts.amountToLp.mul(10)); @@ -46,14 +40,8 @@ describe("Arbitrum Chain Adapter", function () { await hubPool.setCrossChainContracts(arbitrumChainId, arbitrumAdapter.address, mockSpoke.address); - await hubPool.whitelistRoute(arbitrumChainId, l1ChainId, l2Weth, weth.address, true); - - await hubPool.whitelistRoute(arbitrumChainId, l1ChainId, l2Dai, dai.address, true); - - await hubPool.setCrossChainContracts(l1ChainId, mockAdapter.address, mockSpoke.address); - - await hubPool.whitelistRoute(l1ChainId, arbitrumChainId, dai.address, l2Dai, true); - await hubPool.whitelistRoute(l1ChainId, arbitrumChainId, weth.address, l2Weth, true); + await hubPool.whitelistRoute(l1ChainId, arbitrumChainId, dai.address, l2Dai, true, false, true); + await hubPool.whitelistRoute(l1ChainId, arbitrumChainId, weth.address, l2Weth, true, false, true); }); it("relayMessage calls spoke pool functions", async function () { diff --git a/test/chain-adapters/Ethereum_Adapter.ts b/test/chain-adapters/Ethereum_Adapter.ts index cc4e42d4e..152e8be33 100644 --- a/test/chain-adapters/Ethereum_Adapter.ts +++ b/test/chain-adapters/Ethereum_Adapter.ts @@ -32,9 +32,9 @@ describe("Ethereum Chain Adapter", function () { await hubPool.setCrossChainContracts(l1ChainId, ethAdapter.address, mockSpoke.address); - await hubPool.whitelistRoute(l1ChainId, l1ChainId, weth.address, weth.address, true); + await hubPool.whitelistRoute(l1ChainId, l1ChainId, weth.address, weth.address, true, false, true); - await hubPool.whitelistRoute(l1ChainId, l1ChainId, dai.address, dai.address, true); + await hubPool.whitelistRoute(l1ChainId, l1ChainId, dai.address, dai.address, true, false, true); }); it("relayMessage calls spoke pool functions", async function () { diff --git a/test/chain-adapters/Optimism_Adapter.ts b/test/chain-adapters/Optimism_Adapter.ts index 4d14c0423..112b9f6df 100644 --- a/test/chain-adapters/Optimism_Adapter.ts +++ b/test/chain-adapters/Optimism_Adapter.ts @@ -42,12 +42,8 @@ describe("Optimism Chain Adapter", function () { ).deploy(weth.address, l1CrossDomainMessenger.address, l1StandardBridge.address); await hubPool.setCrossChainContracts(optimismChainId, optimismAdapter.address, mockSpoke.address); - await hubPool.whitelistRoute(optimismChainId, l1ChainId, l2Weth, weth.address, true); - await hubPool.whitelistRoute(optimismChainId, l1ChainId, l2Dai, dai.address, true); - - await hubPool.setCrossChainContracts(l1ChainId, mockAdapter.address, mockSpoke.address); - await hubPool.whitelistRoute(l1ChainId, optimismChainId, weth.address, l2Weth, true); - await hubPool.whitelistRoute(l1ChainId, optimismChainId, dai.address, l2Dai, true); + await hubPool.whitelistRoute(l1ChainId, optimismChainId, weth.address, l2Weth, true, false, true); + await hubPool.whitelistRoute(l1ChainId, optimismChainId, dai.address, l2Dai, true, false, true); }); it("relayMessage calls spoke pool functions", async function () { diff --git a/test/chain-adapters/Polygon_Adapter.ts b/test/chain-adapters/Polygon_Adapter.ts index b6dd4111e..f199e7372 100644 --- a/test/chain-adapters/Polygon_Adapter.ts +++ b/test/chain-adapters/Polygon_Adapter.ts @@ -42,12 +42,8 @@ describe("Polygon Chain Adapter", function () { ).deploy(rootChainManager.address, fxStateSender.address, weth.address); await hubPool.setCrossChainContracts(polygonChainId, polygonAdapter.address, mockSpoke.address); - await hubPool.whitelistRoute(polygonChainId, l1ChainId, l2Weth, weth.address, true); - await hubPool.whitelistRoute(polygonChainId, l1ChainId, l2Dai, dai.address, true); - - await hubPool.setCrossChainContracts(l1ChainId, mockAdapter.address, mockSpoke.address); - await hubPool.whitelistRoute(l1ChainId, polygonChainId, weth.address, l2Weth, true); - await hubPool.whitelistRoute(l1ChainId, polygonChainId, dai.address, l2Dai, true); + await hubPool.whitelistRoute(l1ChainId, polygonChainId, weth.address, l2Weth, true, false, true); + await hubPool.whitelistRoute(l1ChainId, polygonChainId, dai.address, l2Dai, true, false, true); }); it("relayMessage calls spoke pool functions", async function () { diff --git a/test/chain-specific-spokepools/Arbitrum_SpokePool.ts b/test/chain-specific-spokepools/Arbitrum_SpokePool.ts index a2e8dbe32..2bb42bd26 100644 --- a/test/chain-specific-spokepools/Arbitrum_SpokePool.ts +++ b/test/chain-specific-spokepools/Arbitrum_SpokePool.ts @@ -28,7 +28,6 @@ describe("Arbitrum Spoke Pool", function () { ).deploy(l2GatewayRouter.address, owner.address, hubPool.address, l2Weth, timer.address); await seedContract(arbitrumSpokePool, relayer, [dai], weth, amountHeldByPool); - await arbitrumSpokePool.connect(crossDomainAlias).whitelistToken(l2Dai, dai.address); }); it("Only cross domain owner can set L2GatewayRouter", async function () { @@ -38,15 +37,11 @@ describe("Arbitrum Spoke Pool", function () { }); it("Only cross domain owner can enable a route", async function () { - await expect(arbitrumSpokePool.setEnableRoute(l2Dai, 1, true)).to.be.reverted; - await arbitrumSpokePool.connect(crossDomainAlias).setEnableRoute(l2Dai, 1, true); - expect(await arbitrumSpokePool.enabledDepositRoutes(l2Dai, 1)).to.equal(true); - }); - - it("Only cross domain owner can whitelist a token pair", async function () { - await expect(arbitrumSpokePool.whitelistToken(l2Dai, dai.address)).to.be.reverted; - await arbitrumSpokePool.connect(crossDomainAlias).whitelistToken(l2Dai, dai.address); - expect(await arbitrumSpokePool.whitelistedTokens(l2Dai)).to.equal(dai.address); + await expect(arbitrumSpokePool.setEnableRoute(l2Dai, dai.address, 1, true)).to.be.reverted; + await arbitrumSpokePool.connect(crossDomainAlias).setEnableRoute(l2Dai, dai.address, 1, true); + const destinationTokenStruct = await arbitrumSpokePool.enabledDepositRoutes(l2Dai, 1); + expect(destinationTokenStruct.enabled).to.equal(true); + expect(destinationTokenStruct.destinationToken).to.equal(dai.address); }); it("Only cross domain owner can set the cross domain admin", async function () { @@ -85,6 +80,13 @@ describe("Arbitrum Spoke Pool", function () { it("Bridge tokens to hub pool correctly calls the Standard L2 Gateway router", async function () { const { leafs, tree } = await constructSingleRelayerRefundTree(l2Dai, await arbitrumSpokePool.callStatic.chainId()); await arbitrumSpokePool.connect(crossDomainAlias).relayRootBundle(tree.getHexRoot(), mockTreeRoot); + + // Reverts if route from arbitrum to mainnet for l2Dai isn't whitelisted. + await expect( + arbitrumSpokePool.executeRelayerRefundRoot(0, leafs[0], tree.getHexProof(leafs[0])) + ).to.be.revertedWith("Uninitialized mainnet token"); + + await arbitrumSpokePool.connect(crossDomainAlias).setEnableRoute(l2Dai, dai.address, 1, true); await arbitrumSpokePool.connect(relayer).executeRelayerRefundRoot(0, leafs[0], tree.getHexProof(leafs[0])); // This should have sent tokens back to L1. Check the correct methods on the gateway are correctly called. diff --git a/test/chain-specific-spokepools/Ethereum_SpokePool.ts b/test/chain-specific-spokepools/Ethereum_SpokePool.ts index 7693c27be..22c75c272 100644 --- a/test/chain-specific-spokepools/Ethereum_SpokePool.ts +++ b/test/chain-specific-spokepools/Ethereum_SpokePool.ts @@ -4,14 +4,14 @@ import { getContractFactory, seedContract } from "../utils"; import { hubPoolFixture } from "../fixtures/HubPool.Fixture"; import { constructSingleRelayerRefundTree } from "../MerkleLib.utils"; -let hubPool: Contract, spokePool: Contract, timer: Contract, dai: Contract, weth: Contract; +let hubPool: Contract, spokePool: Contract, timer: Contract, dai: Contract, weth: Contract, l2Dai: string; let owner: SignerWithAddress, relayer: SignerWithAddress, rando: SignerWithAddress; describe("Ethereum Spoke Pool", function () { beforeEach(async function () { [owner, relayer, rando] = await ethers.getSigners(); - ({ weth, dai, hubPool, timer } = await hubPoolFixture()); + ({ weth, dai, hubPool, timer, l2Dai } = await hubPoolFixture()); spokePool = await ( await getContractFactory("Ethereum_SpokePool", owner) @@ -29,9 +29,11 @@ describe("Ethereum Spoke Pool", function () { }); it("Only owner can enable a route", async function () { - await expect(spokePool.connect(rando).setEnableRoute(dai.address, 1, true)).to.be.reverted; - await spokePool.connect(owner).setEnableRoute(dai.address, 1, true); - expect(await spokePool.enabledDepositRoutes(dai.address, 1)).to.equal(true); + await expect(spokePool.connect(rando).setEnableRoute(l2Dai, dai.address, 1, true)).to.be.reverted; + await spokePool.connect(owner).setEnableRoute(l2Dai, dai.address, 1, true); + const destinationTokenStruct = await spokePool.enabledDepositRoutes(l2Dai, 1); + expect(destinationTokenStruct.enabled).to.equal(true); + expect(destinationTokenStruct.destinationToken).to.equal(dai.address); }); it("Only owner can set the hub pool address", async function () { diff --git a/test/chain-specific-spokepools/Optimism_SpokePool.ts b/test/chain-specific-spokepools/Optimism_SpokePool.ts index 51d1f246e..064de86ed 100644 --- a/test/chain-specific-spokepools/Optimism_SpokePool.ts +++ b/test/chain-specific-spokepools/Optimism_SpokePool.ts @@ -49,10 +49,12 @@ describe("Optimism Spoke Pool", function () { }); it("Only cross domain owner can enable a route", async function () { - await expect(optimismSpokePool.setEnableRoute(l2Dai, 1, true)).to.be.reverted; + await expect(optimismSpokePool.setEnableRoute(l2Dai, dai.address, 1, true)).to.be.reverted; crossDomainMessenger.xDomainMessageSender.returns(owner.address); - await optimismSpokePool.connect(crossDomainMessenger.wallet).setEnableRoute(l2Dai, 1, true); - expect(await optimismSpokePool.enabledDepositRoutes(l2Dai, 1)).to.equal(true); + await optimismSpokePool.connect(crossDomainMessenger.wallet).setEnableRoute(l2Dai, dai.address, 1, true); + const destinationTokenStruct = await optimismSpokePool.enabledDepositRoutes(l2Dai, 1); + expect(destinationTokenStruct.enabled).to.equal(true); + expect(destinationTokenStruct.destinationToken).to.equal(dai.address); }); it("Only cross domain owner can set the cross domain admin", async function () { diff --git a/test/chain-specific-spokepools/Polygon_SpokePool.ts b/test/chain-specific-spokepools/Polygon_SpokePool.ts index b49eaaf39..a97e1abdb 100644 --- a/test/chain-specific-spokepools/Polygon_SpokePool.ts +++ b/test/chain-specific-spokepools/Polygon_SpokePool.ts @@ -4,14 +4,14 @@ import { ethers, expect, Contract, SignerWithAddress, getContractFactory, seedCo import { hubPoolFixture } from "../fixtures/HubPool.Fixture"; import { constructSingleRelayerRefundTree } from "../MerkleLib.utils"; -let hubPool: Contract, polygonSpokePool: Contract, timer: Contract, dai: Contract, weth: Contract; +let hubPool: Contract, polygonSpokePool: Contract, timer: Contract, dai: Contract, weth: Contract, l2Dai: string; let owner: SignerWithAddress, relayer: SignerWithAddress, rando: SignerWithAddress, fxChild: SignerWithAddress; describe("Polygon Spoke Pool", function () { beforeEach(async function () { [owner, relayer, fxChild, rando] = await ethers.getSigners(); - ({ weth, hubPool, timer } = await hubPoolFixture()); + ({ weth, hubPool, timer, l2Dai } = await hubPoolFixture()); const polygonTokenBridger = await ( await getContractFactory("PolygonTokenBridger", owner) @@ -59,6 +59,28 @@ describe("Polygon Spoke Pool", function () { expect(await polygonSpokePool.hubPool()).to.equal(rando.address); }); + it("Only correct caller can enable a route", async function () { + const setEnableRouteData = polygonSpokePool.interface.encodeFunctionData("setEnableRoute", [ + l2Dai, + dai.address, + 1, + true, + ]); + + // Wrong rootMessageSender address. + await expect(polygonSpokePool.connect(fxChild).processMessageFromRoot(0, rando.address, setEnableRouteData)).to.be + .reverted; + + // Wrong calling address. + await expect(polygonSpokePool.connect(rando).processMessageFromRoot(0, owner.address, setEnableRouteData)).to.be + .reverted; + + await polygonSpokePool.connect(fxChild).processMessageFromRoot(0, owner.address, setEnableRouteData); + const destinationTokenStruct = await polygonSpokePool.enabledDepositRoutes(l2Dai, 1); + expect(destinationTokenStruct.enabled).to.equal(true); + expect(destinationTokenStruct.destinationToken).to.equal(dai.address); + }); + it("Only correct caller can set the quote time buffer", async function () { const setDepositQuoteTimeBufferData = polygonSpokePool.interface.encodeFunctionData("setDepositQuoteTimeBuffer", [ 12345, diff --git a/test/fixtures/HubPool.Fixture.ts b/test/fixtures/HubPool.Fixture.ts index cae24b707..a43865c69 100644 --- a/test/fixtures/HubPool.Fixture.ts +++ b/test/fixtures/HubPool.Fixture.ts @@ -58,12 +58,10 @@ export const hubPoolFixture = hre.deployments.createFixture(async ({ ethers }) = // Deploy mock l2 tokens for each token created before and whitelist the routes. const mockTokens = { l2Weth: randomAddress(), l2Dai: randomAddress(), l2Usdc: randomAddress() }; - await hubPool.whitelistRoute(originChainId, repaymentChainId, weth.address, mockTokens.l2Weth, true); - await hubPool.whitelistRoute(originChainId, repaymentChainId, dai.address, mockTokens.l2Dai, true); - await hubPool.whitelistRoute(originChainId, repaymentChainId, usdc.address, mockTokens.l2Usdc, true); - await hubPool.whitelistRoute(mainnetChainId, repaymentChainId, weth.address, mockTokens.l2Weth, true); - await hubPool.whitelistRoute(mainnetChainId, repaymentChainId, dai.address, mockTokens.l2Dai, true); - await hubPool.whitelistRoute(mainnetChainId, repaymentChainId, usdc.address, mockTokens.l2Usdc, true); + // Whitelist pool rebalance routes but don't relay any messages to SpokePool + await hubPool.whitelistRoute(mainnetChainId, repaymentChainId, weth.address, mockTokens.l2Weth, true, false, true); + await hubPool.whitelistRoute(mainnetChainId, repaymentChainId, dai.address, mockTokens.l2Dai, true, false, true); + await hubPool.whitelistRoute(mainnetChainId, repaymentChainId, usdc.address, mockTokens.l2Usdc, true, false, true); return { ...tokens, ...mockTokens, hubPool, mockAdapter, mockSpoke, crossChainAdmin, ...parentFixture }; }); diff --git a/test/fixtures/SpokePool.Fixture.ts b/test/fixtures/SpokePool.Fixture.ts index 96aed8d9a..618cdd26d 100644 --- a/test/fixtures/SpokePool.Fixture.ts +++ b/test/fixtures/SpokePool.Fixture.ts @@ -1,4 +1,3 @@ -import { originChainId } from "./../constants"; import { TokenRolesEnum } from "@uma/common"; import { getContractFactory, SignerWithAddress, Contract, hre, ethers, BigNumber, defaultAbiCoder } from "../utils"; import * as consts from "../constants"; @@ -45,6 +44,7 @@ export async function deploySpokePool(ethers: any): Promise<{ export interface DepositRoute { originToken: string; + destinationToken: string; destinationChainId?: number; enabled?: boolean; } @@ -52,6 +52,7 @@ export async function enableRoutes(spokePool: Contract, routes: DepositRoute[]) for (const route of routes) { await spokePool.setEnableRoute( route.originToken, + route.destinationToken, route.destinationChainId ? route.destinationChainId : consts.destinationChainId, route.enabled !== undefined ? route.enabled : true ); diff --git a/test/gas-analytics/HubPool.RootExecution.ts b/test/gas-analytics/HubPool.RootExecution.ts index b41a5abff..701251925 100644 --- a/test/gas-analytics/HubPool.RootExecution.ts +++ b/test/gas-analytics/HubPool.RootExecution.ts @@ -103,7 +103,7 @@ describe("Gas Analytics: HubPool Root Bundle Execution", function () { // Just whitelist route from mainnet to l2 (hacky), which shouldn't change gas estimates, but will allow refunds to be sent. await Promise.all( l1Tokens.map(async (token) => { - await hubPool.whitelistRoute(hubPoolChainId, i, token.address, randomAddress(), true); + await hubPool.whitelistRoute(hubPoolChainId, i, token.address, randomAddress(), true, false, true); }) ); destinationChainIds.push(i); diff --git a/test/gas-analytics/utils.ts b/test/gas-analytics/utils.ts index 55922aeb7..485f89b1c 100644 --- a/test/gas-analytics/utils.ts +++ b/test/gas-analytics/utils.ts @@ -1,4 +1,4 @@ -import { SignerWithAddress, getContractFactory, BigNumber, toBN, Contract } from "../utils"; +import { SignerWithAddress, getContractFactory, BigNumber, toBN, Contract, randomAddress } from "../utils"; import { TokenRolesEnum } from "@uma/common"; import * as consts from "../constants"; import { getDepositParams, getRelayHash, getFillRelayParams, enableRoutes } from "../fixtures/SpokePool.Fixture"; @@ -63,6 +63,7 @@ export async function warmSpokePool( await enableRoutes(_spokePool, [ { originToken: _currencyAddress, + destinationToken: randomAddress(), destinationChainId: 1, }, ]); From 4175d660603b0f293e678c43803213c1b08593a4 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 16 Mar 2022 10:08:40 -0400 Subject: [PATCH 05/20] Update SpokePool.sol --- contracts/SpokePool.sol | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index f66a0b705..b506dc71a 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -306,6 +306,11 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall // In this case the msg.value will be set to 0, indicating a "normal" ERC20 bridging action. } else IERC20(originToken).safeTransferFrom(msg.sender, address(this), amount); + // Note: Off-chain relayers are expected to send to the recipient `destinationToken` on the destination chain, + // but there is a chance that the destination token is set incorrectly or cannot be sent for other reasons. In + // these cases, we expect that the UMIP explains how to handle such cases. For example, a rule could be: + // "If the destination token address doesn't exist on the destination chain, then wait to relay the deposit + // until the destination token address is reset, or refund the user their token on the origin chain". _emitDeposit( amount, chainId(), From 2955f320606744d43925b68f15c1d0710fa63662 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 16 Mar 2022 10:25:43 -0400 Subject: [PATCH 06/20] split up whitelistRoute --- contracts/HubPool.sol | 122 ++++++++------------ contracts/HubPoolInterface.sol | 21 ++-- test/HubPool.Admin.ts | 27 ++--- test/HubPool.ExecuteRootBundle.ts | 10 +- test/chain-adapters/Arbitrum_Adapter.ts | 4 +- test/chain-adapters/Ethereum_Adapter.ts | 4 +- test/chain-adapters/Optimism_Adapter.ts | 4 +- test/chain-adapters/Polygon_Adapter.ts | 4 +- test/fixtures/HubPool.Fixture.ts | 6 +- test/gas-analytics/HubPool.RootExecution.ts | 3 +- 10 files changed, 80 insertions(+), 125 deletions(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index 079ad65b9..6d397fb49 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -74,8 +74,8 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { // Whether the bundle proposal process is paused. bool public paused; - // Stores paths from origin token + ID to destination token + ID. Since different tokens on an origin might map to - // to the same address on a destination, we hash (origin chain ID, origin token address, destination ID) to + // Stores paths from origin token to destination ID + token. Since different tokens on an origin might map to + // to the same address on a destination, we hash (origin token address, destination ID) to // use as a key that maps to a destination token. This mapping is used to enable pool rebalances from // HubPool to SpokePool. Note that the value component of the mapping, or the "destination token" can be // set to 0x0 to disable a pool rebalance route. @@ -180,10 +180,9 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { address indexed liquidityProvider ); event SetPoolRebalanceRoute( - uint256 indexed originChainId, uint256 indexed destinationChainId, address indexed originToken, - address destinationToken + address indexed destinationToken ); event SetEnableDepositRoute( uint256 indexed originChainId, @@ -378,73 +377,48 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { } /** - * @notice Stores mapping from origin chain ID + token + destination chain ID to token. This mapping corresponds - * to token transfer routes and can be used for HubPool => SpokePool rebalances. Callable only by owner. - * @notice Can also be used to send cross-chain message to SpokePool on originChainId to enable or disable - * mapping from origin token to destination chain ID. If this path is enabled on the SpokePool, then relays - * are allowed to be sent via that path. - * @notice The meaning of a "route" is reused in this contract and is possible confusing. What's important to note - * is that token transfers always go from origin chain to destination chain. The HubPool stores a whitelist - * of routes where tokens can be sent from it to SpokePools for "pool rebalances", but it has no control over - * which deposits are enabled on SpokePools. Conversely, the SpokePool whitelists routes that deposited tokens - * can be sent along to execute cross-chain relays. The reason that this function is overloaded is because this - * contract is the only one that can update the SpokePool's whitelist of allowed deposit routes. It is likely - * in practice that the admin would want to simultaneously whitelist pool rebalance routes from the HubPool - * to the Optimism and Arbitrum SpokePools, and whitelist deposits from Optimism to Arbitrum. - * @param originChainId Chain where token transfer originates. - * @param destinationChainId Chain where token transfer ends up. - * @param originToken Token sent from origin chain. + * @notice Store which destination token we need to bridge to SpokePool on origin chain. Callable only by owner. + * @param destinationChainId Chain where pool rebalance sends tokens to. + * @param originToken Token sent from this pool. + * @param destinationToken Token received at destination chain. Destination chain counterpart to originToken. + */ + function setPoolRebalanceRoute( + uint256 destinationChainId, + address originToken, + address destinationToken + ) public override onlyOwner nonReentrant { + poolRebalanceRoutes[_poolRebalanceRouteKey(originToken, destinationChainId)] = destinationToken; + emit SetPoolRebalanceRoute(destinationChainId, originToken, destinationToken); + } + + /** + * @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. + * @param originChainId Chain where token deposit occurs. + * @param destinationChainId Chain where token depositor wants to receive funds. + * @param originToken Token sent in deposit. * @param destinationToken Token received at destination chain. * @param depositsEnabled Set to true to whitelist this route for deposits, set to false if caller just wants to - * map the origin token + origin ID + destination ID to the destination token address and wants to explicitly - * disable this route for deposits. This variable is used only if `relayToSpokePool` is True, in which case it is - * relayed as part of the message to the SpokePool. - * @param relayToSpokePool Set to true to relay a message to the SpokePool on the origin chain to enable - * deposits for the origin token to the destination chain ID. Set to false to skip this step - * @param setInHubPool Set to true to store the mapping from origin chain ID + token + destination chain ID to - * the destination token and whether deposits are enabled or not. Set to false to skip this step. + * map the origin token + destination ID to the destination token address on the origin chain's SpokePool. */ - - function whitelistRoute( + function setDepositRoute( uint256 originChainId, uint256 destinationChainId, address originToken, address destinationToken, - bool depositsEnabled, - bool relayToSpokePool, - bool setInHubPool - ) public override onlyOwner { - if (setInHubPool) { - // Store which destination token we need to bridge to SpokePool on origin chain. - // @dev There is an argument we should require that `originChainId == block.chainId` since pool rebalances - // will only ever originate from this contract, but this is an onlyOwner function and we leave it to the - // admin to use this function correctly. - poolRebalanceRoutes[ - _poolRebalanceRouteKey(originChainId, originToken, destinationChainId) - ] = destinationToken; - emit SetPoolRebalanceRoute(originChainId, destinationChainId, originToken, destinationToken); - } - if (relayToSpokePool) { - // Store whether deposit route from origin chain to destination chain should be enabled or disabled on - // spoke pool on origin chain. - _relaySpokePoolAdminFunction( - originChainId, - abi.encodeWithSignature( - "setEnableRoute(address,address,uint256,bool)", - originToken, - destinationToken, - destinationChainId, - depositsEnabled - ) - ); - emit SetEnableDepositRoute( - originChainId, - destinationChainId, + bool depositsEnabled + ) public override nonReentrant onlyOwner { + _relaySpokePoolAdminFunction( + originChainId, + abi.encodeWithSignature( + "setEnableRoute(address,address,uint256,bool)", originToken, destinationToken, + destinationChainId, depositsEnabled - ); - } + ) + ); + emit SetEnableDepositRoute(originChainId, destinationChainId, originToken, destinationToken, depositsEnabled); } /** @@ -835,20 +809,20 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { } /** - * @notice Conveniently queries which destination token is mapped to the hash of an origin chain + token + + * @notice Conveniently queries which destination token is mapped to the hash of an origin token + * destination chain ID for use in pool rebalances. - * @param originChainId Origin chain. * @param destinationChainId Where pool rebalance sends funds. * @param originToken Rebalance token. * @return destinationToken address SpokePool can receive this token on destination chain following pool * rebalance. */ - function poolRebalanceRoute( - uint256 originChainId, - uint256 destinationChainId, - address originToken - ) external view override returns (address destinationToken) { - return poolRebalanceRoutes[_poolRebalanceRouteKey(originChainId, originToken, destinationChainId)]; + function poolRebalanceRoute(uint256 destinationChainId, address originToken) + external + view + override + returns (address destinationToken) + { + return poolRebalanceRoutes[_poolRebalanceRouteKey(originToken, destinationChainId)]; } /** @@ -906,7 +880,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { address l1Token = l1Tokens[i]; // 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 = poolRebalanceRoutes[_poolRebalanceRouteKey(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 @@ -1060,12 +1034,8 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { emit SpokePoolAdminFunctionTriggered(chainId, functionData); } - function _poolRebalanceRouteKey( - uint256 originChainId, - address originToken, - uint256 destinationChainId - ) internal pure returns (bytes32) { - return keccak256(abi.encode(originChainId, originToken, destinationChainId)); + function _poolRebalanceRouteKey(address originToken, uint256 destinationChainId) internal pure returns (bytes32) { + return keccak256(abi.encode(originToken, destinationChainId)); } function _activeRequest() internal view returns (bool) { diff --git a/contracts/HubPoolInterface.sol b/contracts/HubPoolInterface.sol index e77861cb0..90a22433d 100644 --- a/contracts/HubPoolInterface.sol +++ b/contracts/HubPoolInterface.sol @@ -98,21 +98,24 @@ interface HubPoolInterface { function getRootBundleProposalAncillaryData() external view returns (bytes memory ancillaryData); - function whitelistRoute( - uint256 originChainId, + function setPoolRebalanceRoute( uint256 destinationChainId, address originToken, - address destinationToken, - bool depositsEnabled, - bool relayToSpokePool, - bool setInHubPool + address destinationToken ) external; - function poolRebalanceRoute( + function setDepositRoute( uint256 originChainId, uint256 destinationChainId, - address originToken - ) external view returns (address destinationToken); + address originToken, + address destinationToken, + bool depositsEnabled + ) external; + + function poolRebalanceRoute(uint256 destinationChainId, address originToken) + external + view + returns (address destinationToken); function loadEthForL2Calls() external payable; } diff --git a/test/HubPool.Admin.ts b/test/HubPool.Admin.ts index 4aa0482d5..426e62d43 100644 --- a/test/HubPool.Admin.ts +++ b/test/HubPool.Admin.ts @@ -47,33 +47,24 @@ describe("HubPool Admin functions", function () { hubPool.connect(other).setCrossChainContracts(destinationChainId, mockAdapter.address, mockSpoke.address) ).to.be.reverted; }); - it("Only owner can whitelist route for deposits and rebalances", async function () { + it("Only owner can whitelist route for rebalances and deposits", async function () { await hubPool.setCrossChainContracts(destinationChainId, mockAdapter.address, mockSpoke.address); - await expect( - hubPool - .connect(other) - .whitelistRoute(originChainId, destinationChainId, weth.address, usdc.address, true, false, true) - ).to.be.reverted; - await expect( - hubPool.whitelistRoute(originChainId, destinationChainId, weth.address, usdc.address, true, false, true) - ) + 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(originChainId, destinationChainId, weth.address, usdc.address); + .withArgs(destinationChainId, weth.address, usdc.address); - // Relay whitelist mapping to spoke pool. Check content of messages sent to mock spoke pool. + // Relay deposit mapping to spoke pool. Check content of messages sent to mock spoke pool. await expect( - hubPool - .connect(other) - .whitelistRoute(originChainId, destinationChainId, weth.address, usdc.address, true, true, false) + hubPool.connect(other).setDepositRoute(originChainId, destinationChainId, weth.address, usdc.address, true) ).to.be.reverted; - await expect( - hubPool.whitelistRoute(originChainId, destinationChainId, weth.address, usdc.address, true, true, false) - ) + await expect(hubPool.setDepositRoute(originChainId, destinationChainId, weth.address, usdc.address, true)) .to.emit(hubPool, "SetEnableDepositRoute") .withArgs(originChainId, destinationChainId, weth.address, usdc.address, true); // Disable deposit route on SpokePool right after: - await hubPool.whitelistRoute(originChainId, destinationChainId, weth.address, usdc.address, false, true, true); + await hubPool.setDepositRoute(originChainId, destinationChainId, weth.address, usdc.address, false); // Since the mock adapter is delegatecalled, when querying, its address should be the hubPool address. const mockAdapterAtHubPool = mockAdapter.attach(hubPool.address); diff --git a/test/HubPool.ExecuteRootBundle.ts b/test/HubPool.ExecuteRootBundle.ts index ad04d249a..332184bea 100644 --- a/test/HubPool.ExecuteRootBundle.ts +++ b/test/HubPool.ExecuteRootBundle.ts @@ -126,15 +126,7 @@ describe("HubPool Root Bundle Execution", function () { ); // Let's set weth pool rebalance route to zero address. - await hubPool.whitelistRoute( - await hre.getChainId(), - consts.repaymentChainId, - weth.address, - ZERO_ADDRESS, - true, - false, - true - ); + await hubPool.setPoolRebalanceRoute(consts.repaymentChainId, weth.address, ZERO_ADDRESS); // Advance time so the request can be executed and check that executing the request reverts. await timer.setCurrentTime(Number(await timer.getCurrentTime()) + consts.refundProposalLiveness + 1); diff --git a/test/chain-adapters/Arbitrum_Adapter.ts b/test/chain-adapters/Arbitrum_Adapter.ts index 6f3dd55ca..c02004ff9 100644 --- a/test/chain-adapters/Arbitrum_Adapter.ts +++ b/test/chain-adapters/Arbitrum_Adapter.ts @@ -40,8 +40,8 @@ describe("Arbitrum Chain Adapter", function () { await hubPool.setCrossChainContracts(arbitrumChainId, arbitrumAdapter.address, mockSpoke.address); - await hubPool.whitelistRoute(l1ChainId, arbitrumChainId, dai.address, l2Dai, true, false, true); - await hubPool.whitelistRoute(l1ChainId, arbitrumChainId, weth.address, l2Weth, true, false, true); + await hubPool.setPoolRebalanceRoute(arbitrumChainId, dai.address, l2Dai); + await hubPool.setPoolRebalanceRoute(arbitrumChainId, weth.address, l2Weth); }); it("relayMessage calls spoke pool functions", async function () { diff --git a/test/chain-adapters/Ethereum_Adapter.ts b/test/chain-adapters/Ethereum_Adapter.ts index 152e8be33..0dfe1226d 100644 --- a/test/chain-adapters/Ethereum_Adapter.ts +++ b/test/chain-adapters/Ethereum_Adapter.ts @@ -32,9 +32,9 @@ describe("Ethereum Chain Adapter", function () { await hubPool.setCrossChainContracts(l1ChainId, ethAdapter.address, mockSpoke.address); - await hubPool.whitelistRoute(l1ChainId, l1ChainId, weth.address, weth.address, true, false, true); + await hubPool.setPoolRebalanceRoute(l1ChainId, weth.address, weth.address); - await hubPool.whitelistRoute(l1ChainId, l1ChainId, dai.address, dai.address, true, false, true); + await hubPool.setPoolRebalanceRoute(l1ChainId, dai.address, dai.address); }); it("relayMessage calls spoke pool functions", async function () { diff --git a/test/chain-adapters/Optimism_Adapter.ts b/test/chain-adapters/Optimism_Adapter.ts index 112b9f6df..2476bb66f 100644 --- a/test/chain-adapters/Optimism_Adapter.ts +++ b/test/chain-adapters/Optimism_Adapter.ts @@ -42,8 +42,8 @@ describe("Optimism Chain Adapter", function () { ).deploy(weth.address, l1CrossDomainMessenger.address, l1StandardBridge.address); await hubPool.setCrossChainContracts(optimismChainId, optimismAdapter.address, mockSpoke.address); - await hubPool.whitelistRoute(l1ChainId, optimismChainId, weth.address, l2Weth, true, false, true); - await hubPool.whitelistRoute(l1ChainId, optimismChainId, dai.address, l2Dai, true, false, true); + await hubPool.setPoolRebalanceRoute(optimismChainId, weth.address, l2Weth); + await hubPool.setPoolRebalanceRoute(optimismChainId, dai.address, l2Dai); }); it("relayMessage calls spoke pool functions", async function () { diff --git a/test/chain-adapters/Polygon_Adapter.ts b/test/chain-adapters/Polygon_Adapter.ts index f199e7372..2e941f69e 100644 --- a/test/chain-adapters/Polygon_Adapter.ts +++ b/test/chain-adapters/Polygon_Adapter.ts @@ -42,8 +42,8 @@ describe("Polygon Chain Adapter", function () { ).deploy(rootChainManager.address, fxStateSender.address, weth.address); await hubPool.setCrossChainContracts(polygonChainId, polygonAdapter.address, mockSpoke.address); - await hubPool.whitelistRoute(l1ChainId, polygonChainId, weth.address, l2Weth, true, false, true); - await hubPool.whitelistRoute(l1ChainId, polygonChainId, dai.address, l2Dai, true, false, true); + await hubPool.setPoolRebalanceRoute(polygonChainId, weth.address, l2Weth); + await hubPool.setPoolRebalanceRoute(polygonChainId, dai.address, l2Dai); }); it("relayMessage calls spoke pool functions", async function () { diff --git a/test/fixtures/HubPool.Fixture.ts b/test/fixtures/HubPool.Fixture.ts index a43865c69..a72da3764 100644 --- a/test/fixtures/HubPool.Fixture.ts +++ b/test/fixtures/HubPool.Fixture.ts @@ -59,9 +59,9 @@ export const hubPoolFixture = hre.deployments.createFixture(async ({ ethers }) = const mockTokens = { l2Weth: randomAddress(), l2Dai: randomAddress(), l2Usdc: randomAddress() }; // Whitelist pool rebalance routes but don't relay any messages to SpokePool - await hubPool.whitelistRoute(mainnetChainId, repaymentChainId, weth.address, mockTokens.l2Weth, true, false, true); - await hubPool.whitelistRoute(mainnetChainId, repaymentChainId, dai.address, mockTokens.l2Dai, true, false, true); - await hubPool.whitelistRoute(mainnetChainId, repaymentChainId, usdc.address, mockTokens.l2Usdc, true, false, true); + await hubPool.setPoolRebalanceRoute(repaymentChainId, weth.address, mockTokens.l2Weth); + await hubPool.setPoolRebalanceRoute(repaymentChainId, dai.address, mockTokens.l2Dai); + await hubPool.setPoolRebalanceRoute(repaymentChainId, usdc.address, mockTokens.l2Usdc); return { ...tokens, ...mockTokens, hubPool, mockAdapter, mockSpoke, crossChainAdmin, ...parentFixture }; }); diff --git a/test/gas-analytics/HubPool.RootExecution.ts b/test/gas-analytics/HubPool.RootExecution.ts index 701251925..338d7be2f 100644 --- a/test/gas-analytics/HubPool.RootExecution.ts +++ b/test/gas-analytics/HubPool.RootExecution.ts @@ -100,10 +100,9 @@ describe("Gas Analytics: HubPool Root Bundle Execution", function () { await getContractFactory("MockSpokePool", owner) ).deploy(randomAddress(), hubPool.address, randomAddress(), ZERO_ADDRESS); await hubPool.setCrossChainContracts(i, adapter.address, spoke.address); - // Just whitelist route from mainnet to l2 (hacky), which shouldn't change gas estimates, but will allow refunds to be sent. await Promise.all( l1Tokens.map(async (token) => { - await hubPool.whitelistRoute(hubPoolChainId, i, token.address, randomAddress(), true, false, true); + await hubPool.setPoolRebalanceRoute(i, token.address, randomAddress()); }) ); destinationChainIds.push(i); From 7025fc3986242a60130c3c543c8915c3467e1f7f Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 16 Mar 2022 10:35:02 -0400 Subject: [PATCH 07/20] Fix --- test/HubPool.Admin.ts | 1 + test/HubPool.ExecuteRootBundle.ts | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/test/HubPool.Admin.ts b/test/HubPool.Admin.ts index d3a5c4a7c..e117b9eae 100644 --- a/test/HubPool.Admin.ts +++ b/test/HubPool.Admin.ts @@ -60,6 +60,7 @@ describe("HubPool Admin functions", function () { it("Only owner can relay spoke pool admin message", async function () { const functionData = mockSpoke.interface.encodeFunctionData("setEnableRoute", [ weth.address, + randomAddress(), destinationChainId, false, ]); diff --git a/test/HubPool.ExecuteRootBundle.ts b/test/HubPool.ExecuteRootBundle.ts index 9adcd918f..a0485b208 100644 --- a/test/HubPool.ExecuteRootBundle.ts +++ b/test/HubPool.ExecuteRootBundle.ts @@ -119,7 +119,7 @@ describe("HubPool Root Bundle Execution", function () { const relayMessageEvents = await mockAdapterAtHubPool.queryFilter( mockAdapterAtHubPool.filters.RelayMessageCalled() ); - expect(relayMessageEvents.length).to.equal(7); // Exactly seven message send from L1->L2. 6 for each whitelist route + expect(relayMessageEvents.length).to.equal(1); // Exactly one message sent from L1->L2. // and 1 for the initiateRelayerRefund. expect(relayMessageEvents[relayMessageEvents.length - 1].args?.target).to.equal(mockSpoke.address); expect(relayMessageEvents[relayMessageEvents.length - 1].args?.message).to.equal( From 7aa0b2f07c9dc2b4edaabee20c913d2b64aff9c1 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 16 Mar 2022 11:18:30 -0400 Subject: [PATCH 08/20] Add convenience method for admin to whitelist pool rebalance and deposit routes atomically --- contracts/HubPool.sol | 66 ++++++++++++++++++++ contracts/HubPoolInterface.sol | 9 +++ test/HubPool.Admin.ts | 110 +++++++++++++++++++++++++++++++++ 3 files changed, 185 insertions(+) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index 09952d3d4..0119f3477 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -426,6 +426,72 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { emit SetEnableDepositRoute(originChainId, destinationChainId, originToken, destinationToken, depositsEnabled); } + /** + * @notice Convenience method that whitelists (or unwhitelists) a pool rebalance route to two SpokePools and also + * whitelists deposits between the two spoke pools. Callable only by owner. For example, the admin might want + * to enable two-way USDC on Optimism <> USDC on Arbitrum relays. In practice, the admin would also need to + * enable pool rebalances from this contract to Optimism and Arbitrum for USDC. So this function would execute + * four transactions atomically: + * - whitelist pool rebalance route from USDC on Ethereum to USDC on Optimism + * - whitelist pool rebalance route from USDC on Ethereum to USDC on Arbitrum + * - whitelist deposit route from USDC on Optimism to USDC on Arbitrum + * - whitelist deposit route from USDC on Arbitrum to USDC on Optimism + * @param depositRouteChainId_1 Chain ID for one SpokePool that we want to enable as a deposit origin and + * destination. Pool rebalances to this chain will also be enabled. + * @param depositRouteChainId_2 The other chain in addition to `depositRouteChainId_1` that we want to enable + * as a deposit origin and destination, and make available for pool rebalances. + * @param ethereumCounterpartToken Token on the current network that will be sent to the SpokePool on + * both `depositRouteChainId_1` and ``depositRouteChainId_2` for pool rebalances. + * @param l2Token_1 Token that we want to enable for deposits on `depositRouteChainId_1` that can be fulfilled by + * `l2Token_2` on `depositRouteChainId_2`. Should be the chainId_1 counterpart of `ethereumCounterpartToken`. + * @param l2Token_2 Token that we want to enable for deposits on `depositRouteChainId_2` that can be fulfilled by + * `l2Token_1` on `depositRouteChainId_1`. Should be the chainId_2 counterpart of `ethereumCounterpartToken`. + * @param enable Set to True to set up pool rebalances to `depositRouteChainId_1` and `depositRouteChainId_2` and + * deposits from `depositRouteChainId_1` to `depositRouteChainId_2`. Set to False to disable all of the above. If + * this value is false, the pool rebalance destination tokens will be set to 0x0 and the deposit routes will be + * disabled on the SpokePools. + */ + function setDepositAndPoolRebalanceRoute( + uint256 depositRouteChainId_1, + uint256 depositRouteChainId_2, + address ethereumCounterpartToken, + address l2Token_1, + address l2Token_2, + bool enable + ) public override nonReentrant onlyOwner { + poolRebalanceRoutes[_poolRebalanceRouteKey(ethereumCounterpartToken, depositRouteChainId_1)] = ( + enable ? l2Token_1 : address(0) + ); + poolRebalanceRoutes[_poolRebalanceRouteKey(ethereumCounterpartToken, depositRouteChainId_2)] = ( + enable ? l2Token_2 : address(0) + ); + _relaySpokePoolAdminFunction( + depositRouteChainId_1, + abi.encodeWithSignature( + "setEnableRoute(address,address,uint256,bool)", + l2Token_1, + l2Token_2, + depositRouteChainId_2, + enable + ) + ); + _relaySpokePoolAdminFunction( + depositRouteChainId_2, + abi.encodeWithSignature( + "setEnableRoute(address,address,uint256,bool)", + l2Token_2, + l2Token_1, + depositRouteChainId_1, + enable + ) + ); + + emit SetPoolRebalanceRoute(depositRouteChainId_1, ethereumCounterpartToken, l2Token_1); + emit SetPoolRebalanceRoute(depositRouteChainId_2, ethereumCounterpartToken, l2Token_2); + emit SetEnableDepositRoute(depositRouteChainId_1, depositRouteChainId_2, l2Token_1, l2Token_2, enable); + emit SetEnableDepositRoute(depositRouteChainId_2, depositRouteChainId_1, l2Token_2, l2Token_1, enable); + } + /** * @notice Enables LPs to provide liquidity for L1 token. Deploys new LP token for L1 token if appropriate. * Callable only by owner. diff --git a/contracts/HubPoolInterface.sol b/contracts/HubPoolInterface.sol index b59f5508b..5b972eb3c 100644 --- a/contracts/HubPoolInterface.sol +++ b/contracts/HubPoolInterface.sol @@ -120,6 +120,15 @@ interface HubPoolInterface { bool depositsEnabled ) external; + function setDepositAndPoolRebalanceRoute( + uint256 depositRouteChainId_1, + uint256 depositRouteChainId_2, + address ethereumCounterpartToken, + address l2Token_1, + address l2Token_2, + bool enable + ) external; + function poolRebalanceRoute(uint256 destinationChainId, address originToken) external view diff --git a/test/HubPool.Admin.ts b/test/HubPool.Admin.ts index e117b9eae..fd5142a8b 100644 --- a/test/HubPool.Admin.ts +++ b/test/HubPool.Admin.ts @@ -116,6 +116,116 @@ describe("HubPool Admin functions", function () { ); }); + it("Only owner can whitelist deposits and rebalances atomically", async function () { + await hubPool.setCrossChainContracts(destinationChainId, mockAdapter.address, mockSpoke.address); + await hubPool.setCrossChainContracts(originChainId, mockAdapter.address, mockSpoke.address); + + const wethOnOriginChain = randomAddress(); + const wethOnDestinationChain = randomAddress(); + await expect( + hubPool + .connect(other) + .setDepositAndPoolRebalanceRoute( + originChainId, + destinationChainId, + weth.address, + wethOnOriginChain, + wethOnDestinationChain, + true + ) + ).to.be.reverted; + await hubPool.setDepositAndPoolRebalanceRoute( + originChainId, + destinationChainId, + weth.address, + wethOnOriginChain, + wethOnDestinationChain, + true + ); + + // Check pool rebalance routes + expect(await hubPool.poolRebalanceRoute(originChainId, weth.address)).to.equal(wethOnOriginChain); + expect(await hubPool.poolRebalanceRoute(destinationChainId, weth.address)).to.equal(wethOnDestinationChain); + + // Check content of messages relayed to spoke pools + const mockAdapterAtHubPool = mockAdapter.attach(hubPool.address); + let relayMessageEvents = await mockAdapterAtHubPool.queryFilter(mockAdapterAtHubPool.filters.RelayMessageCalled()); + expect(relayMessageEvents[relayMessageEvents.length - 1].args?.message).to.equal( + mockSpoke.interface.encodeFunctionData("setEnableRoute", [ + wethOnDestinationChain, + wethOnOriginChain, + originChainId, + true, + ]) + ); + expect(relayMessageEvents[relayMessageEvents.length - 2].args?.message).to.equal( + mockSpoke.interface.encodeFunctionData("setEnableRoute", [ + wethOnOriginChain, + wethOnDestinationChain, + destinationChainId, + true, + ]) + ); + + // Check that HubPool emitted four events. + const poolRebalanceRouteEvents = await hubPool.queryFilter(hubPool.filters.SetPoolRebalanceRoute()); + expect(poolRebalanceRouteEvents[poolRebalanceRouteEvents.length - 1].args?.destinationChainId).to.equal( + destinationChainId + ); + expect(poolRebalanceRouteEvents[poolRebalanceRouteEvents.length - 1].args?.originToken).to.equal(weth.address); + expect(poolRebalanceRouteEvents[poolRebalanceRouteEvents.length - 1].args?.destinationToken).to.equal( + wethOnDestinationChain + ); + expect(poolRebalanceRouteEvents[poolRebalanceRouteEvents.length - 2].args?.destinationChainId).to.equal( + originChainId + ); + expect(poolRebalanceRouteEvents[poolRebalanceRouteEvents.length - 2].args?.originToken).to.equal(weth.address); + expect(poolRebalanceRouteEvents[poolRebalanceRouteEvents.length - 2].args?.destinationToken).to.equal( + wethOnOriginChain + ); + const depositRouteEvents = await hubPool.queryFilter(hubPool.filters.SetEnableDepositRoute()); + expect(depositRouteEvents[depositRouteEvents.length - 1].args?.originChainId).to.equal(destinationChainId); + expect(depositRouteEvents[depositRouteEvents.length - 1].args?.destinationChainId).to.equal(originChainId); + expect(depositRouteEvents[depositRouteEvents.length - 1].args?.originToken).to.equal(wethOnDestinationChain); + expect(depositRouteEvents[depositRouteEvents.length - 1].args?.destinationToken).to.equal(wethOnOriginChain); + expect(depositRouteEvents[depositRouteEvents.length - 1].args?.depositsEnabled).to.equal(true); + expect(depositRouteEvents[depositRouteEvents.length - 2].args?.originChainId).to.equal(originChainId); + expect(depositRouteEvents[depositRouteEvents.length - 2].args?.destinationChainId).to.equal(destinationChainId); + expect(depositRouteEvents[depositRouteEvents.length - 2].args?.originToken).to.equal(wethOnOriginChain); + expect(depositRouteEvents[depositRouteEvents.length - 2].args?.destinationToken).to.equal(wethOnDestinationChain); + expect(depositRouteEvents[depositRouteEvents.length - 2].args?.depositsEnabled).to.equal(true); + + // Now disable the routes and check that pool rebalance destination tokens are zeroed out and relayed message + // correctly disables the deposit routes. + await hubPool.setDepositAndPoolRebalanceRoute( + originChainId, + destinationChainId, + weth.address, + wethOnOriginChain, + wethOnDestinationChain, + false + ); + expect(await hubPool.poolRebalanceRoute(originChainId, weth.address)).to.equal(ZERO_ADDRESS); + expect(await hubPool.poolRebalanceRoute(destinationChainId, weth.address)).to.equal(ZERO_ADDRESS); + relayMessageEvents = await mockAdapterAtHubPool.queryFilter(mockAdapterAtHubPool.filters.RelayMessageCalled()); + expect(relayMessageEvents[relayMessageEvents.length - 1].args?.message).to.equal( + mockSpoke.interface.encodeFunctionData("setEnableRoute", [ + wethOnDestinationChain, + wethOnOriginChain, + originChainId, + false, + ]) + ); + expect(relayMessageEvents[relayMessageEvents.length - 2].args?.message).to.equal( + mockSpoke.interface.encodeFunctionData("setEnableRoute", [ + wethOnOriginChain, + wethOnDestinationChain, + destinationChainId, + false, + ]) + ); + }); + it("Can change the bond token and amount", async function () { expect(await hubPool.callStatic.bondToken()).to.equal(weth.address); // Default set in the fixture. expect(await hubPool.callStatic.bondAmount()).to.equal(bondAmount.add(finalFee)); // Default set in the fixture. From 2b2c0ec98c42ed2bbda9eebda314b3a1b549d3c0 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 16 Mar 2022 12:28:17 -0400 Subject: [PATCH 09/20] Add bi directional helper method alongside single direction helper --- contracts/HubPool.sol | 122 +++++++++++++++++++++++++-------- contracts/HubPoolInterface.sol | 9 +++ test/HubPool.Admin.ts | 72 ++++++++++++++----- 3 files changed, 157 insertions(+), 46 deletions(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index 0119f3477..e30b6919f 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -426,11 +426,53 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { emit SetEnableDepositRoute(originChainId, destinationChainId, originToken, destinationToken, depositsEnabled); } + /** + * @notice Convenience method that whitelists (or unwhitelists) a pool rebalance route to a destination SpokePool + * and whitelists deposits from a different spoke pool to that destination spoke pool. Callable only by owner. + * For example, the admin might want to enable USDC on Optimism --> USDC on Arbitrum relays. In practice, the + * admin would also need to enable pool rebalances from this contract to Arbitrum for USDC to refund relayers. + * So the admin would like to execute two transactions atomically: + * - whitelist pool rebalance route from USDC on Ethereum to USDC on Arbitrum + * - whitelist deposit route from USDC on Optimism to USDC on Arbitrum + * @param depositOriginChainId Chain ID for one SpokePool that we want to enable as a deposit origin. + * @param depositDestinationChainId Chain ID that we want the newly whitelited deposit route to use as a + * destination. Will also be enabled for pool rebalances from this contract, since we'll likely need to + * send funds to this network to refund relayers. + * @param ethereumCounterpartToken Token on the current network that will be sent to the SpokePool on + * both `depositDestinationChainId` for pool rebalances. + * @param originToken Token that we want to enable for deposits on `depositOriginChainId` that can be fulfilled by + * `destinationToken` on `depositDestinationChainId`. Should be the `depositOriginChainId` counterpart of + * `ethereumCounterpartToken`. + * @param destinationToken Token that deposits from `depositOriginChainId` for the `originToken` should be sent + * to recipients on `depositDestinationChainId`. + * @param enable Set to True to set up pool rebalances to `depositDestinationChainId` and deposits from + * `depositOriginChainId` to `depositDestinationChainId`. Set to False to disable all of the above. If + * this value is false, the pool rebalance destination token will be set to 0x0 and the deposit route will be + * disabled on the SpokePools. + */ + function setDepositAndPoolRebalanceRoute( + uint256 depositOriginChainId, + uint256 depositDestinationChainId, + address ethereumCounterpartToken, + address originToken, + address destinationToken, + bool enable + ) public override nonReentrant onlyOwner { + _setDepositAndPoolRebalanceRoute( + depositOriginChainId, + depositDestinationChainId, + ethereumCounterpartToken, + originToken, + destinationToken, + enable + ); + } + /** * @notice Convenience method that whitelists (or unwhitelists) a pool rebalance route to two SpokePools and also * whitelists deposits between the two spoke pools. Callable only by owner. For example, the admin might want * to enable two-way USDC on Optimism <> USDC on Arbitrum relays. In practice, the admin would also need to - * enable pool rebalances from this contract to Optimism and Arbitrum for USDC. So this function would execute + * enable pool rebalances from this contract to Optimism and Arbitrum for USDC. So the admin would like to execute * four transactions atomically: * - whitelist pool rebalance route from USDC on Ethereum to USDC on Optimism * - whitelist pool rebalance route from USDC on Ethereum to USDC on Arbitrum @@ -441,7 +483,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { * @param depositRouteChainId_2 The other chain in addition to `depositRouteChainId_1` that we want to enable * as a deposit origin and destination, and make available for pool rebalances. * @param ethereumCounterpartToken Token on the current network that will be sent to the SpokePool on - * both `depositRouteChainId_1` and ``depositRouteChainId_2` for pool rebalances. + * both `depositRouteChainId_1` and `depositRouteChainId_2` for pool rebalances. * @param l2Token_1 Token that we want to enable for deposits on `depositRouteChainId_1` that can be fulfilled by * `l2Token_2` on `depositRouteChainId_2`. Should be the chainId_1 counterpart of `ethereumCounterpartToken`. * @param l2Token_2 Token that we want to enable for deposits on `depositRouteChainId_2` that can be fulfilled by @@ -451,7 +493,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { * this value is false, the pool rebalance destination tokens will be set to 0x0 and the deposit routes will be * disabled on the SpokePools. */ - function setDepositAndPoolRebalanceRoute( + function setDepositAndPoolRebalanceBiDirectionRoute( uint256 depositRouteChainId_1, uint256 depositRouteChainId_2, address ethereumCounterpartToken, @@ -459,37 +501,22 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { address l2Token_2, bool enable ) public override nonReentrant onlyOwner { - poolRebalanceRoutes[_poolRebalanceRouteKey(ethereumCounterpartToken, depositRouteChainId_1)] = ( - enable ? l2Token_1 : address(0) - ); - poolRebalanceRoutes[_poolRebalanceRouteKey(ethereumCounterpartToken, depositRouteChainId_2)] = ( - enable ? l2Token_2 : address(0) - ); - _relaySpokePoolAdminFunction( + _setDepositAndPoolRebalanceRoute( + depositRouteChainId_2, depositRouteChainId_1, - abi.encodeWithSignature( - "setEnableRoute(address,address,uint256,bool)", - l2Token_1, - l2Token_2, - depositRouteChainId_2, - enable - ) + ethereumCounterpartToken, + l2Token_2, + l2Token_1, + enable ); - _relaySpokePoolAdminFunction( + _setDepositAndPoolRebalanceRoute( + depositRouteChainId_1, depositRouteChainId_2, - abi.encodeWithSignature( - "setEnableRoute(address,address,uint256,bool)", - l2Token_2, - l2Token_1, - depositRouteChainId_1, - enable - ) + ethereumCounterpartToken, + l2Token_1, + l2Token_2, + enable ); - - emit SetPoolRebalanceRoute(depositRouteChainId_1, ethereumCounterpartToken, l2Token_1); - emit SetPoolRebalanceRoute(depositRouteChainId_2, ethereumCounterpartToken, l2Token_2); - emit SetEnableDepositRoute(depositRouteChainId_1, depositRouteChainId_2, l2Token_1, l2Token_2, enable); - emit SetEnableDepositRoute(depositRouteChainId_2, depositRouteChainId_1, l2Token_2, l2Token_1, enable); } /** @@ -925,6 +952,41 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { * INTERNAL FUNCTIONS * *************************************************/ + function _setDepositAndPoolRebalanceRoute( + uint256 depositOriginChainId, + uint256 depositDestinationChainId, + address ethereumCounterpartToken, + address originToken, + address destinationToken, + bool enable + ) internal { + poolRebalanceRoutes[_poolRebalanceRouteKey(ethereumCounterpartToken, depositDestinationChainId)] = ( + enable ? destinationToken : address(0) + ); + _relaySpokePoolAdminFunction( + depositOriginChainId, + abi.encodeWithSignature( + "setEnableRoute(address,address,uint256,bool)", + originToken, + destinationToken, + depositDestinationChainId, + enable + ) + ); + emit SetPoolRebalanceRoute( + depositDestinationChainId, + ethereumCounterpartToken, + enable ? destinationToken : address(0) + ); + emit SetEnableDepositRoute( + depositOriginChainId, + depositDestinationChainId, + originToken, + destinationToken, + enable + ); + } + // Called when a dispute fails due to parameter changes. This effectively resets the state and cancels the request // with no loss of funds, thereby enabling a new bundle to be added. function _cancelBundle(bytes memory ancillaryData) internal { diff --git a/contracts/HubPoolInterface.sol b/contracts/HubPoolInterface.sol index 5b972eb3c..810921955 100644 --- a/contracts/HubPoolInterface.sol +++ b/contracts/HubPoolInterface.sol @@ -121,6 +121,15 @@ interface HubPoolInterface { ) external; function setDepositAndPoolRebalanceRoute( + uint256 depositOriginChainId, + uint256 depositDestinationChainId, + address ethereumCounterpartToken, + address originToken, + address destinationToken, + bool enable + ) external; + + function setDepositAndPoolRebalanceBiDirectionRoute( uint256 depositRouteChainId_1, uint256 depositRouteChainId_2, address ethereumCounterpartToken, diff --git a/test/HubPool.Admin.ts b/test/HubPool.Admin.ts index fd5142a8b..87f24bdee 100644 --- a/test/HubPool.Admin.ts +++ b/test/HubPool.Admin.ts @@ -125,7 +125,7 @@ describe("HubPool Admin functions", function () { await expect( hubPool .connect(other) - .setDepositAndPoolRebalanceRoute( + .setDepositAndPoolRebalanceBiDirectionRoute( originChainId, destinationChainId, weth.address, @@ -134,7 +134,7 @@ describe("HubPool Admin functions", function () { true ) ).to.be.reverted; - await hubPool.setDepositAndPoolRebalanceRoute( + await hubPool.setDepositAndPoolRebalanceBiDirectionRoute( originChainId, destinationChainId, weth.address, @@ -150,7 +150,7 @@ describe("HubPool Admin functions", function () { // Check content of messages relayed to spoke pools const mockAdapterAtHubPool = mockAdapter.attach(hubPool.address); let relayMessageEvents = await mockAdapterAtHubPool.queryFilter(mockAdapterAtHubPool.filters.RelayMessageCalled()); - expect(relayMessageEvents[relayMessageEvents.length - 1].args?.message).to.equal( + expect(relayMessageEvents[relayMessageEvents.length - 2].args?.message).to.equal( mockSpoke.interface.encodeFunctionData("setEnableRoute", [ wethOnDestinationChain, wethOnOriginChain, @@ -158,7 +158,7 @@ describe("HubPool Admin functions", function () { true, ]) ); - expect(relayMessageEvents[relayMessageEvents.length - 2].args?.message).to.equal( + expect(relayMessageEvents[relayMessageEvents.length - 1].args?.message).to.equal( mockSpoke.interface.encodeFunctionData("setEnableRoute", [ wethOnOriginChain, wethOnDestinationChain, @@ -184,20 +184,20 @@ describe("HubPool Admin functions", function () { wethOnOriginChain ); const depositRouteEvents = await hubPool.queryFilter(hubPool.filters.SetEnableDepositRoute()); - expect(depositRouteEvents[depositRouteEvents.length - 1].args?.originChainId).to.equal(destinationChainId); - expect(depositRouteEvents[depositRouteEvents.length - 1].args?.destinationChainId).to.equal(originChainId); - expect(depositRouteEvents[depositRouteEvents.length - 1].args?.originToken).to.equal(wethOnDestinationChain); - expect(depositRouteEvents[depositRouteEvents.length - 1].args?.destinationToken).to.equal(wethOnOriginChain); - expect(depositRouteEvents[depositRouteEvents.length - 1].args?.depositsEnabled).to.equal(true); - expect(depositRouteEvents[depositRouteEvents.length - 2].args?.originChainId).to.equal(originChainId); - expect(depositRouteEvents[depositRouteEvents.length - 2].args?.destinationChainId).to.equal(destinationChainId); - expect(depositRouteEvents[depositRouteEvents.length - 2].args?.originToken).to.equal(wethOnOriginChain); - expect(depositRouteEvents[depositRouteEvents.length - 2].args?.destinationToken).to.equal(wethOnDestinationChain); + expect(depositRouteEvents[depositRouteEvents.length - 2].args?.originChainId).to.equal(destinationChainId); + expect(depositRouteEvents[depositRouteEvents.length - 2].args?.destinationChainId).to.equal(originChainId); + expect(depositRouteEvents[depositRouteEvents.length - 2].args?.originToken).to.equal(wethOnDestinationChain); + expect(depositRouteEvents[depositRouteEvents.length - 2].args?.destinationToken).to.equal(wethOnOriginChain); expect(depositRouteEvents[depositRouteEvents.length - 2].args?.depositsEnabled).to.equal(true); + expect(depositRouteEvents[depositRouteEvents.length - 1].args?.originChainId).to.equal(originChainId); + expect(depositRouteEvents[depositRouteEvents.length - 1].args?.destinationChainId).to.equal(destinationChainId); + expect(depositRouteEvents[depositRouteEvents.length - 1].args?.originToken).to.equal(wethOnOriginChain); + expect(depositRouteEvents[depositRouteEvents.length - 1].args?.destinationToken).to.equal(wethOnDestinationChain); + expect(depositRouteEvents[depositRouteEvents.length - 1].args?.depositsEnabled).to.equal(true); // Now disable the routes and check that pool rebalance destination tokens are zeroed out and relayed message // correctly disables the deposit routes. - await hubPool.setDepositAndPoolRebalanceRoute( + await hubPool.setDepositAndPoolRebalanceBiDirectionRoute( originChainId, destinationChainId, weth.address, @@ -208,7 +208,7 @@ describe("HubPool Admin functions", function () { expect(await hubPool.poolRebalanceRoute(originChainId, weth.address)).to.equal(ZERO_ADDRESS); expect(await hubPool.poolRebalanceRoute(destinationChainId, weth.address)).to.equal(ZERO_ADDRESS); relayMessageEvents = await mockAdapterAtHubPool.queryFilter(mockAdapterAtHubPool.filters.RelayMessageCalled()); - expect(relayMessageEvents[relayMessageEvents.length - 1].args?.message).to.equal( + expect(relayMessageEvents[relayMessageEvents.length - 2].args?.message).to.equal( mockSpoke.interface.encodeFunctionData("setEnableRoute", [ wethOnDestinationChain, wethOnOriginChain, @@ -216,7 +216,7 @@ describe("HubPool Admin functions", function () { false, ]) ); - expect(relayMessageEvents[relayMessageEvents.length - 2].args?.message).to.equal( + expect(relayMessageEvents[relayMessageEvents.length - 1].args?.message).to.equal( mockSpoke.interface.encodeFunctionData("setEnableRoute", [ wethOnOriginChain, wethOnDestinationChain, @@ -224,6 +224,46 @@ describe("HubPool Admin functions", function () { false, ]) ); + + // Finally, re-enable the routes individually using the one-way helper methods. + await hubPool.setDepositAndPoolRebalanceRoute( + originChainId, + destinationChainId, + weth.address, + wethOnOriginChain, + wethOnDestinationChain, + true + ); + expect(await hubPool.poolRebalanceRoute(destinationChainId, weth.address)).to.equal(wethOnDestinationChain); + expect(await hubPool.poolRebalanceRoute(originChainId, weth.address)).to.equal(ZERO_ADDRESS); + relayMessageEvents = await mockAdapterAtHubPool.queryFilter(mockAdapterAtHubPool.filters.RelayMessageCalled()); + expect(relayMessageEvents[relayMessageEvents.length - 1].args?.message).to.equal( + mockSpoke.interface.encodeFunctionData("setEnableRoute", [ + wethOnOriginChain, + wethOnDestinationChain, + destinationChainId, + true, + ]) + ); + await hubPool.setDepositAndPoolRebalanceRoute( + destinationChainId, + originChainId, + weth.address, + wethOnDestinationChain, + wethOnOriginChain, + true + ); + expect(await hubPool.poolRebalanceRoute(destinationChainId, weth.address)).to.equal(wethOnDestinationChain); + expect(await hubPool.poolRebalanceRoute(originChainId, weth.address)).to.equal(wethOnOriginChain); + relayMessageEvents = await mockAdapterAtHubPool.queryFilter(mockAdapterAtHubPool.filters.RelayMessageCalled()); + expect(relayMessageEvents[relayMessageEvents.length - 1].args?.message).to.equal( + mockSpoke.interface.encodeFunctionData("setEnableRoute", [ + wethOnDestinationChain, + wethOnOriginChain, + originChainId, + true, + ]) + ); }); it("Can change the bond token and amount", async function () { From 0bbf20871ca2c5004e3afed5204498d94ad03293 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 16 Mar 2022 17:03:05 -0400 Subject: [PATCH 10/20] remove destinationToken from spokePool --- contracts/Arbitrum_SpokePool.sol | 29 ++- contracts/HubPool.sol | 175 +++--------------- contracts/HubPoolInterface.sol | 23 +-- contracts/SpokePool.sol | 38 +--- contracts/SpokePoolInterface.sol | 1 - test/HubPool.Admin.ts | 166 +---------------- test/SpokePool.Admin.ts | 9 +- test/SpokePool.Deposit.ts | 26 +-- test/SpokePool.SlowRelay.ts | 5 +- .../Arbitrum_SpokePool.ts | 19 +- .../Ethereum_SpokePool.ts | 12 +- .../Optimism_SpokePool.ts | 8 +- .../Polygon_SpokePool.ts | 11 +- test/fixtures/SpokePool.Fixture.ts | 2 - test/gas-analytics/utils.ts | 3 +- 15 files changed, 92 insertions(+), 435 deletions(-) diff --git a/contracts/Arbitrum_SpokePool.sol b/contracts/Arbitrum_SpokePool.sol index 605fc3715..f887431a4 100644 --- a/contracts/Arbitrum_SpokePool.sol +++ b/contracts/Arbitrum_SpokePool.sol @@ -20,8 +20,13 @@ contract Arbitrum_SpokePool is SpokePool { // Address of the Arbitrum L2 token gateway to send funds to L1. address public l2GatewayRouter; + // Admin controlled mapping of arbitrum tokens to L1 counterpart. L1 counterpart addresses + // are necessary params used when bridging tokens to L1. + mapping(address => address) public whitelistedTokens; + event ArbitrumTokensBridged(address indexed l1Token, address target, uint256 numberOfTokensBridged); event SetL2GatewayRouter(address indexed newL2GatewayRouter); + event WhitelistedTokens(address indexed l2Token, address indexed l1Token); /** * @notice Construct the AVM SpokePool. @@ -58,19 +63,24 @@ contract Arbitrum_SpokePool is SpokePool { _setL2GatewayRouter(newL2GatewayRouter); } + /** + * @notice Add L2 -> L1 token mapping. Callable only by admin. + * @param l2Token Arbitrum token. + * @param l1Token Ethereum version of l2Token. + */ + function whitelistToken(address l2Token, address l1Token) public onlyAdmin { + _whitelistToken(l2Token, l1Token); + } + /************************************** * INTERNAL FUNCTIONS * **************************************/ function _bridgeTokensToHubPool(RelayerRefundLeaf memory relayerRefundLeaf) internal override { - // Check that the Ethereum counterpart of the L2 token is stored on this contract. We assume that the HubPool - // is deployed with a chain ID of 1. - require( - enabledDepositRoutes[relayerRefundLeaf.l2TokenAddress][1].destinationToken != address(0), - "Uninitialized mainnet token" - ); + // Check that the Ethereum counterpart of the L2 token is stored on this contract. + require(whitelistedTokens[relayerRefundLeaf.l2TokenAddress] != address(0), "Uninitialized mainnet token"); StandardBridgeLike(l2GatewayRouter).outboundTransfer( - enabledDepositRoutes[relayerRefundLeaf.l2TokenAddress][1].destinationToken, // _l1Token. Address of the + whitelistedTokens[relayerRefundLeaf.l2TokenAddress], // _l1Token. Address of the L1 token to bridge over., // _l1Token. Address of the // L1 token to bridge over. hubPool, // _to. Withdraw, over the bridge, to the l1 hub pool contract. relayerRefundLeaf.amountToReturn, // _amount. @@ -79,6 +89,11 @@ contract Arbitrum_SpokePool is SpokePool { emit ArbitrumTokensBridged(address(0), hubPool, relayerRefundLeaf.amountToReturn); } + function _whitelistToken(address _l2Token, address _l1Token) internal { + whitelistedTokens[_l2Token] = _l1Token; + emit WhitelistedTokens(_l2Token, _l1Token); + } + function _setL2GatewayRouter(address _l2GatewayRouter) internal { l2GatewayRouter = _l2GatewayRouter; emit SetL2GatewayRouter(l2GatewayRouter); diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index e30b6919f..c90dbf01b 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -77,11 +77,12 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { // Whether the bundle proposal process is paused. bool public paused; - // Stores paths from origin token to destination ID + token. Since different tokens on an origin might map to - // to the same address on a destination, we hash (origin token address, destination ID) to - // use as a key that maps to a destination token. This mapping is used to enable pool rebalances from - // HubPool to SpokePool. Note that the value component of the mapping, or the "destination token" can be - // set to 0x0 to disable a pool rebalance route. + // Stores paths from L1 token to destination ID + 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. mapping(bytes32 => address) private poolRebalanceRoutes; struct PooledToken { @@ -184,14 +185,13 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { ); event SetPoolRebalanceRoute( uint256 indexed destinationChainId, - address indexed originToken, + address indexed l1Token, address indexed destinationToken ); event SetEnableDepositRoute( uint256 indexed originChainId, uint256 indexed destinationChainId, address indexed originToken, - address destinationToken, bool depositsEnabled ); event ProposeRootBundle( @@ -383,17 +383,19 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { /** * @notice Store which destination token we need to bridge to SpokePool on origin chain. Callable only by owner. + * @dev Admin can set destinationToken to effectively disable pool rebalances for this l1 token + destination + * chain ID combination. * @param destinationChainId Chain where pool rebalance sends tokens to. - * @param originToken Token sent from this pool. - * @param destinationToken Token received at destination chain. Destination chain counterpart to originToken. + * @param l1Token Token sent from this pool. + * @param destinationToken Token received at destination chain. Destination chain counterpart to l1Token. */ function setPoolRebalanceRoute( uint256 destinationChainId, - address originToken, + address l1Token, address destinationToken ) public override onlyOwner nonReentrant { - poolRebalanceRoutes[_poolRebalanceRouteKey(originToken, destinationChainId)] = destinationToken; - emit SetPoolRebalanceRoute(destinationChainId, originToken, destinationToken); + poolRebalanceRoutes[_poolRebalanceRouteKey(l1Token, destinationChainId)] = destinationToken; + emit SetPoolRebalanceRoute(destinationChainId, l1Token, destinationToken); } /** @@ -402,7 +404,6 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { * @param originChainId Chain where token deposit occurs. * @param destinationChainId Chain where token depositor wants to receive funds. * @param originToken Token sent in deposit. - * @param destinationToken Token received at destination chain. * @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. */ @@ -410,113 +411,18 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { uint256 originChainId, uint256 destinationChainId, address originToken, - address destinationToken, bool depositsEnabled ) public override nonReentrant onlyOwner { _relaySpokePoolAdminFunction( originChainId, abi.encodeWithSignature( - "setEnableRoute(address,address,uint256,bool)", + "setEnableRoute(address,uint256,bool)", originToken, - destinationToken, destinationChainId, depositsEnabled ) ); - emit SetEnableDepositRoute(originChainId, destinationChainId, originToken, destinationToken, depositsEnabled); - } - - /** - * @notice Convenience method that whitelists (or unwhitelists) a pool rebalance route to a destination SpokePool - * and whitelists deposits from a different spoke pool to that destination spoke pool. Callable only by owner. - * For example, the admin might want to enable USDC on Optimism --> USDC on Arbitrum relays. In practice, the - * admin would also need to enable pool rebalances from this contract to Arbitrum for USDC to refund relayers. - * So the admin would like to execute two transactions atomically: - * - whitelist pool rebalance route from USDC on Ethereum to USDC on Arbitrum - * - whitelist deposit route from USDC on Optimism to USDC on Arbitrum - * @param depositOriginChainId Chain ID for one SpokePool that we want to enable as a deposit origin. - * @param depositDestinationChainId Chain ID that we want the newly whitelited deposit route to use as a - * destination. Will also be enabled for pool rebalances from this contract, since we'll likely need to - * send funds to this network to refund relayers. - * @param ethereumCounterpartToken Token on the current network that will be sent to the SpokePool on - * both `depositDestinationChainId` for pool rebalances. - * @param originToken Token that we want to enable for deposits on `depositOriginChainId` that can be fulfilled by - * `destinationToken` on `depositDestinationChainId`. Should be the `depositOriginChainId` counterpart of - * `ethereumCounterpartToken`. - * @param destinationToken Token that deposits from `depositOriginChainId` for the `originToken` should be sent - * to recipients on `depositDestinationChainId`. - * @param enable Set to True to set up pool rebalances to `depositDestinationChainId` and deposits from - * `depositOriginChainId` to `depositDestinationChainId`. Set to False to disable all of the above. If - * this value is false, the pool rebalance destination token will be set to 0x0 and the deposit route will be - * disabled on the SpokePools. - */ - function setDepositAndPoolRebalanceRoute( - uint256 depositOriginChainId, - uint256 depositDestinationChainId, - address ethereumCounterpartToken, - address originToken, - address destinationToken, - bool enable - ) public override nonReentrant onlyOwner { - _setDepositAndPoolRebalanceRoute( - depositOriginChainId, - depositDestinationChainId, - ethereumCounterpartToken, - originToken, - destinationToken, - enable - ); - } - - /** - * @notice Convenience method that whitelists (or unwhitelists) a pool rebalance route to two SpokePools and also - * whitelists deposits between the two spoke pools. Callable only by owner. For example, the admin might want - * to enable two-way USDC on Optimism <> USDC on Arbitrum relays. In practice, the admin would also need to - * enable pool rebalances from this contract to Optimism and Arbitrum for USDC. So the admin would like to execute - * four transactions atomically: - * - whitelist pool rebalance route from USDC on Ethereum to USDC on Optimism - * - whitelist pool rebalance route from USDC on Ethereum to USDC on Arbitrum - * - whitelist deposit route from USDC on Optimism to USDC on Arbitrum - * - whitelist deposit route from USDC on Arbitrum to USDC on Optimism - * @param depositRouteChainId_1 Chain ID for one SpokePool that we want to enable as a deposit origin and - * destination. Pool rebalances to this chain will also be enabled. - * @param depositRouteChainId_2 The other chain in addition to `depositRouteChainId_1` that we want to enable - * as a deposit origin and destination, and make available for pool rebalances. - * @param ethereumCounterpartToken Token on the current network that will be sent to the SpokePool on - * both `depositRouteChainId_1` and `depositRouteChainId_2` for pool rebalances. - * @param l2Token_1 Token that we want to enable for deposits on `depositRouteChainId_1` that can be fulfilled by - * `l2Token_2` on `depositRouteChainId_2`. Should be the chainId_1 counterpart of `ethereumCounterpartToken`. - * @param l2Token_2 Token that we want to enable for deposits on `depositRouteChainId_2` that can be fulfilled by - * `l2Token_1` on `depositRouteChainId_1`. Should be the chainId_2 counterpart of `ethereumCounterpartToken`. - * @param enable Set to True to set up pool rebalances to `depositRouteChainId_1` and `depositRouteChainId_2` and - * deposits from `depositRouteChainId_1` to `depositRouteChainId_2`. Set to False to disable all of the above. If - * this value is false, the pool rebalance destination tokens will be set to 0x0 and the deposit routes will be - * disabled on the SpokePools. - */ - function setDepositAndPoolRebalanceBiDirectionRoute( - uint256 depositRouteChainId_1, - uint256 depositRouteChainId_2, - address ethereumCounterpartToken, - address l2Token_1, - address l2Token_2, - bool enable - ) public override nonReentrant onlyOwner { - _setDepositAndPoolRebalanceRoute( - depositRouteChainId_2, - depositRouteChainId_1, - ethereumCounterpartToken, - l2Token_2, - l2Token_1, - enable - ); - _setDepositAndPoolRebalanceRoute( - depositRouteChainId_1, - depositRouteChainId_2, - ethereumCounterpartToken, - l2Token_1, - l2Token_2, - enable - ); + emit SetEnableDepositRoute(originChainId, destinationChainId, originToken, depositsEnabled); } /** @@ -771,6 +677,8 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { // 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; + // Note: if any of the keccak256(l1Tokens, chainId) are not mapped to a destination token address, then this + // internal method will revert. _sendTokensToChainAndUpdatePooledTokenTrackers( adapter, spokePool, @@ -926,20 +834,20 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { } /** - * @notice Conveniently queries which destination token is mapped to the hash of an origin token + + * @notice Conveniently queries which destination token is mapped to the hash of an l1 token + * destination chain ID for use in pool rebalances. * @param destinationChainId Where pool rebalance sends funds. - * @param originToken Rebalance token. + * @param l1Token Ethereum version token. * @return destinationToken address SpokePool can receive this token on destination chain following pool * rebalance. */ - function poolRebalanceRoute(uint256 destinationChainId, address originToken) + function poolRebalanceRoute(uint256 destinationChainId, address l1Token) external view override returns (address destinationToken) { - return poolRebalanceRoutes[_poolRebalanceRouteKey(originToken, destinationChainId)]; + return poolRebalanceRoutes[_poolRebalanceRouteKey(l1Token, destinationChainId)]; } /** @@ -952,41 +860,6 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { * INTERNAL FUNCTIONS * *************************************************/ - function _setDepositAndPoolRebalanceRoute( - uint256 depositOriginChainId, - uint256 depositDestinationChainId, - address ethereumCounterpartToken, - address originToken, - address destinationToken, - bool enable - ) internal { - poolRebalanceRoutes[_poolRebalanceRouteKey(ethereumCounterpartToken, depositDestinationChainId)] = ( - enable ? destinationToken : address(0) - ); - _relaySpokePoolAdminFunction( - depositOriginChainId, - abi.encodeWithSignature( - "setEnableRoute(address,address,uint256,bool)", - originToken, - destinationToken, - depositDestinationChainId, - enable - ) - ); - emit SetPoolRebalanceRoute( - depositDestinationChainId, - ethereumCounterpartToken, - enable ? destinationToken : address(0) - ); - emit SetEnableDepositRoute( - depositOriginChainId, - depositDestinationChainId, - originToken, - destinationToken, - enable - ); - } - // Called when a dispute fails due to parameter changes. This effectively resets the state and cancels the request // with no loss of funds, thereby enabling a new bundle to be added. function _cancelBundle(bytes memory ancillaryData) internal { @@ -1168,8 +1041,8 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { emit SpokePoolAdminFunctionTriggered(chainId, functionData); } - function _poolRebalanceRouteKey(address originToken, uint256 destinationChainId) internal pure returns (bytes32) { - return keccak256(abi.encode(originToken, destinationChainId)); + function _poolRebalanceRouteKey(address l1Token, uint256 destinationChainId) internal pure returns (bytes32) { + return keccak256(abi.encode(l1Token, destinationChainId)); } function _activeRequest() internal view returns (bool) { diff --git a/contracts/HubPoolInterface.sol b/contracts/HubPoolInterface.sol index 810921955..3d6400455 100644 --- a/contracts/HubPoolInterface.sol +++ b/contracts/HubPoolInterface.sol @@ -108,7 +108,7 @@ interface HubPoolInterface { function setPoolRebalanceRoute( uint256 destinationChainId, - address originToken, + address l1Token, address destinationToken ) external; @@ -116,29 +116,10 @@ interface HubPoolInterface { uint256 originChainId, uint256 destinationChainId, address originToken, - address destinationToken, bool depositsEnabled ) external; - function setDepositAndPoolRebalanceRoute( - uint256 depositOriginChainId, - uint256 depositDestinationChainId, - address ethereumCounterpartToken, - address originToken, - address destinationToken, - bool enable - ) external; - - function setDepositAndPoolRebalanceBiDirectionRoute( - uint256 depositRouteChainId_1, - uint256 depositRouteChainId_2, - address ethereumCounterpartToken, - address l2Token_1, - address l2Token_2, - bool enable - ) external; - - function poolRebalanceRoute(uint256 destinationChainId, address originToken) + function poolRebalanceRoute(uint256 destinationChainId, address l1Token) external view returns (address destinationToken); diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 40fbcec2c..63b094606 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -48,12 +48,8 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall // Count of deposits is used to construct a unique deposit identifier for this spoke pool. uint32 public numberOfDeposits; - struct DepositDestinationToken { - address destinationToken; - bool enabled; - } // Origin token to destination token routings can be turned on or off, which can enable or disable deposits. - mapping(address => mapping(uint256 => DepositDestinationToken)) public enabledDepositRoutes; + 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 // by "data workers" via inclusion proofs to execute leaves in the roots. @@ -80,12 +76,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall ****************************************/ event SetXDomainAdmin(address indexed newAdmin); event SetHubPool(address indexed newHubPool); - event EnabledDepositRoute( - uint256 indexed destinationChainId, - address indexed originToken, - address indexed destinationToken, - bool enabled - ); + event EnabledDepositRoute(uint256 indexed destinationChainId, address indexed originToken, bool enabled); event SetDepositQuoteTimeBuffer(uint32 newBuffer); event FundsDeposited( uint256 amount, @@ -95,7 +86,6 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall uint32 indexed depositId, uint32 quoteTimestamp, address indexed originToken, - address destinationToken, address recipient, address indexed depositor ); @@ -194,21 +184,16 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall /** * @notice Enable/Disable an origin token => destination chain ID route for deposits. Callable by admin only. * @param originToken Token that depositor can deposit to this contract. - * @param destinationToken Token that recipient on destination chain receives. * @param destinationChainId Chain ID for where depositor wants to receive funds. * @param enabled True to enable deposits, False otherwise. */ function setEnableRoute( address originToken, - address destinationToken, uint256 destinationChainId, bool enabled ) public override onlyAdmin { - enabledDepositRoutes[originToken][destinationChainId] = DepositDestinationToken({ - destinationToken: destinationToken, - enabled: enabled - }); - emit EnabledDepositRoute(destinationChainId, originToken, destinationToken, enabled); + enabledDepositRoutes[originToken][destinationChainId] = enabled; + emit EnabledDepositRoute(destinationChainId, originToken, enabled); } /** @@ -257,7 +242,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall * tokens in this contract and receive a destination token on the destination chain. The origin => destination * token mapping is stored on the L1 HubPool. * @notice The caller must first approve this contract to spend amount of originToken. - * @notice The originToken => destinationChainId must be enabled and the destination token must be set. + * @notice The originToken => destinationChainId must be enabled. * @notice This method is payable because the caller is able to deposit ETH if the originToken is WETH and this * function will handle wrapping ETH. * @param recipient Address to receive funds at on destination chain. @@ -276,9 +261,8 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall uint64 relayerFeePct, uint32 quoteTimestamp ) public payable override nonReentrant { - // Check that deposit route is initialized. - DepositDestinationToken memory destinationToken = enabledDepositRoutes[originToken][destinationChainId]; - require(destinationToken.destinationToken != address(0) && destinationToken.enabled, "Disabled route"); + // 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"); @@ -302,11 +286,6 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall // In this case the msg.value will be set to 0, indicating a "normal" ERC20 bridging action. } else IERC20(originToken).safeTransferFrom(msg.sender, address(this), amount); - // Note: Off-chain relayers are expected to send to the recipient `destinationToken` on the destination chain, - // but there is a chance that the destination token is set incorrectly or cannot be sent for other reasons. In - // these cases, we expect that the UMIP explains how to handle such cases. For example, a rule could be: - // "If the destination token address doesn't exist on the destination chain, then wait to relay the deposit - // until the destination token address is reset, or refund the user their token on the origin chain". _emitDeposit( amount, chainId(), @@ -315,7 +294,6 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall numberOfDeposits, quoteTimestamp, originToken, - destinationToken.destinationToken, recipient, msg.sender ); @@ -840,7 +818,6 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall uint32 depositId, uint32 quoteTimestamp, address originToken, - address destinationToken, address recipient, address depositor ) internal { @@ -852,7 +829,6 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall depositId, quoteTimestamp, originToken, - destinationToken, recipient, depositor ); diff --git a/contracts/SpokePoolInterface.sol b/contracts/SpokePoolInterface.sol index eeb22fec3..5f49b0ca8 100644 --- a/contracts/SpokePoolInterface.sol +++ b/contracts/SpokePoolInterface.sol @@ -54,7 +54,6 @@ interface SpokePoolInterface { function setEnableRoute( address originToken, - address destinationToken, uint256 destinationChainId, bool enable ) external; diff --git a/test/HubPool.Admin.ts b/test/HubPool.Admin.ts index 87f24bdee..bd6b35d6f 100644 --- a/test/HubPool.Admin.ts +++ b/test/HubPool.Admin.ts @@ -60,7 +60,6 @@ describe("HubPool Admin functions", function () { it("Only owner can relay spoke pool admin message", async function () { const functionData = mockSpoke.interface.encodeFunctionData("setEnableRoute", [ weth.address, - randomAddress(), destinationChainId, false, ]); @@ -82,16 +81,15 @@ describe("HubPool Admin functions", function () { .to.emit(hubPool, "SetPoolRebalanceRoute") .withArgs(destinationChainId, weth.address, usdc.address); - // Relay deposit mapping to spoke pool. Check content of messages sent to mock spoke pool. - await expect( - hubPool.connect(other).setDepositRoute(originChainId, destinationChainId, weth.address, usdc.address, true) - ).to.be.reverted; - await expect(hubPool.setDepositRoute(originChainId, destinationChainId, weth.address, usdc.address, true)) + // 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, usdc.address, true); + .withArgs(originChainId, destinationChainId, weth.address, true); // Disable deposit route on SpokePool right after: - await hubPool.setDepositRoute(originChainId, destinationChainId, weth.address, usdc.address, false); + 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); @@ -101,7 +99,6 @@ describe("HubPool Admin functions", function () { expect(relayMessageEvents[relayMessageEvents.length - 1].args?.message).to.equal( mockSpoke.interface.encodeFunctionData("setEnableRoute", [ weth.address, - usdc.address, destinationChainId, false, // Latest call disabled the route ]) @@ -109,163 +106,12 @@ describe("HubPool Admin functions", function () { expect(relayMessageEvents[relayMessageEvents.length - 2].args?.message).to.equal( mockSpoke.interface.encodeFunctionData("setEnableRoute", [ weth.address, - usdc.address, destinationChainId, true, // Second to last call enabled the route ]) ); }); - it("Only owner can whitelist deposits and rebalances atomically", async function () { - await hubPool.setCrossChainContracts(destinationChainId, mockAdapter.address, mockSpoke.address); - await hubPool.setCrossChainContracts(originChainId, mockAdapter.address, mockSpoke.address); - - const wethOnOriginChain = randomAddress(); - const wethOnDestinationChain = randomAddress(); - await expect( - hubPool - .connect(other) - .setDepositAndPoolRebalanceBiDirectionRoute( - originChainId, - destinationChainId, - weth.address, - wethOnOriginChain, - wethOnDestinationChain, - true - ) - ).to.be.reverted; - await hubPool.setDepositAndPoolRebalanceBiDirectionRoute( - originChainId, - destinationChainId, - weth.address, - wethOnOriginChain, - wethOnDestinationChain, - true - ); - - // Check pool rebalance routes - expect(await hubPool.poolRebalanceRoute(originChainId, weth.address)).to.equal(wethOnOriginChain); - expect(await hubPool.poolRebalanceRoute(destinationChainId, weth.address)).to.equal(wethOnDestinationChain); - - // Check content of messages relayed to spoke pools - const mockAdapterAtHubPool = mockAdapter.attach(hubPool.address); - let relayMessageEvents = await mockAdapterAtHubPool.queryFilter(mockAdapterAtHubPool.filters.RelayMessageCalled()); - expect(relayMessageEvents[relayMessageEvents.length - 2].args?.message).to.equal( - mockSpoke.interface.encodeFunctionData("setEnableRoute", [ - wethOnDestinationChain, - wethOnOriginChain, - originChainId, - true, - ]) - ); - expect(relayMessageEvents[relayMessageEvents.length - 1].args?.message).to.equal( - mockSpoke.interface.encodeFunctionData("setEnableRoute", [ - wethOnOriginChain, - wethOnDestinationChain, - destinationChainId, - true, - ]) - ); - - // Check that HubPool emitted four events. - const poolRebalanceRouteEvents = await hubPool.queryFilter(hubPool.filters.SetPoolRebalanceRoute()); - expect(poolRebalanceRouteEvents[poolRebalanceRouteEvents.length - 1].args?.destinationChainId).to.equal( - destinationChainId - ); - expect(poolRebalanceRouteEvents[poolRebalanceRouteEvents.length - 1].args?.originToken).to.equal(weth.address); - expect(poolRebalanceRouteEvents[poolRebalanceRouteEvents.length - 1].args?.destinationToken).to.equal( - wethOnDestinationChain - ); - expect(poolRebalanceRouteEvents[poolRebalanceRouteEvents.length - 2].args?.destinationChainId).to.equal( - originChainId - ); - expect(poolRebalanceRouteEvents[poolRebalanceRouteEvents.length - 2].args?.originToken).to.equal(weth.address); - expect(poolRebalanceRouteEvents[poolRebalanceRouteEvents.length - 2].args?.destinationToken).to.equal( - wethOnOriginChain - ); - const depositRouteEvents = await hubPool.queryFilter(hubPool.filters.SetEnableDepositRoute()); - expect(depositRouteEvents[depositRouteEvents.length - 2].args?.originChainId).to.equal(destinationChainId); - expect(depositRouteEvents[depositRouteEvents.length - 2].args?.destinationChainId).to.equal(originChainId); - expect(depositRouteEvents[depositRouteEvents.length - 2].args?.originToken).to.equal(wethOnDestinationChain); - expect(depositRouteEvents[depositRouteEvents.length - 2].args?.destinationToken).to.equal(wethOnOriginChain); - expect(depositRouteEvents[depositRouteEvents.length - 2].args?.depositsEnabled).to.equal(true); - expect(depositRouteEvents[depositRouteEvents.length - 1].args?.originChainId).to.equal(originChainId); - expect(depositRouteEvents[depositRouteEvents.length - 1].args?.destinationChainId).to.equal(destinationChainId); - expect(depositRouteEvents[depositRouteEvents.length - 1].args?.originToken).to.equal(wethOnOriginChain); - expect(depositRouteEvents[depositRouteEvents.length - 1].args?.destinationToken).to.equal(wethOnDestinationChain); - expect(depositRouteEvents[depositRouteEvents.length - 1].args?.depositsEnabled).to.equal(true); - - // Now disable the routes and check that pool rebalance destination tokens are zeroed out and relayed message - // correctly disables the deposit routes. - await hubPool.setDepositAndPoolRebalanceBiDirectionRoute( - originChainId, - destinationChainId, - weth.address, - wethOnOriginChain, - wethOnDestinationChain, - false - ); - expect(await hubPool.poolRebalanceRoute(originChainId, weth.address)).to.equal(ZERO_ADDRESS); - expect(await hubPool.poolRebalanceRoute(destinationChainId, weth.address)).to.equal(ZERO_ADDRESS); - relayMessageEvents = await mockAdapterAtHubPool.queryFilter(mockAdapterAtHubPool.filters.RelayMessageCalled()); - expect(relayMessageEvents[relayMessageEvents.length - 2].args?.message).to.equal( - mockSpoke.interface.encodeFunctionData("setEnableRoute", [ - wethOnDestinationChain, - wethOnOriginChain, - originChainId, - false, - ]) - ); - expect(relayMessageEvents[relayMessageEvents.length - 1].args?.message).to.equal( - mockSpoke.interface.encodeFunctionData("setEnableRoute", [ - wethOnOriginChain, - wethOnDestinationChain, - destinationChainId, - false, - ]) - ); - - // Finally, re-enable the routes individually using the one-way helper methods. - await hubPool.setDepositAndPoolRebalanceRoute( - originChainId, - destinationChainId, - weth.address, - wethOnOriginChain, - wethOnDestinationChain, - true - ); - expect(await hubPool.poolRebalanceRoute(destinationChainId, weth.address)).to.equal(wethOnDestinationChain); - expect(await hubPool.poolRebalanceRoute(originChainId, weth.address)).to.equal(ZERO_ADDRESS); - relayMessageEvents = await mockAdapterAtHubPool.queryFilter(mockAdapterAtHubPool.filters.RelayMessageCalled()); - expect(relayMessageEvents[relayMessageEvents.length - 1].args?.message).to.equal( - mockSpoke.interface.encodeFunctionData("setEnableRoute", [ - wethOnOriginChain, - wethOnDestinationChain, - destinationChainId, - true, - ]) - ); - await hubPool.setDepositAndPoolRebalanceRoute( - destinationChainId, - originChainId, - weth.address, - wethOnDestinationChain, - wethOnOriginChain, - true - ); - expect(await hubPool.poolRebalanceRoute(destinationChainId, weth.address)).to.equal(wethOnDestinationChain); - expect(await hubPool.poolRebalanceRoute(originChainId, weth.address)).to.equal(wethOnOriginChain); - relayMessageEvents = await mockAdapterAtHubPool.queryFilter(mockAdapterAtHubPool.filters.RelayMessageCalled()); - expect(relayMessageEvents[relayMessageEvents.length - 1].args?.message).to.equal( - mockSpoke.interface.encodeFunctionData("setEnableRoute", [ - wethOnDestinationChain, - wethOnOriginChain, - originChainId, - true, - ]) - ); - }); - it("Can change the bond token and amount", async function () { expect(await hubPool.callStatic.bondToken()).to.equal(weth.address); // Default set in the fixture. expect(await hubPool.callStatic.bondAmount()).to.equal(bondAmount.add(finalFee)); // Default set in the fixture. diff --git a/test/SpokePool.Admin.ts b/test/SpokePool.Admin.ts index eda3adf1f..cbe7e2ab0 100644 --- a/test/SpokePool.Admin.ts +++ b/test/SpokePool.Admin.ts @@ -11,13 +11,10 @@ describe("SpokePool Admin Functions", async function () { ({ spokePool, erc20 } = await spokePoolFixture()); }); it("Enable token path", async function () { - const destToken = randomAddress(); - await expect(spokePool.connect(owner).setEnableRoute(erc20.address, destToken, destinationChainId, true)) + await expect(spokePool.connect(owner).setEnableRoute(erc20.address, destinationChainId, true)) .to.emit(spokePool, "EnabledDepositRoute") - .withArgs(destinationChainId, erc20.address, destToken, true); - const destTokenStruct = await spokePool.enabledDepositRoutes(erc20.address, destinationChainId); - expect(destTokenStruct.enabled).to.equal(true); - expect(destTokenStruct.destinationToken).to.equal(destToken); + .withArgs(destinationChainId, erc20.address, true); + expect(await spokePool.enabledDepositRoutes(erc20.address, destinationChainId)).to.equal(true); }); it("Change deposit quote buffer", async function () { await expect(spokePool.connect(owner).setDepositQuoteTimeBuffer(60)) diff --git a/test/SpokePool.Deposit.ts b/test/SpokePool.Deposit.ts index 7c73dfb85..a58b12ee7 100644 --- a/test/SpokePool.Deposit.ts +++ b/test/SpokePool.Deposit.ts @@ -19,10 +19,7 @@ describe("SpokePool Depositor Logic", async function () { await weth.connect(depositor).approve(spokePool.address, amountToDeposit); // Whitelist origin token => destination chain ID routes: - await enableRoutes(spokePool, [ - { originToken: erc20.address, destinationToken: weth.address }, - { originToken: weth.address, destinationToken: erc20.address }, - ]); + await enableRoutes(spokePool, [{ originToken: erc20.address }, { originToken: weth.address }]); }); it("Depositing ERC20 tokens correctly pulls tokens and changes contract state", async function () { const currentSpokePoolTime = await spokePool.getCurrentTime(); @@ -49,7 +46,6 @@ describe("SpokePool Depositor Logic", async function () { 0, currentSpokePoolTime, erc20.address, - weth.address, recipient.address, depositor.address ); @@ -157,23 +153,7 @@ describe("SpokePool Depositor Logic", async function () { ).to.be.reverted; // Cannot deposit disabled route. - await spokePool.connect(depositor).setEnableRoute(erc20.address, weth.address, destinationChainId, false); - await expect( - spokePool - .connect(depositor) - .deposit( - ...getDepositParams( - recipient.address, - erc20.address, - amountToDeposit, - destinationChainId, - depositRelayerFeePct, - currentSpokePoolTime - ) - ) - ).to.be.reverted; - // Re-enable route but "forget" to set destination token address - await spokePool.connect(depositor).setEnableRoute(erc20.address, ZERO_ADDRESS, destinationChainId, true); + await spokePool.connect(depositor).setEnableRoute(erc20.address, destinationChainId, false); await expect( spokePool .connect(depositor) @@ -190,7 +170,7 @@ describe("SpokePool Depositor Logic", async function () { ).to.be.reverted; // Re-enable route and demonstrate that call would work. - await spokePool.connect(depositor).setEnableRoute(erc20.address, weth.address, destinationChainId, true); + await spokePool.connect(depositor).setEnableRoute(erc20.address, destinationChainId, true); await expect( spokePool .connect(depositor) diff --git a/test/SpokePool.SlowRelay.ts b/test/SpokePool.SlowRelay.ts index 10d6b4840..c749059e2 100644 --- a/test/SpokePool.SlowRelay.ts +++ b/test/SpokePool.SlowRelay.ts @@ -38,10 +38,7 @@ describe("SpokePool Slow Relay Logic", async function () { await weth.connect(relayer).approve(spokePool.address, fullRelayAmountPostFees); // Whitelist origin token => destination chain ID routes: - await enableRoutes(spokePool, [ - { originToken: erc20.address, destinationToken: randomAddress() }, - { originToken: weth.address, destinationToken: randomAddress() }, - ]); + await enableRoutes(spokePool, [{ originToken: erc20.address }, { originToken: weth.address }]); relays = []; for (let i = 0; i < 99; i++) { diff --git a/test/chain-specific-spokepools/Arbitrum_SpokePool.ts b/test/chain-specific-spokepools/Arbitrum_SpokePool.ts index 2bb42bd26..4bb2ae294 100644 --- a/test/chain-specific-spokepools/Arbitrum_SpokePool.ts +++ b/test/chain-specific-spokepools/Arbitrum_SpokePool.ts @@ -3,6 +3,7 @@ import { ethers, expect, Contract, FakeContract, SignerWithAddress, createFake, import { getContractFactory, seedContract, avmL1ToL2Alias, hre, toBN, toBNWei } from "../utils"; import { hubPoolFixture } from "../fixtures/HubPool.Fixture"; import { constructSingleRelayerRefundTree } from "../MerkleLib.utils"; +import { ZERO_ADDRESS } from "@uma/common"; let hubPool: Contract, arbitrumSpokePool: Contract, timer: Contract, dai: Contract, weth: Contract; let l2Weth: string, l2Dai: string, crossDomainAliasAddress; @@ -28,6 +29,7 @@ describe("Arbitrum Spoke Pool", function () { ).deploy(l2GatewayRouter.address, owner.address, hubPool.address, l2Weth, timer.address); await seedContract(arbitrumSpokePool, relayer, [dai], weth, amountHeldByPool); + await arbitrumSpokePool.connect(crossDomainAlias).whitelistToken(l2Dai, dai.address); }); it("Only cross domain owner can set L2GatewayRouter", async function () { @@ -37,11 +39,15 @@ describe("Arbitrum Spoke Pool", function () { }); it("Only cross domain owner can enable a route", async function () { - await expect(arbitrumSpokePool.setEnableRoute(l2Dai, dai.address, 1, true)).to.be.reverted; - await arbitrumSpokePool.connect(crossDomainAlias).setEnableRoute(l2Dai, dai.address, 1, true); - const destinationTokenStruct = await arbitrumSpokePool.enabledDepositRoutes(l2Dai, 1); - expect(destinationTokenStruct.enabled).to.equal(true); - expect(destinationTokenStruct.destinationToken).to.equal(dai.address); + await expect(arbitrumSpokePool.setEnableRoute(l2Dai, 1, true)).to.be.reverted; + await arbitrumSpokePool.connect(crossDomainAlias).setEnableRoute(l2Dai, 1, true); + expect(await arbitrumSpokePool.enabledDepositRoutes(l2Dai, 1)).to.equal(true); + }); + + it("Only cross domain owner can whitelist a token pair", async function () { + await expect(arbitrumSpokePool.whitelistToken(l2Dai, dai.address)).to.be.reverted; + await arbitrumSpokePool.connect(crossDomainAlias).whitelistToken(l2Dai, dai.address); + expect(await arbitrumSpokePool.whitelistedTokens(l2Dai)).to.equal(dai.address); }); it("Only cross domain owner can set the cross domain admin", async function () { @@ -82,11 +88,12 @@ describe("Arbitrum Spoke Pool", function () { await arbitrumSpokePool.connect(crossDomainAlias).relayRootBundle(tree.getHexRoot(), mockTreeRoot); // Reverts if route from arbitrum to mainnet for l2Dai isn't whitelisted. + await arbitrumSpokePool.connect(crossDomainAlias).whitelistToken(l2Dai, ZERO_ADDRESS); await expect( arbitrumSpokePool.executeRelayerRefundRoot(0, leafs[0], tree.getHexProof(leafs[0])) ).to.be.revertedWith("Uninitialized mainnet token"); + await arbitrumSpokePool.connect(crossDomainAlias).whitelistToken(l2Dai, dai.address); - await arbitrumSpokePool.connect(crossDomainAlias).setEnableRoute(l2Dai, dai.address, 1, true); await arbitrumSpokePool.connect(relayer).executeRelayerRefundRoot(0, leafs[0], tree.getHexProof(leafs[0])); // This should have sent tokens back to L1. Check the correct methods on the gateway are correctly called. diff --git a/test/chain-specific-spokepools/Ethereum_SpokePool.ts b/test/chain-specific-spokepools/Ethereum_SpokePool.ts index 22c75c272..7693c27be 100644 --- a/test/chain-specific-spokepools/Ethereum_SpokePool.ts +++ b/test/chain-specific-spokepools/Ethereum_SpokePool.ts @@ -4,14 +4,14 @@ import { getContractFactory, seedContract } from "../utils"; import { hubPoolFixture } from "../fixtures/HubPool.Fixture"; import { constructSingleRelayerRefundTree } from "../MerkleLib.utils"; -let hubPool: Contract, spokePool: Contract, timer: Contract, dai: Contract, weth: Contract, l2Dai: string; +let hubPool: Contract, spokePool: Contract, timer: Contract, dai: Contract, weth: Contract; let owner: SignerWithAddress, relayer: SignerWithAddress, rando: SignerWithAddress; describe("Ethereum Spoke Pool", function () { beforeEach(async function () { [owner, relayer, rando] = await ethers.getSigners(); - ({ weth, dai, hubPool, timer, l2Dai } = await hubPoolFixture()); + ({ weth, dai, hubPool, timer } = await hubPoolFixture()); spokePool = await ( await getContractFactory("Ethereum_SpokePool", owner) @@ -29,11 +29,9 @@ describe("Ethereum Spoke Pool", function () { }); it("Only owner can enable a route", async function () { - await expect(spokePool.connect(rando).setEnableRoute(l2Dai, dai.address, 1, true)).to.be.reverted; - await spokePool.connect(owner).setEnableRoute(l2Dai, dai.address, 1, true); - const destinationTokenStruct = await spokePool.enabledDepositRoutes(l2Dai, 1); - expect(destinationTokenStruct.enabled).to.equal(true); - expect(destinationTokenStruct.destinationToken).to.equal(dai.address); + await expect(spokePool.connect(rando).setEnableRoute(dai.address, 1, true)).to.be.reverted; + await spokePool.connect(owner).setEnableRoute(dai.address, 1, true); + expect(await spokePool.enabledDepositRoutes(dai.address, 1)).to.equal(true); }); it("Only owner can set the hub pool address", async function () { diff --git a/test/chain-specific-spokepools/Optimism_SpokePool.ts b/test/chain-specific-spokepools/Optimism_SpokePool.ts index 064de86ed..51d1f246e 100644 --- a/test/chain-specific-spokepools/Optimism_SpokePool.ts +++ b/test/chain-specific-spokepools/Optimism_SpokePool.ts @@ -49,12 +49,10 @@ describe("Optimism Spoke Pool", function () { }); it("Only cross domain owner can enable a route", async function () { - await expect(optimismSpokePool.setEnableRoute(l2Dai, dai.address, 1, true)).to.be.reverted; + await expect(optimismSpokePool.setEnableRoute(l2Dai, 1, true)).to.be.reverted; crossDomainMessenger.xDomainMessageSender.returns(owner.address); - await optimismSpokePool.connect(crossDomainMessenger.wallet).setEnableRoute(l2Dai, dai.address, 1, true); - const destinationTokenStruct = await optimismSpokePool.enabledDepositRoutes(l2Dai, 1); - expect(destinationTokenStruct.enabled).to.equal(true); - expect(destinationTokenStruct.destinationToken).to.equal(dai.address); + await optimismSpokePool.connect(crossDomainMessenger.wallet).setEnableRoute(l2Dai, 1, true); + expect(await optimismSpokePool.enabledDepositRoutes(l2Dai, 1)).to.equal(true); }); it("Only cross domain owner can set the cross domain admin", async function () { diff --git a/test/chain-specific-spokepools/Polygon_SpokePool.ts b/test/chain-specific-spokepools/Polygon_SpokePool.ts index a97e1abdb..a22f2544d 100644 --- a/test/chain-specific-spokepools/Polygon_SpokePool.ts +++ b/test/chain-specific-spokepools/Polygon_SpokePool.ts @@ -60,12 +60,7 @@ describe("Polygon Spoke Pool", function () { }); it("Only correct caller can enable a route", async function () { - const setEnableRouteData = polygonSpokePool.interface.encodeFunctionData("setEnableRoute", [ - l2Dai, - dai.address, - 1, - true, - ]); + const setEnableRouteData = polygonSpokePool.interface.encodeFunctionData("setEnableRoute", [l2Dai, 1, true]); // Wrong rootMessageSender address. await expect(polygonSpokePool.connect(fxChild).processMessageFromRoot(0, rando.address, setEnableRouteData)).to.be @@ -76,9 +71,7 @@ describe("Polygon Spoke Pool", function () { .reverted; await polygonSpokePool.connect(fxChild).processMessageFromRoot(0, owner.address, setEnableRouteData); - const destinationTokenStruct = await polygonSpokePool.enabledDepositRoutes(l2Dai, 1); - expect(destinationTokenStruct.enabled).to.equal(true); - expect(destinationTokenStruct.destinationToken).to.equal(dai.address); + expect(await polygonSpokePool.enabledDepositRoutes(l2Dai, 1)).to.equal(true); }); it("Only correct caller can set the quote time buffer", async function () { diff --git a/test/fixtures/SpokePool.Fixture.ts b/test/fixtures/SpokePool.Fixture.ts index 618cdd26d..d6d8ce3f0 100644 --- a/test/fixtures/SpokePool.Fixture.ts +++ b/test/fixtures/SpokePool.Fixture.ts @@ -44,7 +44,6 @@ export async function deploySpokePool(ethers: any): Promise<{ export interface DepositRoute { originToken: string; - destinationToken: string; destinationChainId?: number; enabled?: boolean; } @@ -52,7 +51,6 @@ export async function enableRoutes(spokePool: Contract, routes: DepositRoute[]) for (const route of routes) { await spokePool.setEnableRoute( route.originToken, - route.destinationToken, route.destinationChainId ? route.destinationChainId : consts.destinationChainId, route.enabled !== undefined ? route.enabled : true ); diff --git a/test/gas-analytics/utils.ts b/test/gas-analytics/utils.ts index 485f89b1c..55922aeb7 100644 --- a/test/gas-analytics/utils.ts +++ b/test/gas-analytics/utils.ts @@ -1,4 +1,4 @@ -import { SignerWithAddress, getContractFactory, BigNumber, toBN, Contract, randomAddress } from "../utils"; +import { SignerWithAddress, getContractFactory, BigNumber, toBN, Contract } from "../utils"; import { TokenRolesEnum } from "@uma/common"; import * as consts from "../constants"; import { getDepositParams, getRelayHash, getFillRelayParams, enableRoutes } from "../fixtures/SpokePool.Fixture"; @@ -63,7 +63,6 @@ export async function warmSpokePool( await enableRoutes(_spokePool, [ { originToken: _currencyAddress, - destinationToken: randomAddress(), destinationChainId: 1, }, ]); From d257d982766524bd1fbce1b76bf32a0f8c4a45d6 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 16 Mar 2022 17:05:54 -0400 Subject: [PATCH 11/20] Update HubPool.sol --- contracts/HubPool.sol | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index c90dbf01b..0a96a9fe9 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -678,7 +678,10 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { // then the delegate call should revert. address adapter = crossChainContracts[chainId].adapter; // Note: if any of the keccak256(l1Tokens, chainId) are not mapped to a destination token address, then this - // internal method will revert. + // 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, From 2282c9658d3a2d08e04246128a3c30e352f37405 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 16 Mar 2022 17:07:33 -0400 Subject: [PATCH 12/20] Update Arbitrum_SpokePool.sol --- contracts/Arbitrum_SpokePool.sol | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/contracts/Arbitrum_SpokePool.sol b/contracts/Arbitrum_SpokePool.sol index f887431a4..5e6dd64d4 100644 --- a/contracts/Arbitrum_SpokePool.sol +++ b/contracts/Arbitrum_SpokePool.sol @@ -80,8 +80,7 @@ contract Arbitrum_SpokePool is SpokePool { // Check that the Ethereum counterpart of the L2 token is stored on this contract. require(whitelistedTokens[relayerRefundLeaf.l2TokenAddress] != address(0), "Uninitialized mainnet token"); StandardBridgeLike(l2GatewayRouter).outboundTransfer( - whitelistedTokens[relayerRefundLeaf.l2TokenAddress], // _l1Token. Address of the L1 token to bridge over., // _l1Token. Address of the - // L1 token to bridge over. + whitelistedTokens[relayerRefundLeaf.l2TokenAddress], // _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. From f7c95ffd7ba172940b41392f471b334625a614ce Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 16 Mar 2022 17:07:54 -0400 Subject: [PATCH 13/20] Update Arbitrum_SpokePool.sol --- contracts/Arbitrum_SpokePool.sol | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/contracts/Arbitrum_SpokePool.sol b/contracts/Arbitrum_SpokePool.sol index 5e6dd64d4..ef4111172 100644 --- a/contracts/Arbitrum_SpokePool.sol +++ b/contracts/Arbitrum_SpokePool.sol @@ -88,16 +88,16 @@ contract Arbitrum_SpokePool is SpokePool { emit ArbitrumTokensBridged(address(0), hubPool, relayerRefundLeaf.amountToReturn); } - function _whitelistToken(address _l2Token, address _l1Token) internal { - whitelistedTokens[_l2Token] = _l1Token; - emit WhitelistedTokens(_l2Token, _l1Token); - } - function _setL2GatewayRouter(address _l2GatewayRouter) internal { l2GatewayRouter = _l2GatewayRouter; emit SetL2GatewayRouter(l2GatewayRouter); } + function _whitelistToken(address _l2Token, address _l1Token) internal { + whitelistedTokens[_l2Token] = _l1Token; + emit WhitelistedTokens(_l2Token, _l1Token); + } + // L1 addresses are transformed during l1->l2 calls. // See https://developer.offchainlabs.com/docs/l1_l2_messages#address-aliasing for more information. // This cannot be pulled directly from Arbitrum contracts because their contracts are not 0.8.X compatible and From 9bdff276b850b27fa213176c029e4a4f545cbf20 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 16 Mar 2022 17:08:34 -0400 Subject: [PATCH 14/20] Update Arbitrum_SpokePool.sol --- contracts/Arbitrum_SpokePool.sol | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/contracts/Arbitrum_SpokePool.sol b/contracts/Arbitrum_SpokePool.sol index ef4111172..5a0ef4a97 100644 --- a/contracts/Arbitrum_SpokePool.sol +++ b/contracts/Arbitrum_SpokePool.sol @@ -78,9 +78,10 @@ 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. - require(whitelistedTokens[relayerRefundLeaf.l2TokenAddress] != address(0), "Uninitialized mainnet token"); + 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. From 38978da2b569ada38c1b7cf03244c0d54165eaf4 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 16 Mar 2022 17:17:00 -0400 Subject: [PATCH 15/20] fix --- contracts/HubPool.sol | 30 ++++++++++++++++++------------ test/SpokePool.Deposit.ts | 1 - 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index 61d32248f..766fddbaa 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -82,7 +82,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { // 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. + // to 0x0 to disable a pool rebalance route and block executeRootBundle() from executing. mapping(bytes32 => address) private poolRebalanceRoutes; struct PooledToken { @@ -384,12 +384,13 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { } /** - * @notice Store which destination token we need to bridge to SpokePool on origin chain. Callable only by owner. - * @dev Admin can set destinationToken to effectively disable pool rebalances for this l1 token + destination - * chain ID combination. - * @param destinationChainId Chain where pool rebalance sends tokens to. - * @param l1Token Token sent from this pool. - * @param destinationToken Token received at destination chain. Destination chain counterpart to l1Token. + * @notice Store canonical destination token counterpart for l1 token. Callable only by owner. + * @dev Admin can set destinationToken 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 setPoolRebalanceRoute( uint256 destinationChainId, @@ -403,6 +404,11 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { /** * @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. @@ -673,11 +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) 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. + // 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, diff --git a/test/SpokePool.Deposit.ts b/test/SpokePool.Deposit.ts index a58b12ee7..3488832a7 100644 --- a/test/SpokePool.Deposit.ts +++ b/test/SpokePool.Deposit.ts @@ -1,7 +1,6 @@ import { expect, ethers, Contract, SignerWithAddress, seedWallet, toBN, toWei, randomAddress } from "./utils"; import { spokePoolFixture, enableRoutes, getDepositParams } from "./fixtures/SpokePool.Fixture"; import { amountToSeedWallets, amountToDeposit, destinationChainId, depositRelayerFeePct } from "./constants"; -import { ZERO_ADDRESS } from "@uma/common"; let spokePool: Contract, weth: Contract, erc20: Contract, unwhitelistedErc20: Contract; let depositor: SignerWithAddress, recipient: SignerWithAddress; From f5a8e7451ee581253d5d2789042f5de16918cb06 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 16 Mar 2022 20:28:10 -0400 Subject: [PATCH 16/20] Update HubPool.ExecuteRootBundle.ts --- test/HubPool.ExecuteRootBundle.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/HubPool.ExecuteRootBundle.ts b/test/HubPool.ExecuteRootBundle.ts index 839089132..8087a5aae 100644 --- a/test/HubPool.ExecuteRootBundle.ts +++ b/test/HubPool.ExecuteRootBundle.ts @@ -1,4 +1,4 @@ -import { toBNWei, SignerWithAddress, seedWallet, expect, Contract, ethers } from "./utils"; +import { toBNWei, SignerWithAddress, seedWallet, expect, Contract, ethers, randomAddress } from "./utils"; import * as consts from "./constants"; import { hubPoolFixture, enableTokensForLP } from "./fixtures/HubPool.Fixture"; import { buildPoolRebalanceLeafTree, buildPoolRebalanceLeafs } from "./MerkleLib.utils"; From 8ed603c7f2d74b7cb0bc67cfe4611100fce8c6a0 Mon Sep 17 00:00:00 2001 From: chrismaree Date: Thu, 17 Mar 2022 11:38:45 +0200 Subject: [PATCH 17/20] release Signed-off-by: chrismaree --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index a4c047768..3bddda9a5 100644 --- a/package.json +++ b/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": { From 62d85bb25ed6167b0e18964a104ef56003ff48e8 Mon Sep 17 00:00:00 2001 From: chrismaree Date: Thu, 17 Mar 2022 11:46:35 +0200 Subject: [PATCH 18/20] comment nit Signed-off-by: chrismaree --- contracts/HubPool.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index 766fddbaa..6d6e79b96 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -402,8 +402,8 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { } /** - * @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. + * @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 From fb3f7a4d881e4e5065a185876ef6bb0e0e6585eb Mon Sep 17 00:00:00 2001 From: chrismaree Date: Thu, 17 Mar 2022 11:52:55 +0200 Subject: [PATCH 19/20] comment nit Signed-off-by: chrismaree --- contracts/HubPool.sol | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index 6d6e79b96..3a3092f9c 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -367,7 +367,7 @@ 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. @@ -839,12 +839,11 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { } /** - * @notice Conveniently queries which destination token is mapped to the hash of an l1 token + - * destination chain ID for use in pool rebalances. + * @notice Conveniently queries which destination token is mapped to the hash of an l1 token + destination chain ID + * for use in pool rebalances. * @param destinationChainId Where pool rebalance sends funds. * @param l1Token Ethereum version token. - * @return destinationToken address SpokePool can receive this token on destination chain following pool - * rebalance. + * @return destinationToken address SpokePool can receive this token on destination chain following pool rebalance. */ function poolRebalanceRoute(uint256 destinationChainId, address l1Token) external From 2d0adf78647070e4dd20690f67f46daaa6fc82c4 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Thu, 17 Mar 2022 09:16:18 -0400 Subject: [PATCH 20/20] Final fix --- contracts/HubPool.sol | 19 ++++++++++--------- contracts/SpokePool.sol | 4 ++-- test/SpokePool.Admin.ts | 4 ++-- test/SpokePool.Deposit.ts | 2 +- 4 files changed, 15 insertions(+), 14 deletions(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index 3a3092f9c..4b6ca3922 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -77,7 +77,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { // Whether the bundle proposal process is paused. bool public paused; - // Stores paths from L1 token to destination ID + token. Since different tokens on L1 might map to + // 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 @@ -371,7 +371,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { * 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( @@ -385,8 +385,8 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { /** * @notice Store canonical destination token counterpart for l1 token. Callable only by owner. - * @dev Admin can set destinationToken to effectively disable executing any root bundles with leaves containing - * this l1 token + destination chain ID combination. + * @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. @@ -682,7 +682,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { // 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 + // 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, @@ -839,11 +839,12 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { } /** - * @notice Conveniently queries which destination token is mapped to the hash of an l1 token + destination chain ID - * for use in pool rebalances. - * @param destinationChainId Where pool rebalance sends funds. + * @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 SpokePool can receive this token on destination chain following pool rebalance. + * @return destinationToken address The destination token that is sent to spoke pools after this contract bridges + * the l1Token to the destination chain. */ function poolRebalanceRoute(uint256 destinationChainId, address l1Token) external diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 63b094606..b2f5f9375 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -76,7 +76,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall ****************************************/ event SetXDomainAdmin(address indexed newAdmin); event SetHubPool(address indexed newHubPool); - event EnabledDepositRoute(uint256 indexed destinationChainId, address indexed originToken, bool enabled); + event EnabledDepositRoute(address indexed originToken, uint256 indexed destinationChainId, bool enabled); event SetDepositQuoteTimeBuffer(uint32 newBuffer); event FundsDeposited( uint256 amount, @@ -193,7 +193,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall bool enabled ) public override onlyAdmin { enabledDepositRoutes[originToken][destinationChainId] = enabled; - emit EnabledDepositRoute(destinationChainId, originToken, enabled); + emit EnabledDepositRoute(originToken, destinationChainId, enabled); } /** diff --git a/test/SpokePool.Admin.ts b/test/SpokePool.Admin.ts index cbe7e2ab0..f9e2126ad 100644 --- a/test/SpokePool.Admin.ts +++ b/test/SpokePool.Admin.ts @@ -1,4 +1,4 @@ -import { expect, ethers, Contract, SignerWithAddress, randomAddress } from "./utils"; +import { expect, ethers, Contract, SignerWithAddress } from "./utils"; import { spokePoolFixture } from "./fixtures/SpokePool.Fixture"; import { destinationChainId, mockRelayerRefundRoot, mockSlowRelayRoot } from "./constants"; @@ -13,7 +13,7 @@ describe("SpokePool Admin Functions", async function () { it("Enable token path", async function () { await expect(spokePool.connect(owner).setEnableRoute(erc20.address, destinationChainId, true)) .to.emit(spokePool, "EnabledDepositRoute") - .withArgs(destinationChainId, erc20.address, true); + .withArgs(erc20.address, destinationChainId, true); expect(await spokePool.enabledDepositRoutes(erc20.address, destinationChainId)).to.equal(true); }); it("Change deposit quote buffer", async function () { diff --git a/test/SpokePool.Deposit.ts b/test/SpokePool.Deposit.ts index 3488832a7..0f38464a5 100644 --- a/test/SpokePool.Deposit.ts +++ b/test/SpokePool.Deposit.ts @@ -1,4 +1,4 @@ -import { expect, ethers, Contract, SignerWithAddress, seedWallet, toBN, toWei, randomAddress } from "./utils"; +import { expect, ethers, Contract, SignerWithAddress, seedWallet, toBN, toWei } from "./utils"; import { spokePoolFixture, enableRoutes, getDepositParams } from "./fixtures/SpokePool.Fixture"; import { amountToSeedWallets, amountToDeposit, destinationChainId, depositRelayerFeePct } from "./constants";