From 27284d16bc3e718368d1f5a5d0e9ea4ed26077ef Mon Sep 17 00:00:00 2001 From: cedoor Date: Tue, 13 Dec 2022 14:10:46 +0100 Subject: [PATCH] fix: add checks for new leaves in the update function This bug was found by Veridise during their audit of Semaphore. fix #35 --- .../contracts/IncrementalBinaryTree.sol | 2 ++ .../contracts/IncrementalQuinTree.sol | 2 ++ .../test/IncrementalBinaryTreeTest.ts | 24 ++++++++++++++++--- .../test/IncrementalQuinTreeTest.ts | 22 ++++++++++++++--- 4 files changed, 44 insertions(+), 6 deletions(-) diff --git a/packages/incremental-merkle-tree.sol/contracts/IncrementalBinaryTree.sol b/packages/incremental-merkle-tree.sol/contracts/IncrementalBinaryTree.sol index 574a5c2..840166c 100644 --- a/packages/incremental-merkle-tree.sol/contracts/IncrementalBinaryTree.sol +++ b/packages/incremental-merkle-tree.sol/contracts/IncrementalBinaryTree.sol @@ -92,6 +92,8 @@ library IncrementalBinaryTree { uint256[] calldata proofSiblings, uint8[] calldata proofPathIndices ) public { + require(newLeaf != leaf, "IncrementalBinaryTree: new leaf cannot be the same as the old one"); + require(newLeaf < SNARK_SCALAR_FIELD, "IncrementalBinaryTree: new leaf must be < SNARK_SCALAR_FIELD"); require( verify(self, leaf, proofSiblings, proofPathIndices), "IncrementalBinaryTree: leaf is not part of the tree" diff --git a/packages/incremental-merkle-tree.sol/contracts/IncrementalQuinTree.sol b/packages/incremental-merkle-tree.sol/contracts/IncrementalQuinTree.sol index 7f1bec2..aa41fe0 100644 --- a/packages/incremental-merkle-tree.sol/contracts/IncrementalQuinTree.sol +++ b/packages/incremental-merkle-tree.sol/contracts/IncrementalQuinTree.sol @@ -108,6 +108,8 @@ library IncrementalQuinTree { uint256[4][] calldata proofSiblings, uint8[] calldata proofPathIndices ) public { + require(newLeaf != leaf, "IncrementalQuinTree: new leaf cannot be the same as the old one"); + require(newLeaf < SNARK_SCALAR_FIELD, "IncrementalQuinTree: new leaf must be < SNARK_SCALAR_FIELD"); require( verify(self, leaf, proofSiblings, proofPathIndices), "IncrementalQuinTree: leaf is not part of the tree" diff --git a/packages/incremental-merkle-tree.sol/test/IncrementalBinaryTreeTest.ts b/packages/incremental-merkle-tree.sol/test/IncrementalBinaryTreeTest.ts index 9765968..fa4130b 100644 --- a/packages/incremental-merkle-tree.sol/test/IncrementalBinaryTreeTest.ts +++ b/packages/incremental-merkle-tree.sol/test/IncrementalBinaryTreeTest.ts @@ -91,11 +91,29 @@ describe("IncrementalBinaryTreeTest", () => { await expect(transaction).to.be.revertedWith("IncrementalBinaryTreeTest: tree does not exist") }) - it("Should not update a leaf if its value is > SNARK_SCALAR_FIELD", async () => { - const leaf = BigInt("21888242871839275222246405745257275088548364400416034343698204186575808495618") + it("Should not update a leaf if the new value is the same as the old one", async () => { + const leaf = BigInt(3) const transaction = contract.updateLeaf(treeId, leaf, leaf, [0, 1], [0, 1]) + await expect(transaction).to.be.revertedWith( + "IncrementalBinaryTree: new leaf cannot be the same as the old one" + ) + }) + + it("Should not update a leaf if its new value is > SNARK_SCALAR_FIELD", async () => { + const leaf = BigInt("21888242871839275222246405745257275088548364400416034343698204186575808495618") + + const transaction = contract.updateLeaf(treeId, BigInt(3), leaf, [0, 1], [0, 1]) + + await expect(transaction).to.be.revertedWith("IncrementalBinaryTree: new leaf must be < SNARK_SCALAR_FIELD") + }) + + it("Should not update a leaf if its original value is > SNARK_SCALAR_FIELD", async () => { + const leaf = BigInt("21888242871839275222246405745257275088548364400416034343698204186575808495618") + + const transaction = contract.updateLeaf(treeId, leaf, BigInt(4), [0, 1], [0, 1]) + await expect(transaction).to.be.revertedWith("IncrementalBinaryTree: leaf must be < SNARK_SCALAR_FIELD") }) @@ -114,7 +132,7 @@ describe("IncrementalBinaryTreeTest", () => { const { pathIndices, siblings } = tree.createProof(2) const transaction = contract.updateLeaf( treeId, - leaf, + BigInt(4), leaf, siblings.map((s) => s[0]), pathIndices diff --git a/packages/incremental-merkle-tree.sol/test/IncrementalQuinTreeTest.ts b/packages/incremental-merkle-tree.sol/test/IncrementalQuinTreeTest.ts index 94685a5..92b834d 100644 --- a/packages/incremental-merkle-tree.sol/test/IncrementalQuinTreeTest.ts +++ b/packages/incremental-merkle-tree.sol/test/IncrementalQuinTreeTest.ts @@ -94,11 +94,27 @@ describe("IncrementalQuinTreeTest", () => { await expect(transaction).to.be.revertedWith("IncrementalQuinTreeTest: tree does not exist") }) - it("Should not update a leaf if its value is > SNARK_SCALAR_FIELD", async () => { - const leaf = BigInt("21888242871839275222246405745257275088548364400416034343698204186575808495618") + it("Should not update a leaf if the new value is the same as the old one", async () => { + const leaf = BigInt(3) const transaction = contract.updateLeaf(treeId, leaf, leaf, [[0, 1, 2, 3]], [0]) + await expect(transaction).to.be.revertedWith("IncrementalQuinTree: new leaf cannot be the same as the old one") + }) + + it("Should not update a leaf if its new value is > SNARK_SCALAR_FIELD", async () => { + const leaf = BigInt("21888242871839275222246405745257275088548364400416034343698204186575808495618") + + const transaction = contract.updateLeaf(treeId, BigInt(3), leaf, [[0, 1, 2, 3]], [0]) + + await expect(transaction).to.be.revertedWith("IncrementalQuinTree: new leaf must be < SNARK_SCALAR_FIELD") + }) + + it("Should not update a leaf if its original value is > SNARK_SCALAR_FIELD", async () => { + const leaf = BigInt("21888242871839275222246405745257275088548364400416034343698204186575808495618") + + const transaction = contract.updateLeaf(treeId, leaf, BigInt(4), [[0, 1, 2, 3]], [0]) + await expect(transaction).to.be.revertedWith("IncrementalQuinTree: leaf must be < SNARK_SCALAR_FIELD") }) @@ -115,7 +131,7 @@ describe("IncrementalQuinTreeTest", () => { tree.update(2, leaf) const { pathIndices, siblings } = tree.createProof(2) - const transaction = contract.updateLeaf(treeId, leaf, leaf, siblings, pathIndices) + const transaction = contract.updateLeaf(treeId, BigInt(4), leaf, siblings, pathIndices) await expect(transaction).to.be.revertedWith("IncrementalQuinTree: leaf is not part of the tree") })