From b62536af38c8704f7d23bb469864822e23de220b Mon Sep 17 00:00:00 2001 From: Daniel Izdebski Date: Tue, 4 Jan 2022 08:31:42 +0100 Subject: [PATCH 01/13] Disallow calling processWithdrawCommitment twice --- contracts/WithdrawManager.sol | 4 ++++ test/slow/rollup.massMigration.test.ts | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/contracts/WithdrawManager.sol b/contracts/WithdrawManager.sol index 2d02b538..6efce8be 100644 --- a/contracts/WithdrawManager.sol +++ b/contracts/WithdrawManager.sol @@ -41,6 +41,10 @@ contract WithdrawManager { uint256 batchID, Types.MMCommitmentInclusionProof memory commitmentMP ) public { + require( + processed[commitmentMP.commitment.body.withdrawRoot] == "", + "WithdrawManager: commitment was already processed" + ); vault.requestApproval(batchID, commitmentMP); (address addr, uint256 l2Unit) = tokenRegistry.safeGetRecord(commitmentMP.commitment.body.tokenID); diff --git a/test/slow/rollup.massMigration.test.ts b/test/slow/rollup.massMigration.test.ts index e7870646..4f00bca4 100644 --- a/test/slow/rollup.massMigration.test.ts +++ b/test/slow/rollup.massMigration.test.ts @@ -201,6 +201,12 @@ describe("Mass Migrations", async function() { "Transaction cost: Process Withdraw Commitment", receiptProcess.gasUsed.toNumber() ); + + await expectRevert( + withdrawManager.processWithdrawCommitment(batchId, batch.proof(0)), + "WithdrawManager: commitment was already processed" + ); + const withdrawProof = migrationTree.getWithdrawProof(0); const [, claimer] = await ethers.getSigners(); const claimerAddress = await claimer.getAddress(); From fa2de57a60dc8b046bec03640f5f5f47c56daa05 Mon Sep 17 00:00:00 2001 From: Daniel Izdebski Date: Tue, 4 Jan 2022 10:25:43 +0100 Subject: [PATCH 02/13] Remove bitmap from Vault --- contracts/Vault.sol | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/contracts/Vault.sol b/contracts/Vault.sol index 34f3c749..2a23ae4f 100644 --- a/contracts/Vault.sol +++ b/contracts/Vault.sol @@ -4,7 +4,6 @@ pragma experimental ABIEncoderV2; import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import { Initializable } from "@openzeppelin/contracts/proxy/Initializable.sol"; -import { Bitmap } from "./libs/Bitmap.sol"; import { Types } from "./libs/Types.sol"; import { MerkleTree } from "./libs/MerkleTree.sol"; import { ImmutableOwnable } from "./libs/ImmutableOwnable.sol"; @@ -21,8 +20,6 @@ contract Vault is Initializable, ImmutableOwnable { SpokeRegistry public immutable spokes; ITokenRegistry public immutable tokenRegistry; - mapping(uint256 => uint256) private bitmap; - constructor(ITokenRegistry _tokenRegistry, SpokeRegistry _spokes) public { tokenRegistry = _tokenRegistry; spokes = _spokes; @@ -37,10 +34,6 @@ contract Vault is Initializable, ImmutableOwnable { rollup = _rollup; } - function isBatchApproved(uint256 batchID) public view returns (bool) { - return Bitmap.isClaimed(batchID, bitmap); - } - function requestApproval( uint256 batchID, Types.MMCommitmentInclusionProof memory commitmentMP @@ -66,10 +59,11 @@ contract Vault is Initializable, ImmutableOwnable { ), "Vault: Commitment is not present in batch" ); + (address addr, uint256 l2Unit) = tokenRegistry.safeGetRecord(commitmentMP.commitment.body.tokenID); - Bitmap.setClaimed(batchID, bitmap); uint256 l1Amount = commitmentMP.commitment.body.amount * l2Unit; + require( IERC20(addr).approve(msg.sender, l1Amount), "Vault: Token approval failed" From 4ce3ac050cac0c889107d469c42d6462b26c7ec4 Mon Sep 17 00:00:00 2001 From: Daniel Izdebski Date: Fri, 21 Jan 2022 12:15:33 +0100 Subject: [PATCH 03/13] Add test for withdraw root collision --- test/slow/rollup.massMigration.test.ts | 130 ++++++++++++++++++------- 1 file changed, 96 insertions(+), 34 deletions(-) diff --git a/test/slow/rollup.massMigration.test.ts b/test/slow/rollup.massMigration.test.ts index 4f00bca4..7441ff9f 100644 --- a/test/slow/rollup.massMigration.test.ts +++ b/test/slow/rollup.massMigration.test.ts @@ -16,6 +16,8 @@ import { expectRevert } from "../../test/utils"; import { Group, txMassMigrationFactory } from "../../ts/factory"; import { deployKeyless } from "../../ts/deployment/deploy"; import { handleNewBatch } from "../../ts/client/batchHandler"; +import { Rollup, WithdrawManager } from "../../types/ethers-contracts"; +import { XCommitmentInclusionProof } from "../../ts/client/features/interface"; chai.use(chaiAsPromised); @@ -28,6 +30,7 @@ describe("Mass Migrations", async function() { let users: Group; let genesisRoot: string; const spokeID = 0; + before(async function() { await mcl.init(); }); @@ -60,6 +63,58 @@ describe("Mass Migrations", async function() { } }); + async function createAndSubmitBatch(rollup: Rollup, batchId: number) { + const feeReceiver = users.getUser(0).stateID; + const alice = users.getUser(1); + const aliceState = stateTree.getState(alice.stateID).state; + + const tx = new TxMassMigration( + alice.stateID, + CommonToken.fromHumanValue("39.99").l2Value, + 1, + CommonToken.fromHumanValue("0.01").l2Value, + aliceState.nonce.add(1).toNumber() + ); + stateTree.processMassMigrationCommit([tx], feeReceiver); + + const { + commitment, + migrationTree + } = MassMigrationCommitment.fromStateProvider( + registry.root(), + [tx], + alice.sign(tx).sol, + feeReceiver, + stateTree + ); + + const batch = commitment.toBatch(); + await batch.submit(rollup, batchId, TESTING_PARAMS.STAKE_AMOUNT); + + return { tx, commitment, migrationTree }; + } + + async function verifyProcessWithdrawCommitment( + withdrawManager: WithdrawManager, + batchId: number, + proof: XCommitmentInclusionProof + ) { + const txProcess = await withdrawManager.processWithdrawCommitment( + batchId, + proof + ); + const receiptProcess = await txProcess.wait(); + console.log( + "Transaction cost: Process Withdraw Commitment", + receiptProcess.gasUsed.toNumber() + ); + + await expectRevert( + withdrawManager.processWithdrawCommitment(batchId, proof), + "WithdrawManager: commitment was already processed" + ); + } + it("fails if batchID is incorrect", async function() { const feeReceiver = users.getUser(0).stateID; const { txs, signature } = txMassMigrationFactory(users, spokeID); @@ -147,36 +202,19 @@ describe("Mass Migrations", async function() { "Good state transition should not rollback" ); }).timeout(80000); + it("submit a batch, finalize, and withdraw", async function() { const { rollup, withdrawManager, exampleToken, vault } = contracts; - const feeReceiver = users.getUser(0).stateID; const alice = users.getUser(1); - const aliceState = stateTree.getState(alice.stateID).state; - const tx = new TxMassMigration( - alice.stateID, - CommonToken.fromHumanValue("39.99").l2Value, - 1, - CommonToken.fromHumanValue("0.01").l2Value, - aliceState.nonce.add(1).toNumber() - ); - stateTree.processMassMigrationCommit([tx], feeReceiver); - const { - commitment, - migrationTree - } = MassMigrationCommitment.fromStateProvider( - registry.root(), - [tx], - alice.sign(tx).sol, - feeReceiver, - stateTree + const batchId = Number(await rollup.nextBatchID()); + + const { tx, commitment, migrationTree } = await createAndSubmitBatch( + rollup, + batchId ); const batch = commitment.toBatch(); - const mMBatchID = 1; - await batch.submit(rollup, mMBatchID, TESTING_PARAMS.STAKE_AMOUNT); - - const batchId = Number(await rollup.nextBatchID()) - 1; await expectRevert( withdrawManager.processWithdrawCommitment(batchId, batch.proof(0)), @@ -192,20 +230,11 @@ describe("Mass Migrations", async function() { CommonToken.fromL2Value(tx.amount).l1Value ); - const txProcess = await withdrawManager.processWithdrawCommitment( + await verifyProcessWithdrawCommitment( + withdrawManager, batchId, batch.proof(0) ); - const receiptProcess = await txProcess.wait(); - console.log( - "Transaction cost: Process Withdraw Commitment", - receiptProcess.gasUsed.toNumber() - ); - - await expectRevert( - withdrawManager.processWithdrawCommitment(batchId, batch.proof(0)), - "WithdrawManager: commitment was already processed" - ); const withdrawProof = migrationTree.getWithdrawProof(0); const [, claimer] = await ethers.getSigners(); @@ -231,4 +260,37 @@ describe("Mass Migrations", async function() { // Try double claim await expectRevert(claim(), "WithdrawManager: Token has been claimed"); }); + + it("verifies absence of withdraw root collisions", async function() { + const { rollup, withdrawManager, exampleToken, vault } = contracts; + + const { tx: tx1, commitment: commitment1 } = await createAndSubmitBatch( + rollup, + 1 + ); + const { commitment: commitment2 } = await createAndSubmitBatch( + rollup, + 2 + ); + + await mineBlocks(ethers.provider, TESTING_PARAMS.BLOCKS_TO_FINALISE); + + // We cheat here a little bit by sending token to the vault manually. + // Ideally the tokens of the vault should come from the deposits + await exampleToken.transfer( + vault.address, + CommonToken.fromL2Value(tx1.amount).l1Value.mul(2) + ); + + await verifyProcessWithdrawCommitment( + withdrawManager, + 1, + commitment1.toBatch().proof(0) + ); + await verifyProcessWithdrawCommitment( + withdrawManager, + 2, + commitment2.toBatch().proof(0) + ); + }); }); From a6c833117d5a66ec80c181e829fef8e3cb0e2ca8 Mon Sep 17 00:00:00 2001 From: Daniel Izdebski Date: Fri, 21 Jan 2022 12:53:18 +0100 Subject: [PATCH 04/13] Use nonce while encoding user state --- contracts/client/FrontendCreate2Transfer.sol | 3 ++- contracts/client/FrontendMassMigration.sol | 3 ++- contracts/libs/Transition.sol | 9 +++++---- ts/commitments.ts | 2 +- 4 files changed, 10 insertions(+), 7 deletions(-) diff --git a/contracts/client/FrontendCreate2Transfer.sol b/contracts/client/FrontendCreate2Transfer.sol index 27234749..5901f95a 100644 --- a/contracts/client/FrontendCreate2Transfer.sol +++ b/contracts/client/FrontendCreate2Transfer.sol @@ -154,7 +154,8 @@ contract FrontendCreate2Transfer { newReceiver = Transition.createState( _tx.toPubkeyID, tokenID, - _tx.amount + _tx.amount, + 0 ); return (sender.encode(), newReceiver, result); diff --git a/contracts/client/FrontendMassMigration.sol b/contracts/client/FrontendMassMigration.sol index 28fda34a..47e13a89 100644 --- a/contracts/client/FrontendMassMigration.sol +++ b/contracts/client/FrontendMassMigration.sol @@ -123,7 +123,8 @@ contract FrontendMassMigration { newReceiver = Transition.createState( sender.pubkeyID, tokenID, - _tx.amount + _tx.amount, + sender.nonce ); return (sender.encode(), newReceiver, result); } diff --git a/contracts/libs/Transition.sol b/contracts/libs/Transition.sol index 7e0bd099..baec949c 100644 --- a/contracts/libs/Transition.sol +++ b/contracts/libs/Transition.sol @@ -63,7 +63,7 @@ library Transition { from ); if (result != Types.Result.Ok) return (newRoot, "", result); - freshState = createState(from.state.pubkeyID, tokenID, _tx.amount); + freshState = createState(from.state.pubkeyID, tokenID, _tx.amount, from.state.nonce); return (newRoot, freshState, Types.Result.Ok); } @@ -106,7 +106,7 @@ library Transition { "Create2Transfer: receiver proof invalid" ); bytes memory encodedState = - createState(_tx.toPubkeyID, tokenID, _tx.amount); + createState(_tx.toPubkeyID, tokenID, _tx.amount, 0); newRoot = MerkleTree.computeRoot( keccak256(encodedState), @@ -212,14 +212,15 @@ library Transition { function createState( uint256 pubkeyID, uint256 tokenID, - uint256 amount + uint256 amount, + uint256 nonce ) internal pure returns (bytes memory stateEncoded) { Types.UserState memory state = Types.UserState({ pubkeyID: pubkeyID, tokenID: tokenID, balance: amount, - nonce: 0 + nonce: nonce }); return state.encode(); } diff --git a/ts/commitments.ts b/ts/commitments.ts index 38f65870..52a2061d 100644 --- a/ts/commitments.ts +++ b/ts/commitments.ts @@ -154,7 +154,7 @@ export class MassMigrationCommitment extends Commitment { origin.pubkeyID, origin.tokenID, tx.amount, - 0 + tx.nonce, ); states.push(destination); } From da086253f34fca4f3a1bc68f7578c2cb1aee0890 Mon Sep 17 00:00:00 2001 From: Daniel Izdebski Date: Fri, 21 Jan 2022 13:09:01 +0100 Subject: [PATCH 05/13] Lint --- contracts/libs/Transition.sol | 7 ++++++- ts/commitments.ts | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/contracts/libs/Transition.sol b/contracts/libs/Transition.sol index baec949c..46ba6c5c 100644 --- a/contracts/libs/Transition.sol +++ b/contracts/libs/Transition.sol @@ -63,7 +63,12 @@ library Transition { from ); if (result != Types.Result.Ok) return (newRoot, "", result); - freshState = createState(from.state.pubkeyID, tokenID, _tx.amount, from.state.nonce); + freshState = createState( + from.state.pubkeyID, + tokenID, + _tx.amount, + from.state.nonce + ); return (newRoot, freshState, Types.Result.Ok); } diff --git a/ts/commitments.ts b/ts/commitments.ts index 52a2061d..75c57f4f 100644 --- a/ts/commitments.ts +++ b/ts/commitments.ts @@ -154,7 +154,7 @@ export class MassMigrationCommitment extends Commitment { origin.pubkeyID, origin.tokenID, tx.amount, - tx.nonce, + tx.nonce ); states.push(destination); } From 49431f781e28f754592aba1ac44957468a38b5c7 Mon Sep 17 00:00:00 2001 From: Daniel Izdebski Date: Wed, 16 Feb 2022 16:06:59 +0100 Subject: [PATCH 06/13] Add new UserState type and use it in withdrawal process --- contracts/WithdrawManager.sol | 2 +- contracts/libs/Types.sol | 25 ++++++++++++++++++++++++- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/contracts/WithdrawManager.sol b/contracts/WithdrawManager.sol index 6efce8be..d906ab6b 100644 --- a/contracts/WithdrawManager.sol +++ b/contracts/WithdrawManager.sol @@ -15,7 +15,7 @@ import { Vault } from "./Vault.sol"; contract WithdrawManager { using Tx for bytes; - using Types for Types.UserState; + using Types for Types.MMUserState; using SafeERC20 for IERC20; // withdrawRoot => a bitmap of whether a publicIndex owner has the token claimed diff --git a/contracts/libs/Types.sol b/contracts/libs/Types.sol index 2dc35f98..40bbf138 100644 --- a/contracts/libs/Types.sol +++ b/contracts/libs/Types.sol @@ -223,13 +223,36 @@ library Types { .decode(encoded, (uint256, uint256, uint256, uint256)); } + struct MMUserState { + uint256 stateID; + uint256 pubkeyID; + uint256 tokenID; + uint256 balance; + uint256 nonce; + } + + function encode(MMUserState memory state) + internal + pure + returns (bytes memory) + { + return + abi.encodePacked( + state.stateID, + state.pubkeyID, + state.tokenID, + state.balance, + state.nonce + ); + } + struct StateMerkleProof { UserState state; bytes32[] witness; } struct StateMerkleProofWithPath { - UserState state; + MMUserState state; uint256 path; bytes32[] witness; } From 3d77655c6954bf243e91da28ed31fcde5da643e2 Mon Sep 17 00:00:00 2001 From: Daniel Izdebski Date: Wed, 16 Feb 2022 16:19:00 +0100 Subject: [PATCH 07/13] Update withdraw root collision test and use new MMUserState type --- test/integration.test.ts | 2 +- test/slow/rollup.massMigration.test.ts | 64 ++++++++++++------- ts/commitments.ts | 5 +- ts/mmState.ts | 86 ++++++++++++++++++++++++++ ts/stateTree.ts | 18 ++++-- 5 files changed, 146 insertions(+), 29 deletions(-) create mode 100644 ts/mmState.ts diff --git a/test/integration.test.ts b/test/integration.test.ts index 5f01bcdf..158537ed 100644 --- a/test/integration.test.ts +++ b/test/integration.test.ts @@ -332,7 +332,7 @@ describe("Integration Test", function() { const { user, index } = group.pickRandom(); const signature = user.signRaw(withdrawerAddress).sol; // The new stateID in the migration tree is the position user in the group - const withdrawProof = tree.getWithdrawProof(index); + const withdrawProof = tree.getWithdrawProof(user.stateID, index); await withdrawManager .connect(withdrawer) .claimTokens( diff --git a/test/slow/rollup.massMigration.test.ts b/test/slow/rollup.massMigration.test.ts index 7441ff9f..dcb382c7 100644 --- a/test/slow/rollup.massMigration.test.ts +++ b/test/slow/rollup.massMigration.test.ts @@ -13,7 +13,7 @@ import { CommonToken } from "../../ts/decimal"; import { Result } from "../../ts/interfaces"; import { hexToUint8Array, mineBlocks } from "../../ts/utils"; import { expectRevert } from "../../test/utils"; -import { Group, txMassMigrationFactory } from "../../ts/factory"; +import { Group, txMassMigrationFactory, User } from "../../ts/factory"; import { deployKeyless } from "../../ts/deployment/deploy"; import { handleNewBatch } from "../../ts/client/batchHandler"; import { Rollup, WithdrawManager } from "../../types/ethers-contracts"; @@ -37,7 +37,7 @@ describe("Mass Migrations", async function() { beforeEach(async function() { const [signer] = await ethers.getSigners(); - users = Group.new({ n: 32, initialStateID: 0, initialPubkeyID: 0 }); + users = Group.new({ n: 30, initialStateID: 0, initialPubkeyID: 0 }); stateTree = new StateTree(TESTING_PARAMS.MAX_DEPTH); // The example token is 18 decimals const initialBalance = CommonToken.fromHumanValue("1000").l2Value; @@ -45,6 +45,18 @@ describe("Mass Migrations", async function() { .connect(stateTree) .createStates({ initialBalance, tokenID, zeroNonce: false }); + const usersWithDifferentPubkeyIDs = users; + + let usersWithTheSamePubkeyID = Group.new({ + n: 2, + initialStateID: 30, + initialPubkeyID: 0 + }); + usersWithTheSamePubkeyID + .connect(stateTree) + .createStates({ initialBalance, tokenID, zeroNonce: false }); + users = users.join(usersWithTheSamePubkeyID); + genesisRoot = stateTree.root; await deployKeyless(signer, false); @@ -57,23 +69,26 @@ describe("Mass Migrations", async function() { users.setupSigners(DOMAIN); registry = await AccountRegistry.new(blsAccountRegistry); - for (const user of users.userIterator()) { + for (const user of usersWithDifferentPubkeyIDs.userIterator()) { const pubkeyID = await registry.register(user.pubkey); assert.equal(pubkeyID, user.pubkeyID); } }); - async function createAndSubmitBatch(rollup: Rollup, batchId: number) { + async function createAndSubmitBatch( + rollup: Rollup, + batchId: number, + user: User + ) { const feeReceiver = users.getUser(0).stateID; - const alice = users.getUser(1); - const aliceState = stateTree.getState(alice.stateID).state; + const userState = stateTree.getState(user.stateID).state; const tx = new TxMassMigration( - alice.stateID, + user.stateID, CommonToken.fromHumanValue("39.99").l2Value, 1, CommonToken.fromHumanValue("0.01").l2Value, - aliceState.nonce.add(1).toNumber() + userState.nonce.add(1).toNumber() ); stateTree.processMassMigrationCommit([tx], feeReceiver); @@ -83,7 +98,7 @@ describe("Mass Migrations", async function() { } = MassMigrationCommitment.fromStateProvider( registry.root(), [tx], - alice.sign(tx).sol, + user.sign(tx).sol, feeReceiver, stateTree ); @@ -209,10 +224,11 @@ describe("Mass Migrations", async function() { const batchId = Number(await rollup.nextBatchID()); - const { tx, commitment, migrationTree } = await createAndSubmitBatch( - rollup, - batchId - ); + const { + tx, + commitment, + migrationTree + } = await createAndSubmitBatch(rollup, batchId, alice); const batch = commitment.toBatch(); @@ -236,7 +252,7 @@ describe("Mass Migrations", async function() { batch.proof(0) ); - const withdrawProof = migrationTree.getWithdrawProof(0); + const withdrawProof = migrationTree.getWithdrawProof(alice.stateID, 0); const [, claimer] = await ethers.getSigners(); const claimerAddress = await claimer.getAddress(); const signature = alice.signRaw(claimerAddress).sol; @@ -264,14 +280,18 @@ describe("Mass Migrations", async function() { it("verifies absence of withdraw root collisions", async function() { const { rollup, withdrawManager, exampleToken, vault } = contracts; - const { tx: tx1, commitment: commitment1 } = await createAndSubmitBatch( - rollup, - 1 - ); - const { commitment: commitment2 } = await createAndSubmitBatch( - rollup, - 2 - ); + const alice = users.getUser(1); + const aliceClone = users.getUser(31); + assert.equal(alice.pubkeyID, aliceClone.pubkeyID); + assert.notEqual(alice.stateID, aliceClone.stateID); + + const { + tx: tx1, + commitment: commitment1 + } = await createAndSubmitBatch(rollup, 1, alice); + const { + commitment: commitment2 + } = await createAndSubmitBatch(rollup, 2, aliceClone); await mineBlocks(ethers.provider, TESTING_PARAMS.BLOCKS_TO_FINALISE); diff --git a/ts/commitments.ts b/ts/commitments.ts index 75c57f4f..aa58f132 100644 --- a/ts/commitments.ts +++ b/ts/commitments.ts @@ -4,7 +4,7 @@ import { Rollup } from "../types/ethers-contracts/Rollup"; import { ZERO_BYTES32 } from "./constants"; import { Usage, Wei } from "./interfaces"; import { solG1 } from "./mcl"; -import { State } from "./state"; +import { MMState } from "./mmState"; import { MigrationTree, StateProvider } from "./stateTree"; import { MemoryTree } from "./tree/memoryTree"; import { serialize, TxMassMigration } from "./tx"; @@ -150,7 +150,8 @@ export class MassMigrationCommitment extends Commitment { const states = []; for (const tx of txs) { const origin = stateProvider.getState(tx.fromIndex).state; - const destination = State.new( + const destination = MMState.new( + tx.fromIndex, origin.pubkeyID, origin.tokenID, tx.amount, diff --git a/ts/mmState.ts b/ts/mmState.ts new file mode 100644 index 00000000..64199311 --- /dev/null +++ b/ts/mmState.ts @@ -0,0 +1,86 @@ +import { BigNumber, BigNumberish, ethers } from "ethers"; +import { solidityPack } from "ethers/lib/utils"; +import { State } from "./state"; + +export class MMState implements State { + public static new( + stateID: BigNumberish, + pubkeyID: BigNumberish, + tokenID: BigNumberish, + balance: BigNumberish, + nonce: BigNumberish + ): MMState { + return new MMState( + BigNumber.from(stateID), + BigNumber.from(pubkeyID), + BigNumber.from(tokenID), + BigNumber.from(balance), + BigNumber.from(nonce) + ); + } + + constructor( + public stateID: BigNumber, + public pubkeyID: BigNumber, + public tokenID: BigNumber, + public balance: BigNumber, + public nonce: BigNumber + ) {} + + public encode(): string { + return solidityPack( + ["uint256", "uint256", "uint256", "uint256", "uint256"], + [ + this.stateID, + this.pubkeyID, + this.tokenID, + this.balance, + this.nonce + ] + ); + } + + public hash(): string { + return ethers.utils.solidityKeccak256( + ["uint256", "uint256", "uint256", "uint256", "uint256"], + [ + this.stateID, + this.pubkeyID, + this.tokenID, + this.balance, + this.nonce + ] + ); + } + + public toJSON() { + return { + stateID: this.stateID.toString(), + pubkeyID: this.pubkeyID.toString(), + tokenID: this.tokenID.toString(), + balance: this.balance.toString(), + nonce: this.nonce.toString() + }; + } + + public toString(): string { + const propsStr = Object.entries(this.toJSON()) + .map(([k, v]) => `${k} ${v}`) + .join(" "); + return ``; + } + + public clone() { + return new MMState( + this.stateID, + this.pubkeyID, + this.tokenID, + this.balance, + this.nonce + ); + } + + public toStateLeaf(): string { + return this.hash(); + } +} diff --git a/ts/stateTree.ts b/ts/stateTree.ts index 27a66ce7..31cba636 100644 --- a/ts/stateTree.ts +++ b/ts/stateTree.ts @@ -1,7 +1,7 @@ import { Hasher } from "./tree"; import { State, ZERO_STATE } from "./state"; import { TxTransfer, TxMassMigration, TxCreate2Transfer } from "./tx"; -import { BigNumber, constants } from "ethers"; +import { BigNumber, BigNumberish, constants } from "ethers"; import { ZERO_BYTES32 } from "./constants"; import { minTreeDepth, sum } from "./utils"; import { @@ -15,6 +15,7 @@ import { } from "./exceptions"; import { Vacant } from "./interfaces"; import { MemoryTree } from "./tree/memoryTree"; +import { MMState } from "./mmState"; export interface StateProvider { getState(stateID: number): SolStateMerkleProof; @@ -379,8 +380,17 @@ export class MigrationTree extends StateTree { return new this(depth).createStateBulk(0, states); } - public getWithdrawProof(stateID: number) { - const { state, witness } = this.getState(stateID); - return { state, witness, path: stateID }; + public getWithdrawProof(stateID: BigNumberish, txIndex: number) { + const { state, witness } = this.getState(txIndex); + + const mmState = MMState.new( + stateID, + state.pubkeyID, + state.tokenID, + state.balance, + state.nonce + ); + + return { state: mmState, witness, path: txIndex }; } } From 69b923d4cb8ef419110222313aaf6cc4301c2635 Mon Sep 17 00:00:00 2001 From: Daniel Izdebski Date: Thu, 17 Feb 2022 12:47:13 +0100 Subject: [PATCH 08/13] Move withdrawal commitment validation to Vault --- contracts/Vault.sol | 10 ++++++++++ contracts/WithdrawManager.sol | 4 ---- test/slow/rollup.massMigration.test.ts | 2 +- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/contracts/Vault.sol b/contracts/Vault.sol index 2a23ae4f..03f3f350 100644 --- a/contracts/Vault.sol +++ b/contracts/Vault.sol @@ -15,6 +15,9 @@ contract Vault is Initializable, ImmutableOwnable { using Types for Types.MassMigrationCommitment; using Types for Types.Batch; + // withdrawRoot => bool + mapping(bytes32 => bool) private approvedWithdrawals; + // Can't be immutable yet. Since the rollup is deployed after Vault Rollup public rollup; SpokeRegistry public immutable spokes; @@ -38,6 +41,11 @@ contract Vault is Initializable, ImmutableOwnable { uint256 batchID, Types.MMCommitmentInclusionProof memory commitmentMP ) public { + require( + approvedWithdrawals[commitmentMP.commitment.body.withdrawRoot] != true, + "Vault: commitment was already approved for withdrawal" + ); + require( msg.sender == spokes.getSpokeAddress(commitmentMP.commitment.body.spokeID), @@ -60,6 +68,8 @@ contract Vault is Initializable, ImmutableOwnable { "Vault: Commitment is not present in batch" ); + approvedWithdrawals[commitmentMP.commitment.body.withdrawRoot] = true; + (address addr, uint256 l2Unit) = tokenRegistry.safeGetRecord(commitmentMP.commitment.body.tokenID); uint256 l1Amount = commitmentMP.commitment.body.amount * l2Unit; diff --git a/contracts/WithdrawManager.sol b/contracts/WithdrawManager.sol index d906ab6b..d0f8ba04 100644 --- a/contracts/WithdrawManager.sol +++ b/contracts/WithdrawManager.sol @@ -41,10 +41,6 @@ contract WithdrawManager { uint256 batchID, Types.MMCommitmentInclusionProof memory commitmentMP ) public { - require( - processed[commitmentMP.commitment.body.withdrawRoot] == "", - "WithdrawManager: commitment was already processed" - ); vault.requestApproval(batchID, commitmentMP); (address addr, uint256 l2Unit) = tokenRegistry.safeGetRecord(commitmentMP.commitment.body.tokenID); diff --git a/test/slow/rollup.massMigration.test.ts b/test/slow/rollup.massMigration.test.ts index dcb382c7..8f92382d 100644 --- a/test/slow/rollup.massMigration.test.ts +++ b/test/slow/rollup.massMigration.test.ts @@ -126,7 +126,7 @@ describe("Mass Migrations", async function() { await expectRevert( withdrawManager.processWithdrawCommitment(batchId, proof), - "WithdrawManager: commitment was already processed" + "Vault: commitment was already approved for withdrawal" ); } From b24c979e5504f3f88d07d283ecc035b4cdff9d06 Mon Sep 17 00:00:00 2001 From: Daniel Izdebski Date: Thu, 17 Feb 2022 13:21:46 +0100 Subject: [PATCH 09/13] Use new MMUserState in processMassMigration --- contracts/libs/Transition.sol | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/contracts/libs/Transition.sol b/contracts/libs/Transition.sol index 46ba6c5c..2ec26632 100644 --- a/contracts/libs/Transition.sol +++ b/contracts/libs/Transition.sol @@ -13,6 +13,7 @@ import { Tx } from "./Tx.sol"; library Transition { using SafeMath for uint256; using Types for Types.UserState; + using Types for Types.MMUserState; function processTransfer( bytes32 stateRoot, @@ -63,7 +64,8 @@ library Transition { from ); if (result != Types.Result.Ok) return (newRoot, "", result); - freshState = createState( + freshState = createMMState( + _tx.fromIndex, from.state.pubkeyID, tokenID, _tx.amount, @@ -229,4 +231,22 @@ library Transition { }); return state.encode(); } + + function createMMState( + uint256 stateID, + uint256 pubkeyID, + uint256 tokenID, + uint256 amount, + uint256 nonce + ) internal pure returns (bytes memory stateEncoded) { + Types.MMUserState memory state = + Types.MMUserState({ + stateID: stateID, + pubkeyID: pubkeyID, + tokenID: tokenID, + balance: amount, + nonce: nonce + }); + return state.encode(); + } } From 9bcb7db2186f457a37a546284b58bef621a09dbb Mon Sep 17 00:00:00 2001 From: Daniel Izdebski Date: Thu, 17 Feb 2022 17:07:05 +0100 Subject: [PATCH 10/13] Lint --- contracts/Vault.sol | 2 +- contracts/libs/Transition.sol | 14 ++++++------- test/slow/rollup.massMigration.test.ts | 27 ++++++++++++++------------ 3 files changed, 23 insertions(+), 20 deletions(-) diff --git a/contracts/Vault.sol b/contracts/Vault.sol index 03f3f350..46413ba2 100644 --- a/contracts/Vault.sol +++ b/contracts/Vault.sol @@ -42,7 +42,7 @@ contract Vault is Initializable, ImmutableOwnable { Types.MMCommitmentInclusionProof memory commitmentMP ) public { require( - approvedWithdrawals[commitmentMP.commitment.body.withdrawRoot] != true, + !approvedWithdrawals[commitmentMP.commitment.body.withdrawRoot], "Vault: commitment was already approved for withdrawal" ); diff --git a/contracts/libs/Transition.sol b/contracts/libs/Transition.sol index 2ec26632..00e6ee68 100644 --- a/contracts/libs/Transition.sol +++ b/contracts/libs/Transition.sol @@ -240,13 +240,13 @@ library Transition { uint256 nonce ) internal pure returns (bytes memory stateEncoded) { Types.MMUserState memory state = - Types.MMUserState({ - stateID: stateID, - pubkeyID: pubkeyID, - tokenID: tokenID, - balance: amount, - nonce: nonce - }); + Types.MMUserState({ + stateID: stateID, + pubkeyID: pubkeyID, + tokenID: tokenID, + balance: amount, + nonce: nonce + }); return state.encode(); } } diff --git a/test/slow/rollup.massMigration.test.ts b/test/slow/rollup.massMigration.test.ts index 8f92382d..2ce392b2 100644 --- a/test/slow/rollup.massMigration.test.ts +++ b/test/slow/rollup.massMigration.test.ts @@ -224,11 +224,11 @@ describe("Mass Migrations", async function() { const batchId = Number(await rollup.nextBatchID()); - const { - tx, - commitment, - migrationTree - } = await createAndSubmitBatch(rollup, batchId, alice); + const { tx, commitment, migrationTree } = await createAndSubmitBatch( + rollup, + batchId, + alice + ); const batch = commitment.toBatch(); @@ -285,13 +285,16 @@ describe("Mass Migrations", async function() { assert.equal(alice.pubkeyID, aliceClone.pubkeyID); assert.notEqual(alice.stateID, aliceClone.stateID); - const { - tx: tx1, - commitment: commitment1 - } = await createAndSubmitBatch(rollup, 1, alice); - const { - commitment: commitment2 - } = await createAndSubmitBatch(rollup, 2, aliceClone); + const { tx: tx1, commitment: commitment1 } = await createAndSubmitBatch( + rollup, + 1, + alice + ); + const { commitment: commitment2 } = await createAndSubmitBatch( + rollup, + 2, + aliceClone + ); await mineBlocks(ethers.provider, TESTING_PARAMS.BLOCKS_TO_FINALISE); From 6e524a216023a50fe1ab62b0f6a4767a53559c7a Mon Sep 17 00:00:00 2001 From: Daniel Izdebski Date: Fri, 18 Feb 2022 11:15:37 +0100 Subject: [PATCH 11/13] Remove comment --- test/integration.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/test/integration.test.ts b/test/integration.test.ts index 158537ed..ba04c2a3 100644 --- a/test/integration.test.ts +++ b/test/integration.test.ts @@ -331,7 +331,6 @@ describe("Integration Test", function() { const tree = migrationTrees[i]; const { user, index } = group.pickRandom(); const signature = user.signRaw(withdrawerAddress).sol; - // The new stateID in the migration tree is the position user in the group const withdrawProof = tree.getWithdrawProof(user.stateID, index); await withdrawManager .connect(withdrawer) From 018536aff2214f098b27fbc356c1f054b1602d4b Mon Sep 17 00:00:00 2001 From: Daniel Izdebski Date: Wed, 23 Feb 2022 13:08:19 +0100 Subject: [PATCH 12/13] Use createMMState in FrontendMassMigration.validateAndApply --- contracts/client/FrontendMassMigration.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/client/FrontendMassMigration.sol b/contracts/client/FrontendMassMigration.sol index 47e13a89..811bb9ce 100644 --- a/contracts/client/FrontendMassMigration.sol +++ b/contracts/client/FrontendMassMigration.sol @@ -120,7 +120,8 @@ contract FrontendMassMigration { sender ); if (result != Types.Result.Ok) return (sender.encode(), "", result); - newReceiver = Transition.createState( + newReceiver = Transition.createMMState( + _tx.fromIndex, sender.pubkeyID, tokenID, _tx.amount, From 6aa25947e7bd6b07493888a3f18a28ea2f8f3bbb Mon Sep 17 00:00:00 2001 From: Daniel Izdebski Date: Wed, 23 Feb 2022 13:09:33 +0100 Subject: [PATCH 13/13] Remove unnecessary parameter from Transition.createState --- contracts/client/FrontendCreate2Transfer.sol | 3 +-- contracts/libs/Transition.sol | 7 +++---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/contracts/client/FrontendCreate2Transfer.sol b/contracts/client/FrontendCreate2Transfer.sol index 5901f95a..27234749 100644 --- a/contracts/client/FrontendCreate2Transfer.sol +++ b/contracts/client/FrontendCreate2Transfer.sol @@ -154,8 +154,7 @@ contract FrontendCreate2Transfer { newReceiver = Transition.createState( _tx.toPubkeyID, tokenID, - _tx.amount, - 0 + _tx.amount ); return (sender.encode(), newReceiver, result); diff --git a/contracts/libs/Transition.sol b/contracts/libs/Transition.sol index 00e6ee68..e695c566 100644 --- a/contracts/libs/Transition.sol +++ b/contracts/libs/Transition.sol @@ -113,7 +113,7 @@ library Transition { "Create2Transfer: receiver proof invalid" ); bytes memory encodedState = - createState(_tx.toPubkeyID, tokenID, _tx.amount, 0); + createState(_tx.toPubkeyID, tokenID, _tx.amount); newRoot = MerkleTree.computeRoot( keccak256(encodedState), @@ -219,15 +219,14 @@ library Transition { function createState( uint256 pubkeyID, uint256 tokenID, - uint256 amount, - uint256 nonce + uint256 amount ) internal pure returns (bytes memory stateEncoded) { Types.UserState memory state = Types.UserState({ pubkeyID: pubkeyID, tokenID: tokenID, balance: amount, - nonce: nonce + nonce: 0 }); return state.encode(); }