Summary

poorly written check allowing NICE people to buy an infinite amount of NFT via collectPresent()

Vulnerability Details

• Actors:

  • Attacker: the malicious NICE person.

  • Victim: SantasList.

  • Protocol: The SantasList contract itself.

• Exploit Scenario:

  • Initial State: The Protocol is already deployed and the people are calling the collectPresent() function.

  • Step 1: The Attacker calls collectPresent() and gets his NFT.

  • Step 2: The Attacker calls collectPresent() and gets his NFT.

  • Step 3: The Attacker calls collectPresent() and gets his NFT.

  • Step 4: I can make up infinite number of steps like that, actually the NICE person can keep on calling collectPresent() and collect his NFTs as many times as he wants, because the check that was supposed to prevent NICE people from collecting an NFT more than once checks the santaToken's balance that is only minted by EXTRA-NICE people. Hence this check is irrelevant for NICE people. For a matter of fact, according to the documentation and comments, it is also irrelevant for EXTRA-NICE people since they can buy a present for someone else by burning their santaTokens, thus finding a way to set their balance to 0 and be able to call collectPresent() again. However, buyPresent() is badly implemented.

  • Outcome: NICE people can mint as many NFTs as they want.

Impact

This vulnerability makes it possible for everyone NICE to collect an unlimited amount of NFTs just by calling a function again and again. The whole protocol is broken.

Tools Used

Manual Analysis

Recommendations

Make the following changes: Declare a mapping in the storage: mapping(address person => bool collected) private s_theListCheckedCollection; Remove this condition: if (balanceOf(msg.sender) > 0) Add this one instead: if (s_theListCheckedCollection[msg.sender]) Keep the revert. Before the 2 return, set s_theListCheckedCollection[msg.sender] = true;