Detect MerkleProof multiproof invariant violation (#4367)

Co-authored-by: Hadrien Croubois <[email protected]>
OpenZeppelin · Jun 19, 2023 · c95a445 · c95a445
1 parent 1a77a50
commit c95a445
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 2 deletions.
diff --git a/.changeset/shy-crews-teach.md b/.changeset/shy-crews-teach.md
@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': patch
+---
+
+`MerkleProof`: Fix a bug in `processMultiProof` and `processMultiProofCalldata` that allows proving arbitrary leaves if the tree contains a node with value 0 at depth 1.
diff --git a/contracts/utils/cryptography/MerkleProof.sol b/contracts/utils/cryptography/MerkleProof.sol
@@ -126,10 +126,11 @@ library MerkleProof {
         // `hashes` array. At the end of the process, the last hash in the `hashes` array should contain the root of
         // the merkle tree.
         uint256 leavesLen = leaves.length;
+        uint256 proofLen = proof.length;
         uint256 totalHashes = proofFlags.length;
 
         // Check proof validity.
-        if (leavesLen + proof.length - 1 != totalHashes) {
+        if (leavesLen + proofLen - 1 != totalHashes) {
             revert MerkleProofInvalidMultiproof();
         }
 
@@ -153,6 +154,9 @@ library MerkleProof {
         }
 
         if (totalHashes > 0) {
+            if (proofPos != proofLen) {
+                revert MerkleProofInvalidMultiproof();
+            }
             unchecked {
                 return hashes[totalHashes - 1];
             }
@@ -180,10 +184,11 @@ library MerkleProof {
         // `hashes` array. At the end of the process, the last hash in the `hashes` array should contain the root of
         // the merkle tree.
         uint256 leavesLen = leaves.length;
+        uint256 proofLen = proof.length;
         uint256 totalHashes = proofFlags.length;
 
         // Check proof validity.
-        if (leavesLen + proof.length - 1 != totalHashes) {
+        if (leavesLen + proofLen - 1 != totalHashes) {
             revert MerkleProofInvalidMultiproof();
         }
 
@@ -207,6 +212,9 @@ library MerkleProof {
         }
 
         if (totalHashes > 0) {
+            if (proofPos != proofLen) {
+                revert MerkleProofInvalidMultiproof();
+            }
             unchecked {
                 return hashes[totalHashes - 1];
             }

diff --git a/test/utils/cryptography/MerkleProof.test.js b/test/utils/cryptography/MerkleProof.test.js
@@ -178,5 +178,30 @@ contract('MerkleProof', function () {
       expect(await this.merkleProof.$multiProofVerify([root], [], root, [])).to.equal(true);
       expect(await this.merkleProof.$multiProofVerifyCalldata([root], [], root, [])).to.equal(true);
     });
+
+    it('reverts processing manipulated proofs with a zero-value node at depth 1', async function () {
+      // Create a merkle tree that contains a zero leaf at depth 1
+      const leaves = [keccak256('real leaf'), Buffer.alloc(32, 0)];
+      const merkleTree = new MerkleTree(leaves, keccak256, { sortPairs: true });
+
+      const root = merkleTree.getRoot();
+
+      // Now we can pass any **malicious** fake leaves as valid!
+      const maliciousLeaves = ['malicious', 'leaves'].map(keccak256).sort(Buffer.compare);
+      const maliciousProof = [leaves[0], leaves[0]];
+      const maliciousProofFlags = [true, true, false];
+
+      await expectRevertCustomError(
+        this.merkleProof.$multiProofVerify(maliciousProof, maliciousProofFlags, root, maliciousLeaves),
+        'MerkleProofInvalidMultiproof',
+        [],
+      );
+
+      await expectRevertCustomError(
+        this.merkleProof.$multiProofVerifyCalldata(maliciousProof, maliciousProofFlags, root, maliciousLeaves),
+        'MerkleProofInvalidMultiproof',
+        [],
+      );
+    });
   });
 });