First Flight #5: Santa's List

Summary

There is a possibility for a reentrancy attack in SantasList::collectPresent function because of the use of ERC721::_safeMint function in SantasList::_mintAndIncrement function.

Vulnerability Details

The use of ERC721::_safeMint() function in the SantasList.sol contract ensures that the receiving address msg.sender implements ERC721 functionality. But there is a reentrancy vulnerability in _safeMint() function. The function is titled _safeMint because it prevents tokens from being unintentionally minted to a contract by checking first whether that contract has implemented ERC721Receiver, i.e. marking itself as a willing recipient of NFTs. This all seems fine, but onERC721Received is an external call to the receiving contract, allowing arbitrary execution. The function _safeMint() awaits a callback from the recipient address. The function onERC721Received in the implementation of _safeMint() verifies the recipient’s callback function by comparing the return value to ERC721Receiver.onERC721Received.selector.
If the recipient address implements the functionality in a malicious manner, where tokens are transferred to another contract and then _safeMint() is called, the attacker can exploit this callback to get more tokens than is intended.

The function _safeMint() is called in function SantasList::_mintAndIncrement, which is called by a functions SantasList::collectPresent and SantasList::buyPresent. That gives an opportunity to malicious user that has status nice or extra nice to make reentrancy attack to collectPresent function.

Impact

The SantasList::collectPresent function is external without any access control and can be called by anyone, but only the people with status nice or extra nice can collect present. Since the collectPresent() function does not use the nonReenter modifier, the attacker can re-enter the function in the _safeMint function and collect more than intended tokens. Therefore, there is a high risk of a reentrancy attack.
Let's consider the following scenario that demonstrates this vulnerability:

1. Malicious user that has status nice or extra nice calls the collectPresent on SantasList contract.

2. The SantasList::collectPresent function is executed determining that the malicious user has for example status nice, and proceeds to mint an NFT using _mintAndIncrement function.

3. The _mintAndIncrement function internally calls ERC721::_safeMint function, which mints the NFT and then calls onERC721Received to ensure that the malicious user can receive ERC721 tokens.

4. Then the onERC721Received on malicious contract is called.

5. MaliciousContract's onERC721Received function is executed, which includes a transfer of the token to another address (to bypass the balanceOf check in the collectPresent function) and reentrant call back to SantasList's collectPresent.

6. If SantasList does not have proper reentrancy protection, the reentrant call to collectPresent is executed again, potentially allowing the malicious contract to mint another NFT before the first transaction is completed.

Tools Used

VS Code

Recommendations

Add reentrancy guard to the SantasList::collectPresent function.