diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index c970e0fd2..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,12 +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). 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. - address spokePool = crossChainContracts[chainId].spokePool; - require(spokePool != address(0), "Uninitialized spoke pool"); + // 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); @@ -659,9 +658,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, @@ -1007,9 +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"); + (address adapter, address spokePool) = _getInitializedCrossChainContracts(chainId); // Perform delegatecall to use the adapter's code with this contract's context. (bool success, ) = adapter.delegatecall( @@ -1023,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 107ac60be..acd15f0d4 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 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.reverted; + await expect(hubPool.relaySpokePoolAdminFunction(destinationChainId, functionData)).to.be.revertedWith( + "SpokePool not initialized" + ); + await hubPool.setCrossChainContracts(destinationChainId, randomAddress(), mockSpoke.address); + await expect(hubPool.relaySpokePoolAdminFunction(destinationChainId, functionData)).to.be.revertedWith( + "Adapter not initialized" + ); 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..9cb2fd265 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"; @@ -163,7 +163,24 @@ 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 () { + const { leafs, tree } = await constructSimpleTree(); + + await hubPool + .connect(dataWorker) + .proposeRootBundle([3117, 3118], 2, tree.getHexRoot(), consts.mockRelayerRefundRoot, consts.mockSlowRelayRoot); + + // 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("Adapter not initialized"); }); it("Execution rejects leaf claim before liveness passed", async function () {