Skip to content

Commit

Permalink
Merge pull request from GHSA-wprv-93r4-jj2p
Browse files Browse the repository at this point in the history
  • Loading branch information
Amxx authored Jun 16, 2023
1 parent f03420b commit 4d2383e
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 6 deletions.
5 changes: 5 additions & 0 deletions .changeset/shy-crews-teach.md
Original file line number Diff line number Diff line change
@@ -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.
8 changes: 6 additions & 2 deletions contracts/utils/cryptography/MerkleProof.sol
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,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.
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".
Expand All @@ -146,6 +147,7 @@ library MerkleProof {
}

if (totalHashes > 0) {
require(proofPos == proofLen, "MerkleProof: invalid multiproof");
unchecked {
return hashes[totalHashes - 1];
}
Expand Down Expand Up @@ -173,10 +175,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.
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".
Expand All @@ -198,6 +201,7 @@ library MerkleProof {
}

if (totalHashes > 0) {
require(proofPos == proofLen, "MerkleProof: invalid multiproof");
unchecked {
return hashes[totalHashes - 1];
}
Expand Down
28 changes: 24 additions & 4 deletions test/utils/cryptography/MerkleProof.test.js
Original file line number Diff line number Diff line change
@@ -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 () {
Expand Down Expand Up @@ -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',
);
});
});
});

0 comments on commit 4d2383e

Please sign in to comment.