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

`SantasList::collectPresent()` uses the transferable `balanceOf(msg.sender)` as its already-collected check, allowing a user to mint unlimited presents and SantaToken by transferring each NFT away before recollecting

au6695

SantasList::collectPresent() uses the transferable balanceOf(msg.sender) as its already-collected check, allowing a user to mint unlimited presents and SantaToken by transferring each NFT away before recollecting

Description

SantasList::collectPresent() is intended to be callable once per eligible address. It enforces this by checking whether the caller already holds an NFT:

if (balanceOf(msg.sender) > 0) {
revert SantasList__AlreadyCollected();
}

balanceOf here is the inherited ERC721 balance, which is not a record of whether the caller has ever collected — it is a record of what the caller currently holds. Because SantasList places no restriction on transfers, the caller can move the minted NFT to any other address they control, returning their balance to zero and making the check pass again. An SantasList::EXTRA_NICE caller can therefore repeat collect-and-transfer without bound, receiving a new NFT and a further 1e18 SantaToken on every pass:

Risk

Likelihood:

  • The exploit requires no privileged role, no specific state beyond ordinary eligibility, and no timing window — only that the caller has been marked nice and that SantasList::CHRISTMAS_2023_BLOCK_TIME has passed.

  • Every legitimately eligible user is able to perform it, and each has a direct financial incentive to do so. Detection is unlikely to deter anyone, as the transfers are indistinguishable from normal ERC721 activity.

  • The receiving addresses cost nothing to create, so the only bound on repetition is gas.

Impact:

  • NFT supply is inflated without limit. s_tokenCounter advances on every pass, so a single caller can mint an arbitrary number of presents against a scheme meant to issue one each.

  • SantaToken supply is inflated without limit on the SantasList::EXTRA_NICE branch, breaking a cap that was otherwise structural rather than enforced.
    The inflated token balance flows into SantasList::buyPresent(), letting one address purchase arbitrarily many additional presents and further compounding NFT inflation.

Proof of Concept

The code below shows the bypass of the already collected check using transfer and how unbounded mints occur

function test_alreadyCollectedCheck_isBypassedByTransfer() public {
address sink = makeAddr("sink");
​
vm.startPrank(santa);
santasList.checkList(user, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(user, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
​
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME());
assertEq(santasList.balanceOf(user), 0);
​
vm.startPrank(user);
santasList.collectPresent();
​
// The guard works while the NFT is held.
vm.expectRevert(SantasList.SantasList__AlreadyCollected.selector);
santasList.collectPresent();
​
// Moving the NFT away defeats it.
santasList.transferFrom(user, sink, 0);
santasList.collectPresent();
vm.stopPrank();
​
assertEq(santaToken.balanceOf(user), 2e18);
}
​
function test_transferBypass_allowsUnboundedMinting() public {
address sink = makeAddr("sink");
uint256 rounds = 10;
​
vm.startPrank(santa);
santasList.checkList(user, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(user, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
​
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME());
assertEq(santasList.balanceOf(user), 0);
​
vm.startPrank(user);
for (uint256 i = 0; i < rounds; i++) {
santasList.collectPresent();
santasList.transferFrom(user, sink, i); // ids sequential from 0
}
vm.stopPrank();
​
assertEq(santasList.balanceOf(sink), rounds);
assertEq(santaToken.balanceOf(user), rounds * 1e18);
assertEq(santasList.balanceOf(user), 0);
}
​

Recommended Mitigation

Keep a state variable s_hasCollected and mark as collected before minting to follow CEI

+ mapping(address => bool) private s_hasCollected;
+
+ error SantasList__AlreadyCollected();
​
function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
- if (balanceOf(msg.sender) > 0) {
+ 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) {
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!