code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-arcade/blob/main/contracts/nft/ReputationBadge.sol#L111-L113

Vulnerability details

Impact

Minting will not work correctly when the associated Merkle Tree has multiple leaves with the same user/tokenId.

A use case of why Arcade.xyz might decide to use multiple leaves for the same user/tokenId is described in the mitigation of issue #386.

Proof of Concept

Consider the scenario where the root of a Merkle tree that contains two leaves for Bob, one for 15 tokens and another for 10 tokens.

In theory, the intent is that Bob should be able to claim a grand total of 25 tokens, but with the current implementation this is not possible.

Suppose that Bob has claimed the first 15 tokens from the first leaf. When he tries to claim the remaining 10 tokens, the following line will revert, as totalClaimable is now 10, but the amountClaimed is 15:

if (amountClaimed[recipient][tokenId] + amount > totalClaimable) {
    revert RB_InvalidClaimAmount(amount, totalClaimable);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Consider changing the verification logic so it's possible to associate multiple leaves with the same user.

If this feature is not necessary (for example if it's always possible to mint the totalClaimable in a single transaction), consider documenting it accordingly to avoid potential overrides.