code-423n4 / 2022-05-opensea-seaport-findings

1 stars 0 forks source link

Verifying criteria is prone to known merkle proof attacks #211

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/CriteriaResolution.sol#L241-L287

Vulnerability details

The Merkle hash root does not indicate the tree depth, enabling a second-preimage attack in which an attacker creates a document other than the original that has the same Merkle hash root. For the example above, an attacker can create a new document containing two data blocks, where the first is hash 0-0 + hash 0–1, and the second is hash 1-0 + hash 1-1.

this implemenation is vulnerable to a second pre-image attack.

Also, this implementation is vulnerable to a forgery attack for an unbalanced tree, where the last leaf node can be duplicated to create an artificial balanced tree, resulting in the same Merkle root hash.

Tools Used

Manual Review

Recommended Mitigation Steps

1) Use a difference hashing function for leaves and nodes, so that H(x) != H'(x). 2) Do not accept unbalanced tree to prevent forgery attack

0age commented 2 years ago

confirmed partially, disputed other aspects

d1ll0n commented 2 years ago

The example given on the wikipedia page of giving two leaves as (0-0 . 0-1) and (1-0 . 1-1) is impossible, as all parent nodes will be 64 bytes before being hashed and the contract will only accept 32 byte IDs as leaves. The only version of this that could work is providing the merkle root itself as a leaf with no proof, as reported in another issue.

Duplicating a leaf node also isn't an issue as it'd just allow you to prove the leaf twice, which has no effect given we don't update merkle roots.

0xleastwood commented 2 years ago

I believe the finding to be invalid because:

The actual issue highlighted by other wardens identifies a way for users to potentially resolve a merkle tree to a potentially valid tokenId by providing an intermediate issue. As there is no mention of this by the warden, I've decided to side with the sponsor.