From b0f02d610fea346efd7e1c49a5b19e28b09327d2 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 16 Mar 2022 15:17:39 -0400 Subject: [PATCH 1/3] fix: Re-add checks that adapter is not 0x0 --- contracts/HubPool.sol | 11 +++++------ test/HubPool.Admin.ts | 10 ++++++++-- test/HubPool.ExecuteRootBundle.ts | 17 +++++++++++++++++ 3 files changed, 30 insertions(+), 8 deletions(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index c970e0fd2..dd44ce4a4 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -646,11 +646,12 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { ); // Make sure SpokePool address is initialized since _sendTokensToChainAndUpdatePooledTokenTrackers() will not - // revert if its accidentally set to address(0). We don't make the same check on the adapter for this - // chainId because the internal method's delegatecall() to the adapter will revert if its address is set - // incorrectly. + // revert if its accidentally set to address(0). Also check that adapter is not 0x0 since delegatecall to the + // 0x0 address will not revert. address spokePool = crossChainContracts[chainId].spokePool; + address adapter = crossChainContracts[chainId].adapter; require(spokePool != address(0), "Uninitialized spoke pool"); + require(adapter != address(0), "Uninitialized adapter"); // Set the leafId in the claimed bitmap. rootBundleProposal.claimedBitMap = MerkleLib.setClaimed1D(rootBundleProposal.claimedBitMap, leafId); @@ -659,9 +660,6 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { rootBundleProposal.unclaimedPoolRebalanceLeafCount--; // Relay each L1 token to destination chain. - // Note: We don't check that the adapter is initialized since if its the zero address or invalid otherwise, - // then the delegate call should revert. - address adapter = crossChainContracts[chainId].adapter; _sendTokensToChainAndUpdatePooledTokenTrackers( adapter, spokePool, @@ -1010,6 +1008,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { address adapter = crossChainContracts[chainId].adapter; address spokePool = crossChainContracts[chainId].spokePool; require(spokePool != address(0), "SpokePool not initialized"); + require(adapter != address(0), "Adapter not initialized"); // Perform delegatecall to use the adapter's code with this contract's context. (bool success, ) = adapter.delegatecall( diff --git a/test/HubPool.Admin.ts b/test/HubPool.Admin.ts index 107ac60be..43b40def2 100644 --- a/test/HubPool.Admin.ts +++ b/test/HubPool.Admin.ts @@ -65,9 +65,15 @@ describe("HubPool Admin functions", function () { ]); await expect(hubPool.connect(other).relaySpokePoolAdminFunction(destinationChainId, functionData)).to.be.reverted; - // Cannot relay admin function if spoke pool is set to zero address. + // Cannot relay admin function if spoke pool or adapter is set to zero address. await hubPool.setCrossChainContracts(destinationChainId, mockAdapter.address, ZERO_ADDRESS); - await expect(hubPool.relaySpokePoolAdminFunction(destinationChainId, functionData)).to.be.reverted; + await expect(hubPool.relaySpokePoolAdminFunction(destinationChainId, functionData)).to.be.revertedWith( + "SpokePool not initialized" + ); + await hubPool.setCrossChainContracts(destinationChainId, ZERO_ADDRESS, mockSpoke.address); + await expect(hubPool.relaySpokePoolAdminFunction(destinationChainId, functionData)).to.be.revertedWith( + "delegatecall failed" + ); await hubPool.setCrossChainContracts(destinationChainId, mockAdapter.address, mockSpoke.address); await expect(hubPool.relaySpokePoolAdminFunction(destinationChainId, functionData)) .to.emit(hubPool, "SpokePoolAdminFunctionTriggered") diff --git a/test/HubPool.ExecuteRootBundle.ts b/test/HubPool.ExecuteRootBundle.ts index cfd40be1a..fbff74a1f 100644 --- a/test/HubPool.ExecuteRootBundle.ts +++ b/test/HubPool.ExecuteRootBundle.ts @@ -166,6 +166,23 @@ describe("HubPool Root Bundle Execution", function () { ).to.be.revertedWith("Uninitialized spoke pool"); }); + it("Reverts if adapter not set for chain ID", async function () { + const { leafs, tree } = await constructSimpleTree(); + + await hubPool + .connect(dataWorker) + .proposeRootBundle([3117, 3118], 2, tree.getHexRoot(), consts.mockRelayerRefundRoot, consts.mockSlowRelayRoot); + + // Set adapter to address 0x0 + await hubPool.setCrossChainContracts(consts.repaymentChainId, ZERO_ADDRESS, mockSpoke.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); + await expect( + hubPool.connect(dataWorker).executeRootBundle(...Object.values(leafs[0]), tree.getHexProof(leafs[0])) + ).to.be.revertedWith("delegatecall failed"); + }); + it("Execution rejects leaf claim before liveness passed", async function () { const { leafs, tree } = await constructSimpleTree(); await hubPool From d327e1925b9cba8a804448862038a003cd18746a Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 16 Mar 2022 15:36:13 -0400 Subject: [PATCH 2/3] refactor --- contracts/HubPool.sol | 32 ++++++++++++++++++------------- test/HubPool.Admin.ts | 6 +++--- test/HubPool.ExecuteRootBundle.ts | 8 ++++---- 3 files changed, 26 insertions(+), 20 deletions(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index dd44ce4a4..2f37be743 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -360,9 +360,11 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { /** * @notice Sets cross chain relay helper contracts for L2 chain ID. Callable only by owner. + * @dev We do not block setting the adapter or spokepool to invalid/zero addresses because we want to allow the + * 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. - * @param spokePool Recipient of relayed messages and tokens on SpokePool. + * @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. */ function setCrossChainContracts( @@ -645,13 +647,9 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { "Bad Proof" ); - // Make sure SpokePool address is initialized since _sendTokensToChainAndUpdatePooledTokenTrackers() will not - // revert if its accidentally set to address(0). Also check that adapter is not 0x0 since delegatecall to the - // 0x0 address will not revert. - address spokePool = crossChainContracts[chainId].spokePool; - address adapter = crossChainContracts[chainId].adapter; - require(spokePool != address(0), "Uninitialized spoke pool"); - require(adapter != address(0), "Uninitialized adapter"); + // Get cross chain helpers for leaf's destination chain ID. This internal method will revert if either helper + // is set improperly. + (address adapter, address spokePool) = _getInitializedCrossChainContracts(chainId); // Set the leafId in the claimed bitmap. rootBundleProposal.claimedBitMap = MerkleLib.setClaimed1D(rootBundleProposal.claimedBitMap, leafId); @@ -1005,10 +1003,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { } function _relaySpokePoolAdminFunction(uint256 chainId, bytes memory functionData) internal { - address adapter = crossChainContracts[chainId].adapter; - address spokePool = crossChainContracts[chainId].spokePool; - require(spokePool != address(0), "SpokePool not initialized"); - require(adapter != address(0), "Adapter not initialized"); + (address adapter, address spokePool) = _getInitializedCrossChainContracts(chainId); // Perform delegatecall to use the adapter's code with this contract's context. (bool success, ) = adapter.delegatecall( @@ -1022,6 +1017,17 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { emit SpokePoolAdminFunctionTriggered(chainId, functionData); } + function _getInitializedCrossChainContracts(uint256 chainId) + internal + view + returns (address adapter, address spokePool) + { + adapter = crossChainContracts[chainId].adapter; + spokePool = crossChainContracts[chainId].spokePool; + require(spokePool != address(0), "SpokePool not initialized"); + require(adapter.isContract(), "Adapter not initialized"); + } + function _whitelistedRouteKey( uint256 originChainId, address originToken, diff --git a/test/HubPool.Admin.ts b/test/HubPool.Admin.ts index 43b40def2..acd15f0d4 100644 --- a/test/HubPool.Admin.ts +++ b/test/HubPool.Admin.ts @@ -65,14 +65,14 @@ describe("HubPool Admin functions", function () { ]); await expect(hubPool.connect(other).relaySpokePoolAdminFunction(destinationChainId, functionData)).to.be.reverted; - // Cannot relay admin function if spoke pool or adapter is set to zero address. + // Cannot relay admin function if spoke pool is set to zero address or adapter is set to non contract.. await hubPool.setCrossChainContracts(destinationChainId, mockAdapter.address, ZERO_ADDRESS); await expect(hubPool.relaySpokePoolAdminFunction(destinationChainId, functionData)).to.be.revertedWith( "SpokePool not initialized" ); - await hubPool.setCrossChainContracts(destinationChainId, ZERO_ADDRESS, mockSpoke.address); + await hubPool.setCrossChainContracts(destinationChainId, randomAddress(), mockSpoke.address); await expect(hubPool.relaySpokePoolAdminFunction(destinationChainId, functionData)).to.be.revertedWith( - "delegatecall failed" + "Adapter not initialized" ); await hubPool.setCrossChainContracts(destinationChainId, mockAdapter.address, mockSpoke.address); await expect(hubPool.relaySpokePoolAdminFunction(destinationChainId, functionData)) diff --git a/test/HubPool.ExecuteRootBundle.ts b/test/HubPool.ExecuteRootBundle.ts index fbff74a1f..a43d2f3fa 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"; @@ -173,14 +173,14 @@ describe("HubPool Root Bundle Execution", function () { .connect(dataWorker) .proposeRootBundle([3117, 3118], 2, tree.getHexRoot(), consts.mockRelayerRefundRoot, consts.mockSlowRelayRoot); - // Set adapter to address 0x0 - await hubPool.setCrossChainContracts(consts.repaymentChainId, ZERO_ADDRESS, mockSpoke.address); + // Set adapter to random address. + await hubPool.setCrossChainContracts(consts.repaymentChainId, randomAddress(), mockSpoke.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); await expect( hubPool.connect(dataWorker).executeRootBundle(...Object.values(leafs[0]), tree.getHexProof(leafs[0])) - ).to.be.revertedWith("delegatecall failed"); + ).to.be.revertedWith("Adapter not initialized"); }); it("Execution rejects leaf claim before liveness passed", async function () { From 4c4928866149dcec5bd6008c5ac8050f30898b7f Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 16 Mar 2022 15:38:51 -0400 Subject: [PATCH 3/3] 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 a43d2f3fa..9cb2fd265 100644 --- a/test/HubPool.ExecuteRootBundle.ts +++ b/test/HubPool.ExecuteRootBundle.ts @@ -163,7 +163,7 @@ describe("HubPool Root Bundle Execution", function () { 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("Uninitialized spoke pool"); + ).to.be.revertedWith("SpokePool not initialized"); }); it("Reverts if adapter not set for chain ID", async function () {