Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a MerkleProof.processProof utility function #2841

Merged
merged 14 commits into from Oct 14, 2021

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Aug 31, 2021

Fixes #2840

PR Checklist

  • Tests (covered by existing tests)
  • Changelog entry

contracts/utils/cryptography/MerkleProof.sol Outdated Show resolved Hide resolved
for (uint256 i = 0; i < proof.length; i++) {
bytes32 proofElement = proof[i];

index <<= 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do index * 2 for overflow protection?

Suggested change
index <<= 1;
index *= 2;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Amxx If this suggestion isn't relevant can you explain why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally cared mostly about consistency between line 50 and 57, but the overflow protection is really interesting.

  • if we use "multiply", we protect against overflow that would happen if there is a right-branching at least 256 down in the tree. For that to happen, the Merkle tree would have to be more than 256 levels depth, which is only required if the tree contains more than 2**256 leaves. (which means there much be duplicates or collisions in the leaves
  • if we use "shift", we don't have this protection.

This protection is technically a breaking changes because some proofs won't be verifiable anymore. These proofs will only realistically happen if trees are not built properly. Without this protection, the returned index value becomes unreliable if trees have more then 256 levels ...

My conclusion

  • In all reasonable cases, adding the protection should have no positive impact on security, and just add some gas cost.
  • In some corner cases, the protection will prevent verifying valid proofs, that were accepted before this PR.
  • I'd go for using shift.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the Merkle tree would have to be more than 256 levels depth, which is only required if the tree contains more than 2**256 leaves

Are trees necessarily balanced?

Copy link
Collaborator Author

@Amxx Amxx Oct 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contract do not require any balancing, but effectiveness of the merkle structure is optimized if the most frequently proved leaves are higher... In practice, this means that any good tooling will try to balance the tree ... and I don't expect we would ever have production trees that are so obviously unbalanced

contracts/utils/cryptography/MerkleProof.sol Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
frangio
frangio previously approved these changes Oct 13, 2021
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

contracts/utils/cryptography/MerkleProof.sol Outdated Show resolved Hide resolved
contracts/utils/cryptography/MerkleProof.sol Outdated Show resolved Hide resolved
contracts/utils/cryptography/MerkleProof.sol Outdated Show resolved Hide resolved
Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
@Amxx
Copy link
Collaborator Author

Amxx commented Oct 13, 2021

After discussion, we decided to remove the indexing feature from this PR, as it doesn't feel ready.

The previous implementation suffers from some issues:

  • Depending on the tree shape, we are not able to guarantee that the indices are sequential.
  • Depending on the tree shape, indices computation could overflow, and break the uniqueness property.
  • Depending on the way leaves hashes are produced, it might be possible to produce valid proofs for inner nodes, which would take the indices of one of the leaves in the subtree.

We will try designing a better mechanism, but until then we recommend building sequential indices inside the leaves (like uniswap does in the MerkleDistributor)

@Amxx Amxx merged commit d244b81 into OpenZeppelin:master Oct 14, 2021
@Amxx Amxx deleted the feature/merkle.getRoot branch October 14, 2021 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MerkleProof rebuild root.
2 participants