Skip to content

fix: [L02] Re-add checks that adapter is not 0x0 #113

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 16, 2022
Merged

fix: [L02] Re-add checks that adapter is not 0x0 #113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b0f02d6
fix: Re-add checks that adapter is not 0x0
nicholaspai Mar 16, 2022
d327e19
refactor
nicholaspai Mar 16, 2022
4c49288
Update HubPool.ExecuteRootBundle.ts
nicholaspai Mar 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 19 additions & 14 deletions contracts/HubPool.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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);
Expand All @@ -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,
Expand Down Expand Up @@ -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(
Expand All @@ -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,
Expand Down
10 changes: 8 additions & 2 deletions test/HubPool.Admin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
21 changes: 19 additions & 2 deletions test/HubPool.ExecuteRootBundle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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 () {
Expand Down