Injection into the mintlist merkle tree

[code423n4]

Lines of code

https://github.com/code-423n4/2022-09-artgobblers/blob/2ae644ace932271fb34399c1c0771aabae2e423c/src/ArtGobblers.sol#L339 https://github.com/code-423n4/2022-09-artgobblers/blob/2ae644ace932271fb34399c1c0771aabae2e423c/src/ArtGobblers.sol#L347 https://github.com/transmissions11/solmate/blob/bff24e835192470ed38bf15dbed6084c2d723ace/src/utils/MerkleProofLib.sol#L8

Vulnerability details

Description

There is claimGobbler function in ArtGobblers contract. It accepts proof as an array of bytes32 values and uses such a proof for the check whether msg.sender is available to claim a gobbler. But there is no check on the length of the proof, so it is unclear that the following attack is not possible:

• attacker creates a merkle tree with many controlled by him addresses as leaves of such tree
• attacker receives an ability to claim just one gobbler
• attacker provides a root of the such tree as the hash of his address to the owner to be included into mintlist's merkle tree
• for each of the added controlled addresses attacker proves the validity of inclusion into the merkle tree by due to lengthening the standard "fair" proof by the part which proves inclusion into his own merkle tree of controlled accounts.

Actually, with the right coordination on the owner side, it is not a possible scenario because in MerkleProofLib library two nodes are hashed (so the length of hash preimage is 64 bytes), but for the leaf, there is a hash of the msg.sender address (so the length of leaf's hash preimage is 20 bytes). If the owner does not accept from the users their addresses (to later hash them and include as leaf values of the mintlist merkle tree) but instead accepts merkle tree leaf value this is a possible scenario.

PoC

Let's consider the following scenario. Attacker, which is included into mintlist, provides as a keccak256 of his address to the deployer side value v which is equal keccak256(abi.encodePacked(a1, a2)), where a1 = keccak256(abi.encodePacked(attacker_first_address)) and a2 = keccak256(abi.encodePacked(attacker_second_address)). After that, he just needs to append to the "fair" proof of including into such tree a keccak256(abi.encodePacked(attacker_first_address)) to prove including of attacker_second_address, and analogically for proving including of attacker_first_address.

Recommended Mitigation Steps

Add a comment regarding a possible attack scenario and make sure that those who will build such merkle tree will accept from the users, which should be included into mintlist, their account addresses (to be later hashed with keccak256 as a 20-bytes sequence), but will not accept merkle tree leaf values.

[Shungy]

This makes no sense.

[GalloDaSballo]

keccak256(abi.encodePacked(a1, a2))

You would need to code a scenario where you can get keccak256(abi.encodePacked(a1)) == keccak256(abi.encodePacked(a2))

The odds of finding a clash are 2^32-1, however if you can find one, next time we'll award a finding.

However, in this instance, I believe the finding to be invalid.

As a rule of thumb, for keccak related attacks, you should code an attack to avoid it getting disputed