From e7d20ee1cecbfa36bd75ffb4b2e5dad4c47801f1 Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 16 Jun 2023 20:18:43 +0000 Subject: [PATCH] Transpile 4d2383e1 --- .changeset/shy-crews-teach.md | 5 ++++ .../cryptography/MerkleProofUpgradeable.sol | 8 ++++-- test/utils/cryptography/MerkleProof.test.js | 28 ++++++++++++++++--- 3 files changed, 35 insertions(+), 6 deletions(-) create mode 100644 .changeset/shy-crews-teach.md diff --git a/.changeset/shy-crews-teach.md b/.changeset/shy-crews-teach.md new file mode 100644 index 000000000..8ab929bf8 --- /dev/null +++ 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/MerkleProofUpgradeable.sol b/contracts/utils/cryptography/MerkleProofUpgradeable.sol index 6cde6a8f9..25925c436 100644 --- a/contracts/utils/cryptography/MerkleProofUpgradeable.sol +++ b/contracts/utils/cryptography/MerkleProofUpgradeable.sol @@ -121,10 +121,11 @@ library MerkleProofUpgradeable { // `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. - require(leavesLen + proof.length - 1 == totalHashes, "MerkleProof: invalid multiproof"); + require(leavesLen + proofLen - 1 == totalHashes, "MerkleProof: invalid multiproof"); // The xxxPos values are "pointers" to the next value to consume in each array. All accesses are done using // `xxx[xxxPos++]`, which return the current value and increment the pointer, thus mimicking a queue's "pop". @@ -146,6 +147,7 @@ library MerkleProofUpgradeable { } if (totalHashes > 0) { + require(proofPos == proofLen, "MerkleProof: invalid multiproof"); unchecked { return hashes[totalHashes - 1]; } @@ -173,10 +175,11 @@ library MerkleProofUpgradeable { // `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. - require(leavesLen + proof.length - 1 == totalHashes, "MerkleProof: invalid multiproof"); + require(leavesLen + proofLen - 1 == totalHashes, "MerkleProof: invalid multiproof"); // The xxxPos values are "pointers" to the next value to consume in each array. All accesses are done using // `xxx[xxxPos++]`, which return the current value and increment the pointer, thus mimicking a queue's "pop". @@ -198,6 +201,7 @@ library MerkleProofUpgradeable { } if (totalHashes > 0) { + require(proofPos == proofLen, "MerkleProof: invalid multiproof"); unchecked { return hashes[totalHashes - 1]; } diff --git a/test/utils/cryptography/MerkleProof.test.js b/test/utils/cryptography/MerkleProof.test.js index 62157b56a..9ed10fd0f 100644 --- a/test/utils/cryptography/MerkleProof.test.js +++ b/test/utils/cryptography/MerkleProof.test.js @@ -1,11 +1,8 @@ -require('@openzeppelin/test-helpers'); - const { expectRevert } = require('@openzeppelin/test-helpers'); +const { expect } = require('chai'); const { MerkleTree } = require('merkletreejs'); const keccak256 = require('keccak256'); -const { expect } = require('chai'); - const MerkleProof = artifacts.require('$MerkleProof'); contract('MerkleProof', function () { @@ -176,5 +173,28 @@ 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 = ['some', 'malicious', 'leaves'].map(keccak256).sort(Buffer.compare); + const maliciousProof = [leaves[0], leaves[0]]; + const maliciousProofFlags = [true, true, false]; + + await expectRevert( + this.merkleProof.$multiProofVerify(maliciousProof, maliciousProofFlags, root, maliciousLeaves), + 'MerkleProof: invalid multiproof', + ); + + await expectRevert( + this.merkleProof.$multiProofVerifyCalldata(maliciousProof, maliciousProofFlags, root, maliciousLeaves), + 'MerkleProof: invalid multiproof', + ); + }); }); });