From db4e757fdd3f0bcb902c65ef402b8ba1c6e728a1 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Fri, 11 Mar 2022 16:32:45 -0500 Subject: [PATCH 1/9] fix(hub-pool): Do not relay root bundle to same spoke pool more than once --- contracts/HubPool.sol | 9 ++- test/HubPool.ExecuteRootBundle.ts | 102 +++++++++++++++++++++--------- 2 files changed, 81 insertions(+), 30 deletions(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index c10d7fb4a..4af5ce85f 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -60,6 +60,8 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { uint256 claimedBitMap; // Proposer of this root bundle. address proposer; + // Keeps track of which SpokePools we have already relayed roots to. + mapping(uint256 => bool) relayedRootToSpokePool; // Number of pool rebalance leaves to execute in the poolRebalanceRoot. After this number // of leaves are executed, a new root bundle can be proposed uint8 unclaimedPoolRebalanceLeafCount; @@ -649,7 +651,12 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { rootBundleProposal.unclaimedPoolRebalanceLeafCount--; _sendTokensToChainAndUpdatePooledTokenTrackers(spokePool, chainId, l1Tokens, netSendAmounts, bundleLpFees); - _relayRootBundleToSpokePool(spokePool, chainId); + + // Do not relay same roots to SpokePool more than once. + if (!rootBundleProposal.relayedRootToSpokePool[chainId]) { + rootBundleProposal.relayedRootToSpokePool[chainId] = true; + _relayRootBundleToSpokePool(spokePool, chainId); + } // Transfer the bondAmount to back to the proposer, if this the last executed leaf. Only sending this once all // leafs have been executed acts to force the data worker to execute all bundles or they wont receive their bond. diff --git a/test/HubPool.ExecuteRootBundle.ts b/test/HubPool.ExecuteRootBundle.ts index e69c9d47f..8f58550df 100644 --- a/test/HubPool.ExecuteRootBundle.ts +++ b/test/HubPool.ExecuteRootBundle.ts @@ -14,18 +14,18 @@ async function constructSimpleTree() { const wethToSendToL2 = toBNWei(100); const daiToSend = toBNWei(1000); const leafs = buildPoolRebalanceLeafs( - [consts.repaymentChainId], // repayment chain. In this test we only want to send one token to one chain. - [[weth.address, dai.address]], // l1Token. We will only be sending WETH and DAI to the associated repayment chain. - [[toBNWei(1), toBNWei(10)]], // bundleLpFees. Set to 1 ETH and 10 DAI respectively to attribute to the LPs. - [[wethToSendToL2, daiToSend]], // netSendAmounts. Set to 100 ETH and 1000 DAI as the amount to send from L1->L2. - [[wethToSendToL2, daiToSend]] // runningBalances. Set to 100 ETH and 1000 DAI. + [consts.repaymentChainId, consts.repaymentChainId], // repayment chain. + [[weth.address, dai.address], []], // l1Token. We will only be sending WETH and DAI to the associated repayment chain. + [[toBNWei(1), toBNWei(10)], []], // bundleLpFees. Set to 1 ETH and 10 DAI respectively to attribute to the LPs. + [[wethToSendToL2, daiToSend], []], // netSendAmounts. Set to 100 ETH and 1000 DAI as the amount to send from L1->L2. + [[wethToSendToL2, daiToSend], []] // runningBalances. Set to 100 ETH and 1000 DAI. ); const tree = await buildPoolRebalanceLeafTree(leafs); return { wethToSendToL2, daiToSend, leafs, tree }; } -describe("HubPool Root Bundle Execution", function () { +describe.only("HubPool Root Bundle Execution", function () { beforeEach(async function () { [owner, dataWorker, liquidityProvider] = await ethers.getSigners(); ({ weth, dai, hubPool, mockAdapter, mockSpoke, timer, l2Weth, l2Dai } = await hubPoolFixture()); @@ -45,19 +45,22 @@ describe("HubPool Root Bundle Execution", function () { const { wethToSendToL2, daiToSend, 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). + [3117, 3118], // bundleEvaluationBlockNumbers used by bots to construct bundles. Length must equal the number of leafs. + 2, // poolRebalanceLeafCount. tree.getHexRoot(), // poolRebalanceRoot. Generated from the merkle tree constructed before. consts.mockRelayerRefundRoot, // Not relevant for this test. consts.mockSlowRelayRoot // Not relevant for this test. ); - // Advance time so the request can be executed and execute the request. + // Advance time so the request can be executed and execute first leaf. await timer.setCurrentTime(Number(await timer.getCurrentTime()) + consts.refundProposalLiveness + 1); await hubPool.connect(dataWorker).executeRootBundle(...Object.values(leafs[0]), tree.getHexProof(leafs[0])); - // Balances should have updated as expected. - expect(await weth.balanceOf(hubPool.address)).to.equal(consts.amountToLp.sub(wethToSendToL2)); + // Balances should have updated as expected. Note that pool should still have bond remaining since a leaf + // is unexecuted. + expect(await weth.balanceOf(hubPool.address)).to.equal( + consts.amountToLp.sub(wethToSendToL2).add(consts.bondAmount.add(consts.finalFee)) + ); expect(await weth.balanceOf(await mockAdapter.bridge())).to.equal(wethToSendToL2); expect(await dai.balanceOf(hubPool.address)).to.equal(consts.amountToLp.mul(10).sub(daiToSend)); expect(await dai.balanceOf(await mockAdapter.bridge())).to.equal(daiToSend); @@ -91,19 +94,64 @@ describe("HubPool Root Bundle Execution", function () { expect(relayTokensEvents[1].args?.to).to.equal(mockSpoke.address); // Check the leaf count was decremented correctly. - expect((await hubPool.rootBundleProposal()).unclaimedPoolRebalanceLeafCount).to.equal(0); + expect((await hubPool.rootBundleProposal()).unclaimedPoolRebalanceLeafCount).to.equal(1); }); - it("Reverts if spoke pool not set for chain ID", async function () { + it("Executing two leaves with the same chain ID does not relay root bundle to spoke pool twice", 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. + await hubPool + .connect(dataWorker) + .proposeRootBundle([3117, 3118], 2, tree.getHexRoot(), consts.mockRelayerRefundRoot, consts.mockSlowRelayRoot); + + // Advance time so the request can be executed and execute two leaves with same chain ID. + await timer.setCurrentTime(Number(await timer.getCurrentTime()) + consts.refundProposalLiveness + 1); + await hubPool.connect(dataWorker).executeRootBundle(...Object.values(leafs[0]), tree.getHexProof(leafs[0])); + await hubPool.connect(dataWorker).executeRootBundle(...Object.values(leafs[1]), tree.getHexProof(leafs[1])); + + // Since the mock adapter is delegatecalled, when querying, its address should be the hubPool address. + const mockAdapterAtHubPool = mockAdapter.attach(hubPool.address); + + // Check the mockAdapter was called with the correct arguments for each method. The event counts should be identical + // to the above test. + 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[relayMessageEvents.length - 1].args?.target).to.equal(mockSpoke.address); + expect(relayMessageEvents[relayMessageEvents.length - 1].args?.message).to.equal( + mockSpoke.interface.encodeFunctionData("relayRootBundle", [ + consts.mockRelayerRefundRoot, + consts.mockSlowRelayRoot, + ]) + ); + }); + + it("Executing all leaves returns bond to proposer", async function () { + const { leafs, tree } = await constructSimpleTree(); + + await hubPool + .connect(dataWorker) + .proposeRootBundle([3117, 3118], 2, tree.getHexRoot(), consts.mockRelayerRefundRoot, consts.mockSlowRelayRoot); + + // Advance time so the request can be executed and execute both leaves. + await timer.setCurrentTime(Number(await timer.getCurrentTime()) + consts.refundProposalLiveness + 1); + await hubPool.connect(dataWorker).executeRootBundle(...Object.values(leafs[0]), tree.getHexProof(leafs[0])); + + // Second execution sends bond back to data worker. + const bondAmount = consts.bondAmount.add(consts.finalFee); + expect( + await hubPool.connect(dataWorker).executeRootBundle(...Object.values(leafs[1]), tree.getHexProof(leafs[1])) + ).to.changeTokenBalances(weth, [dataWorker, hubPool], [bondAmount, bondAmount.mul(-1)]); + }); + + it("Reverts if spoke pool 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 spoke pool to address 0x0 await hubPool.setCrossChainContracts(consts.repaymentChainId, mockAdapter.address, ZERO_ADDRESS); @@ -119,7 +167,7 @@ describe("HubPool Root Bundle Execution", function () { const { leafs, tree } = await constructSimpleTree(); await hubPool .connect(dataWorker) - .proposeRootBundle([3117], 1, tree.getHexRoot(), consts.mockRelayerRefundRoot, consts.mockSlowRelayRoot); + .proposeRootBundle([3117, 3118], 2, tree.getHexRoot(), consts.mockRelayerRefundRoot, consts.mockSlowRelayRoot); // Set time 10 seconds before expiration. Should revert. await timer.setCurrentTime(Number(await timer.getCurrentTime()) + consts.refundProposalLiveness - 10); @@ -137,7 +185,7 @@ describe("HubPool Root Bundle Execution", function () { const { leafs, tree } = await constructSimpleTree(); await hubPool .connect(dataWorker) - .proposeRootBundle([3117], 1, tree.getHexRoot(), consts.mockRelayerRefundRoot, consts.mockSlowRelayRoot); + .proposeRootBundle([3117, 3118], 2, tree.getHexRoot(), consts.mockRelayerRefundRoot, consts.mockSlowRelayRoot); await timer.setCurrentTime(Number(await timer.getCurrentTime()) + consts.refundProposalLiveness + 1); // Take the valid root but change some element within it, such as the chainId. This will change the hash of the leaf @@ -152,7 +200,7 @@ describe("HubPool Root Bundle Execution", function () { const { leafs, tree } = await constructSimpleTree(); await hubPool .connect(dataWorker) - .proposeRootBundle([3117], 1, tree.getHexRoot(), consts.mockRelayerRefundRoot, consts.mockSlowRelayRoot); + .proposeRootBundle([3117, 3118], 2, tree.getHexRoot(), consts.mockRelayerRefundRoot, consts.mockSlowRelayRoot); await timer.setCurrentTime(Number(await timer.getCurrentTime()) + consts.refundProposalLiveness + 1); // First claim should be fine. Second claim should be reverted as you cant double claim a leaf. @@ -165,13 +213,9 @@ describe("HubPool Root Bundle Execution", function () { it("Cannot execute while paused", 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. - ); + await hubPool + .connect(dataWorker) + .proposeRootBundle([3117, 3118], 2, tree.getHexRoot(), consts.mockRelayerRefundRoot, consts.mockSlowRelayRoot); // Advance time so the request can be executed and execute the request. await timer.setCurrentTime(Number(await timer.getCurrentTime()) + consts.refundProposalLiveness + 1); From 30755fe892dcbc0fbe771c45618db33c81c7ac87 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Fri, 11 Mar 2022 16:45:13 -0500 Subject: [PATCH 2/9] Mark `relayToSpokePool` in PoolRebalanceLeaf --- contracts/HubPool.sol | 20 ++++++++++++++++---- contracts/HubPoolInterface.sol | 5 +++++ test/HubPool.ExecuteRootBundle.ts | 5 +++-- test/MerkleLib.Proofs.ts | 1 + test/MerkleLib.utils.ts | 8 ++++++-- test/gas-analytics/HubPool.RootExecution.ts | 3 ++- 6 files changed, 33 insertions(+), 9 deletions(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index 4af5ce85f..1b58603f0 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -196,6 +196,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { address indexed proposer ); event RootBundleExecuted( + bool relayToSpokePool, uint256 indexed leafId, uint256 indexed chainId, address[] l1Token, @@ -599,6 +600,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { * @param netSendAmounts Array representing the amount of tokens to send to the SpokePool on the target chainId. * @param runningBalances Array used to track any unsent tokens that are not included in the netSendAmounts. * @param leafId Index of this executed leaf within the poolRebalance tree. + * @param relayToSpokePool If True, then relay roots to SpokePool via cross chain bridge. * @param l1Tokens Array of all the tokens associated with the bundleLpFees, nedSendAmounts and runningBalances. * @param proof Inclusion proof for this leaf in pool rebalance root in root bundle. */ @@ -609,6 +611,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { int256[] memory netSendAmounts, int256[] memory runningBalances, uint8 leafId, + bool relayToSpokePool, address[] memory l1Tokens, bytes32[] memory proof ) public nonReentrant unpaused { @@ -627,6 +630,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { netSendAmounts: netSendAmounts, runningBalances: runningBalances, leafId: leafId, + relayToSpokePool: relayToSpokePool, l1Tokens: l1Tokens }), proof @@ -652,9 +656,8 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { _sendTokensToChainAndUpdatePooledTokenTrackers(spokePool, chainId, l1Tokens, netSendAmounts, bundleLpFees); - // Do not relay same roots to SpokePool more than once. - if (!rootBundleProposal.relayedRootToSpokePool[chainId]) { - rootBundleProposal.relayedRootToSpokePool[chainId] = true; + // Check bool used by data worker to prevent relaying redundant roots to SpokePool. + if (relayToSpokePool) { _relayRootBundleToSpokePool(spokePool, chainId); } @@ -663,7 +666,16 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { if (rootBundleProposal.unclaimedPoolRebalanceLeafCount == 0) bondToken.safeTransfer(rootBundleProposal.proposer, bondAmount); - emit RootBundleExecuted(leafId, chainId, l1Tokens, bundleLpFees, netSendAmounts, runningBalances, msg.sender); + emit RootBundleExecuted( + relayToSpokePool, + leafId, + chainId, + l1Tokens, + bundleLpFees, + netSendAmounts, + runningBalances, + msg.sender + ); } /** diff --git a/contracts/HubPoolInterface.sol b/contracts/HubPoolInterface.sol index a13f61193..db906cb41 100644 --- a/contracts/HubPoolInterface.sol +++ b/contracts/HubPoolInterface.sol @@ -26,6 +26,10 @@ interface HubPoolInterface { // A positive number indicates that the HubPool owes the SpokePool funds. A negative number indicates that the // SpokePool owes the HubPool funds. See the comment above for the dynamics of this and netSendAmounts int256[] runningBalances; + // Used by data worker to mark which leaves should relay roots to SpokePools. We assume that the data worker + // would not mark this `True` for leaves that share `chainId` otherwise a root would get sent twice to a + // SpokePool which would be exploited to steal funds from the SpokePool. + bool relayToSpokePool; // Used as the index in the bitmap to track whether this leaf has been executed or not. uint8 leafId; // The following arrays are required to be the same length. They are parallel arrays for the given chainId and @@ -96,6 +100,7 @@ interface HubPoolInterface { int256[] memory netSendAmounts, int256[] memory runningBalances, uint8 leafId, + bool relayToSpokePool, address[] memory l1Tokens, bytes32[] memory proof ) external; diff --git a/test/HubPool.ExecuteRootBundle.ts b/test/HubPool.ExecuteRootBundle.ts index 8f58550df..1e317c2f0 100644 --- a/test/HubPool.ExecuteRootBundle.ts +++ b/test/HubPool.ExecuteRootBundle.ts @@ -18,14 +18,15 @@ async function constructSimpleTree() { [[weth.address, dai.address], []], // l1Token. We will only be sending WETH and DAI to the associated repayment chain. [[toBNWei(1), toBNWei(10)], []], // bundleLpFees. Set to 1 ETH and 10 DAI respectively to attribute to the LPs. [[wethToSendToL2, daiToSend], []], // netSendAmounts. Set to 100 ETH and 1000 DAI as the amount to send from L1->L2. - [[wethToSendToL2, daiToSend], []] // runningBalances. Set to 100 ETH and 1000 DAI. + [[wethToSendToL2, daiToSend], []], // runningBalances. Set to 100 ETH and 1000 DAI. + [true, false] // relayToSpokePool. Second leaf should not relay roots to spoke pool. ); const tree = await buildPoolRebalanceLeafTree(leafs); return { wethToSendToL2, daiToSend, leafs, tree }; } -describe.only("HubPool Root Bundle Execution", function () { +describe("HubPool Root Bundle Execution", function () { beforeEach(async function () { [owner, dataWorker, liquidityProvider] = await ethers.getSigners(); ({ weth, dai, hubPool, mockAdapter, mockSpoke, timer, l2Weth, l2Dai } = await hubPoolFixture()); diff --git a/test/MerkleLib.Proofs.ts b/test/MerkleLib.Proofs.ts index 746eae236..0f6b953b8 100644 --- a/test/MerkleLib.Proofs.ts +++ b/test/MerkleLib.Proofs.ts @@ -33,6 +33,7 @@ describe("MerkleLib Proofs", async function () { bundleLpFees, netSendAmounts, runningBalances, + relayToSpokePool: true, }); } diff --git a/test/MerkleLib.utils.ts b/test/MerkleLib.utils.ts index 5424f6dfb..cda360f36 100644 --- a/test/MerkleLib.utils.ts +++ b/test/MerkleLib.utils.ts @@ -8,6 +8,7 @@ export interface PoolRebalanceLeaf { netSendAmounts: BigNumber[]; runningBalances: BigNumber[]; leafId: BigNumber; + relayToSpokePool: boolean; l1Tokens: string[]; } @@ -71,7 +72,8 @@ export function buildPoolRebalanceLeafs( l1Tokens: string[][], bundleLpFees: BigNumber[][], netSendAmounts: BigNumber[][], - runningBalances: BigNumber[][] + runningBalances: BigNumber[][], + relayToSpokePool: boolean[] ): PoolRebalanceLeaf[] { return Array(destinationChainIds.length) .fill(0) @@ -82,6 +84,7 @@ export function buildPoolRebalanceLeafs( netSendAmounts: netSendAmounts[i], runningBalances: runningBalances[i], leafId: BigNumber.from(i), + relayToSpokePool: relayToSpokePool[i], l1Tokens: l1Tokens[i], }; }); @@ -109,7 +112,8 @@ export async function constructSingleChainTree(token: string, scalingSize = 1, r [[token]], // l1Token. We will only be sending 1 token to one chain. [[realizedLpFees]], // bundleLpFees. [[tokensSendToL2]], // netSendAmounts. - [[tokensSendToL2]] // runningBalances. + [[tokensSendToL2]], // runningBalances. + [true] // relayToSpokePool ); const tree = await buildPoolRebalanceLeafTree(leafs); diff --git a/test/gas-analytics/HubPool.RootExecution.ts b/test/gas-analytics/HubPool.RootExecution.ts index 234fb29cd..2924678d7 100644 --- a/test/gas-analytics/HubPool.RootExecution.ts +++ b/test/gas-analytics/HubPool.RootExecution.ts @@ -46,7 +46,8 @@ async function constructSimpleTree(_destinationChainIds: number[], _l1Tokens: Co _l1TokenAddresses, _bundleLpFeeAmounts, _netSendAmounts, // netSendAmounts. - _netSendAmounts // runningBalances. + _netSendAmounts, // runningBalances. + Array(REFUND_CHAIN_COUNT).fill(true) // relayToSpokePool ); const tree = await buildPoolRebalanceLeafTree(leaves); From a33bc1753b43731c33a64751ccd2e0bc696c131f Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Fri, 11 Mar 2022 16:58:54 -0500 Subject: [PATCH 3/9] fix --- contracts/HubPool.sol | 2 -- scripts/buildSampleTree.ts | 1 + 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index 1b58603f0..54dafb488 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -60,8 +60,6 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { uint256 claimedBitMap; // Proposer of this root bundle. address proposer; - // Keeps track of which SpokePools we have already relayed roots to. - mapping(uint256 => bool) relayedRootToSpokePool; // Number of pool rebalance leaves to execute in the poolRebalanceRoot. After this number // of leaves are executed, a new root bundle can be proposed uint8 unclaimedPoolRebalanceLeafCount; diff --git a/scripts/buildSampleTree.ts b/scripts/buildSampleTree.ts index 3a3b17295..65e087501 100644 --- a/scripts/buildSampleTree.ts +++ b/scripts/buildSampleTree.ts @@ -43,6 +43,7 @@ async function main() { bundleLpFees: [toBNWei(0.1)], netSendAmounts: [toBNWei(POOL_REBALANCE_NET_SEND_AMOUNT)], runningBalances: [toWei(0)], + relayToSpokePool: true, leafId: toBN(i), l1Tokens: [L1_TOKEN], }); From d3c470940c8d3933d19cd1b35edb3d0846a39958 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Fri, 11 Mar 2022 17:03:58 -0500 Subject: [PATCH 4/9] fix gas test --- test/gas-analytics/HubPool.RootExecution.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/gas-analytics/HubPool.RootExecution.ts b/test/gas-analytics/HubPool.RootExecution.ts index 2924678d7..0fa094f02 100644 --- a/test/gas-analytics/HubPool.RootExecution.ts +++ b/test/gas-analytics/HubPool.RootExecution.ts @@ -204,7 +204,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 61f59b12928eb900606669d034dc98e82e16d012 Mon Sep 17 00:00:00 2001 From: nicholaspai <9457025+nicholaspai@users.noreply.github.com> Date: Sat, 12 Mar 2022 10:59:44 -0500 Subject: [PATCH 5/9] Update contracts/HubPoolInterface.sol Co-authored-by: Chris Maree --- contracts/HubPoolInterface.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/HubPoolInterface.sol b/contracts/HubPoolInterface.sol index db906cb41..38abddf1b 100644 --- a/contracts/HubPoolInterface.sol +++ b/contracts/HubPoolInterface.sol @@ -27,7 +27,7 @@ interface HubPoolInterface { // SpokePool owes the HubPool funds. See the comment above for the dynamics of this and netSendAmounts int256[] runningBalances; // Used by data worker to mark which leaves should relay roots to SpokePools. We assume that the data worker - // would not mark this `True` for leaves that share `chainId` otherwise a root would get sent twice to a + // would not mark this True for leaves that share chainId otherwise a root would get sent twice to a // SpokePool which would be exploited to steal funds from the SpokePool. bool relayToSpokePool; // Used as the index in the bitmap to track whether this leaf has been executed or not. From 20bfc2031cc415457a867b2674146a1819d3d61e Mon Sep 17 00:00:00 2001 From: nicholaspai <9457025+nicholaspai@users.noreply.github.com> Date: Sat, 12 Mar 2022 10:59:50 -0500 Subject: [PATCH 6/9] Update contracts/HubPool.sol Co-authored-by: Chris Maree --- contracts/HubPool.sol | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index 54dafb488..75143056e 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -655,9 +655,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { _sendTokensToChainAndUpdatePooledTokenTrackers(spokePool, chainId, l1Tokens, netSendAmounts, bundleLpFees); // Check bool used by data worker to prevent relaying redundant roots to SpokePool. - if (relayToSpokePool) { - _relayRootBundleToSpokePool(spokePool, chainId); - } + if (relayToSpokePool) _relayRootBundleToSpokePool(spokePool, chainId); // Transfer the bondAmount to back to the proposer, if this the last executed leaf. Only sending this once all // leafs have been executed acts to force the data worker to execute all bundles or they wont receive their bond. From 799b797e443e76319b42009ba5f8fa64b129b9b2 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Sat, 12 Mar 2022 11:14:50 -0500 Subject: [PATCH 7/9] Update HubPool.ExecuteRootBundle.ts --- test/HubPool.ExecuteRootBundle.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/HubPool.ExecuteRootBundle.ts b/test/HubPool.ExecuteRootBundle.ts index 1e317c2f0..b9547d18f 100644 --- a/test/HubPool.ExecuteRootBundle.ts +++ b/test/HubPool.ExecuteRootBundle.ts @@ -55,7 +55,9 @@ describe("HubPool Root Bundle Execution", function () { // Advance time so the request can be executed and execute first leaf. await timer.setCurrentTime(Number(await timer.getCurrentTime()) + consts.refundProposalLiveness + 1); - await hubPool.connect(dataWorker).executeRootBundle(...Object.values(leafs[0]), tree.getHexProof(leafs[0])); + expect( + await hubPool.connect(dataWorker).executeRootBundle(...Object.values(leafs[0]), tree.getHexProof(leafs[0])) + ).to.emit(hubPool, "RootBundleExecuted"); // Balances should have updated as expected. Note that pool should still have bond remaining since a leaf // is unexecuted. From eb4a4397d6e703a9b0862048bb4074b18526f1bc Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Tue, 15 Mar 2022 14:01:12 -0400 Subject: [PATCH 8/9] Use groupIndex instead of bool --- contracts/HubPool.sol | 13 +++++++------ contracts/HubPoolInterface.sol | 13 ++++++++----- scripts/buildSampleTree.ts | 2 +- test/HubPool.ExecuteRootBundle.ts | 2 +- test/MerkleLib.Proofs.ts | 2 +- test/MerkleLib.utils.ts | 8 ++++---- test/gas-analytics/HubPool.RootExecution.ts | 2 +- 7 files changed, 23 insertions(+), 19 deletions(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index 046a51302..42b7fd1bc 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -193,7 +193,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { address indexed proposer ); event RootBundleExecuted( - bool relayToSpokePool, + uint256 groupIndex, uint256 indexed leafId, uint256 indexed chainId, address[] l1Token, @@ -594,22 +594,23 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { * @dev In some cases, will instruct spokePool to send funds back to L1. * @notice Deletes the published root bundle if this is the last leaf to be executed in the root bundle. * @param chainId ChainId number of the target spoke pool on which the bundle is executed. + * @param groupIndex If set to 0, then relay roots to SpokePool via cross chain bridge. Used by off-chain validator + * to organize leafs with the same chain ID and also set which leaves should result in relayed messages. * @param bundleLpFees Array representing the total LP fee amount per token in this bundle for all bundled relays. * @param netSendAmounts Array representing the amount of tokens to send to the SpokePool on the target chainId. * @param runningBalances Array used to track any unsent tokens that are not included in the netSendAmounts. * @param leafId Index of this executed leaf within the poolRebalance tree. - * @param relayToSpokePool If True, then relay roots to SpokePool via cross chain bridge. * @param l1Tokens Array of all the tokens associated with the bundleLpFees, nedSendAmounts and runningBalances. * @param proof Inclusion proof for this leaf in pool rebalance root in root bundle. */ function executeRootBundle( uint256 chainId, + uint256 groupIndex, uint256[] memory bundleLpFees, int256[] memory netSendAmounts, int256[] memory runningBalances, uint8 leafId, - bool relayToSpokePool, address[] memory l1Tokens, bytes32[] memory proof ) public nonReentrant unpaused { @@ -624,11 +625,11 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { rootBundleProposal.poolRebalanceRoot, PoolRebalanceLeaf({ chainId: chainId, + groupIndex: groupIndex, bundleLpFees: bundleLpFees, netSendAmounts: netSendAmounts, runningBalances: runningBalances, leafId: leafId, - relayToSpokePool: relayToSpokePool, l1Tokens: l1Tokens }), proof @@ -663,7 +664,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { ); // Check bool used by data worker to prevent relaying redundant roots to SpokePool. - if (relayToSpokePool) { + if (groupIndex == 0) { // Relay root bundles to spoke pool on destination chain by // performing delegatecall to use the adapter's code with this contract's context. (bool success, ) = adapter.delegatecall( @@ -686,7 +687,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { bondToken.safeTransfer(rootBundleProposal.proposer, bondAmount); emit RootBundleExecuted( - relayToSpokePool, + groupIndex, leafId, chainId, l1Tokens, diff --git a/contracts/HubPoolInterface.sol b/contracts/HubPoolInterface.sol index 38abddf1b..692ac40d2 100644 --- a/contracts/HubPoolInterface.sol +++ b/contracts/HubPoolInterface.sol @@ -26,10 +26,13 @@ interface HubPoolInterface { // A positive number indicates that the HubPool owes the SpokePool funds. A negative number indicates that the // SpokePool owes the HubPool funds. See the comment above for the dynamics of this and netSendAmounts int256[] runningBalances; - // Used by data worker to mark which leaves should relay roots to SpokePools. We assume that the data worker - // would not mark this True for leaves that share chainId otherwise a root would get sent twice to a - // SpokePool which would be exploited to steal funds from the SpokePool. - bool relayToSpokePool; + // Used by data worker to mark which leaves should relay roots to SpokePools, and to otherwise organize leaves. + // For example, each leaf should contain all the rebalance information for a single chain, but in the case where + // the list of l1Tokens is very large such that they all can't fit into a single leaf that can be executed under + // the block gas limit, then the data worker can use this groupIndex to organize them. Any leaves with + // a groupIndex equal to 0 will relay roots to the SpokePool, so the data worker should ensure that only one + // leaf for a specific chainId should have a groupIndex equal to 0. + uint256 groupIndex; // Used as the index in the bitmap to track whether this leaf has been executed or not. uint8 leafId; // The following arrays are required to be the same length. They are parallel arrays for the given chainId and @@ -96,11 +99,11 @@ interface HubPoolInterface { function executeRootBundle( uint256 chainId, + uint256 groupIndex, uint256[] memory bundleLpFees, int256[] memory netSendAmounts, int256[] memory runningBalances, uint8 leafId, - bool relayToSpokePool, address[] memory l1Tokens, bytes32[] memory proof ) external; diff --git a/scripts/buildSampleTree.ts b/scripts/buildSampleTree.ts index 65e087501..0a7477010 100644 --- a/scripts/buildSampleTree.ts +++ b/scripts/buildSampleTree.ts @@ -43,7 +43,7 @@ async function main() { bundleLpFees: [toBNWei(0.1)], netSendAmounts: [toBNWei(POOL_REBALANCE_NET_SEND_AMOUNT)], runningBalances: [toWei(0)], - relayToSpokePool: true, + groupIndex: 0, leafId: toBN(i), l1Tokens: [L1_TOKEN], }); diff --git a/test/HubPool.ExecuteRootBundle.ts b/test/HubPool.ExecuteRootBundle.ts index b9547d18f..cfd40be1a 100644 --- a/test/HubPool.ExecuteRootBundle.ts +++ b/test/HubPool.ExecuteRootBundle.ts @@ -19,7 +19,7 @@ async function constructSimpleTree() { [[toBNWei(1), toBNWei(10)], []], // bundleLpFees. Set to 1 ETH and 10 DAI respectively to attribute to the LPs. [[wethToSendToL2, daiToSend], []], // netSendAmounts. Set to 100 ETH and 1000 DAI as the amount to send from L1->L2. [[wethToSendToL2, daiToSend], []], // runningBalances. Set to 100 ETH and 1000 DAI. - [true, false] // relayToSpokePool. Second leaf should not relay roots to spoke pool. + [0, 1] // groupIndex. Second leaf should not relay roots to spoke pool. ); const tree = await buildPoolRebalanceLeafTree(leafs); diff --git a/test/MerkleLib.Proofs.ts b/test/MerkleLib.Proofs.ts index 0f6b953b8..7c45c5636 100644 --- a/test/MerkleLib.Proofs.ts +++ b/test/MerkleLib.Proofs.ts @@ -33,7 +33,7 @@ describe("MerkleLib Proofs", async function () { bundleLpFees, netSendAmounts, runningBalances, - relayToSpokePool: true, + groupIndex: BigNumber.from(0), }); } diff --git a/test/MerkleLib.utils.ts b/test/MerkleLib.utils.ts index cda360f36..131698a24 100644 --- a/test/MerkleLib.utils.ts +++ b/test/MerkleLib.utils.ts @@ -4,11 +4,11 @@ import { MerkleTree } from "../utils/MerkleTree"; import { RelayData } from "./fixtures/SpokePool.Fixture"; export interface PoolRebalanceLeaf { chainId: BigNumber; + groupIndex: BigNumber; bundleLpFees: BigNumber[]; netSendAmounts: BigNumber[]; runningBalances: BigNumber[]; leafId: BigNumber; - relayToSpokePool: boolean; l1Tokens: string[]; } @@ -73,18 +73,18 @@ export function buildPoolRebalanceLeafs( bundleLpFees: BigNumber[][], netSendAmounts: BigNumber[][], runningBalances: BigNumber[][], - relayToSpokePool: boolean[] + groupIndex: number[] ): PoolRebalanceLeaf[] { return Array(destinationChainIds.length) .fill(0) .map((_, i) => { return { chainId: BigNumber.from(destinationChainIds[i]), + groupIndex: BigNumber.from(groupIndex[i]), bundleLpFees: bundleLpFees[i], netSendAmounts: netSendAmounts[i], runningBalances: runningBalances[i], leafId: BigNumber.from(i), - relayToSpokePool: relayToSpokePool[i], l1Tokens: l1Tokens[i], }; }); @@ -113,7 +113,7 @@ export async function constructSingleChainTree(token: string, scalingSize = 1, r [[realizedLpFees]], // bundleLpFees. [[tokensSendToL2]], // netSendAmounts. [[tokensSendToL2]], // runningBalances. - [true] // relayToSpokePool + [0] // groupIndex ); const tree = await buildPoolRebalanceLeafTree(leafs); diff --git a/test/gas-analytics/HubPool.RootExecution.ts b/test/gas-analytics/HubPool.RootExecution.ts index 0fa094f02..8463ca5d6 100644 --- a/test/gas-analytics/HubPool.RootExecution.ts +++ b/test/gas-analytics/HubPool.RootExecution.ts @@ -47,7 +47,7 @@ async function constructSimpleTree(_destinationChainIds: number[], _l1Tokens: Co _bundleLpFeeAmounts, _netSendAmounts, // netSendAmounts. _netSendAmounts, // runningBalances. - Array(REFUND_CHAIN_COUNT).fill(true) // relayToSpokePool + Array(REFUND_CHAIN_COUNT).fill(0) // relayToSpokePool ); const tree = await buildPoolRebalanceLeafTree(leaves); From 248bb4d67dfb195b7077f8632f548fa3db808be5 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Tue, 15 Mar 2022 14:50:28 -0400 Subject: [PATCH 9/9] Update buildSampleTree.ts --- scripts/buildSampleTree.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/buildSampleTree.ts b/scripts/buildSampleTree.ts index 0a7477010..75acc73a1 100644 --- a/scripts/buildSampleTree.ts +++ b/scripts/buildSampleTree.ts @@ -43,7 +43,7 @@ async function main() { bundleLpFees: [toBNWei(0.1)], netSendAmounts: [toBNWei(POOL_REBALANCE_NET_SEND_AMOUNT)], runningBalances: [toWei(0)], - groupIndex: 0, + groupIndex: toBN(0), leafId: toBN(i), l1Tokens: [L1_TOKEN], });