Groupbuy: Construction of merkle tree allows some unintended IDs to be bought

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tessera/blob/1e408ebc1c4fdcc72678ea7f21a94d38855ccc0b/src/modules/GroupBuy.sol#L188

Vulnerability details

Impact

In GroupBuy.purchase, when no proof is provided, it is required that the provided token ID is equal to the stored merkleRoot:

if (_purchaseProof.length == 0) {
            // Hashes tokenId to verify merkle root if proof is empty
            if (bytes32(_tokenId) != merkleRoot) revert InvalidProof();
}

This makes sense because the token ID is stored in the variable merkleRoot directly when only one ID is provided. However, the problem is that a user can also provide no merkle proof when merkleRoot does not contain the token ID of a single token, but a valid merkle root. In that case, the user can mint the token with the ID of the merkle root (interpreteted as a uint256), although that was never intended. Because token IDs are arbitrary uint256 and do not have to be consecutive, this can be a valid token ID.

Note on severity: The issue is very similar (or basically the same) as M-01 in the Seaport contest, where cmichel provided the following reasoning for the severity:

One might argue that this attack is not feasible because the provided hash is random and tokenIds are generally a counter. However, this is not required in the standard.
“While some ERC-721 smart contracts may find it convenient to start with ID 0 and simply increment by one for each new NFT, callers SHALL NOT assume that ID numbers have any specific pattern to them, and MUST treat the ID as a ‘black box’.” EIP721
Neither do the standard OpenZeppelin/Solmate implementations use a counter. They only provide internal _mint(address to, uint256 id) functions that allow specifying an arbitrary id. NFT contracts could let the user choose the token ID to mint, especially contracts that do not have any linked off-chain metadata like Uniswap LP positions.
Therefore, ERC721-compliant token contracts are vulnerable to this attack.

Proof Of Concept

We assume that the root of the merkle tree for some provided token IDs is 0xDEADBEEF.. When calling purchase, the user can now provide an empty _purchaseProof and use uint256(0xDEADBEEF..) as _tokenId. This allows him to buy a token ID that the creator never whitelisted (uint256(0xDEADBEEF..)).

Recommended Mitigation Steps

Also hash a single token ID before storing it in merkleRoot (and before the check in purchase).

[c4-judge]

HickupHH3 marked the issue as satisfactory

[c4-judge]

HickupHH3 marked the issue as primary issue

[c4-sponsor]

stevennevins marked the issue as disagree with severity

[0x0aa0]

While this is technically correct we do not see it as an issue. For this scenario to play out the contract targeted by a pool would need to allow users or an owner to mint arbitrary ids, which I would say is not the case for the large majority of projects that would be suited to a group buy. Even in projects such as ENS where token ids are not sequential the issue would still require a hash collision between the root and a user input.

[HickupHH3]

While I agree that it's extremely unlikely for the merkleRoot for multiple token IDs to be itself a valid tokenID, the attack vector is the same as the Seaport finding which was given a medium severity rating.

[C4-Staff]

liveactionllama marked the issue as duplicate of #14