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

User may collect infinite amount of NFTs

0x141414

Summary

The revert SantasList__AlreadyCollected() message is enforced using a balance check, resulting in easy manipulation to collect more than one NFT.

Vulnerability Details

In the external collectPresent() function, it is stated in 2 places that addresses should not be able to collect an NFT more than once (the natspec @notice comment on line 145, and the SantasList__AlreadyCollected revert message on line 152).

Despite the aforementioned rule, a malicious actor may bypass the balance check on line 151 by sending their previously minted NFT to a different wallet, and then called collectPresent() again. They may do this repeatedly and infinitely.

The below PoC (written as a forge test) demonstrates a NICE user repeatedly minting and transferring the NFT to an alternative wallet, resulting in a token balance of 1,000.

function test_infiniteCollectPresent() public {
SantasList.Status nice = SantasList.Status.NICE;
address user = makeAddr("user");
address userAlt = makeAddr("userAlt");
address santa = makeAddr("santa");
​
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
vm.startPrank(santa);
santasList.checkList(user, nice);
santasList.checkTwice(user, nice);
vm.stopPrank();
​
vm.startPrank(user);
​
for (uint i; i < 1_000; i++) {
santasList.collectPresent();
santasList.transferFrom(user, userAlt, i);
}
​
assert(santasList.balanceOf(userAlt) == 1_000);
}

Impact

Users may repeatedly mint many NFTs, potentially destroying the value of each token due to so much supply.

Tools Used

Forge

Recommendations

Use a mapping to track which addresses have collected their NFTs, rather than a balanceOf() check.

Make sure to update the mappings value for the person upon minting in both the collectPresent() and buyPresent() functions as well.

/*//////////////////////////////////////////////////////////////
STATE VARIABLES
//////////////////////////////////////////////////////////////*/
mapping(address person => bool) private personHasCollected;
// ... other variables
function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
if (personHasCollected[msg.sender]) {
revert SantasList__AlreadyCollected();
}
if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) {
personHasCollected[msg.sender] = true;
_mintAndIncrement();
return;
} else if (
s_theListCheckedOnce[msg.sender] == Status.EXTRA_NICE
&& s_theListCheckedTwice[msg.sender] == Status.EXTRA_NICE
) {
personHasCollected[msg.sender] = true;
_mintAndIncrement();
i_santaToken.mint(msg.sender);
return;
}
revert SantasList__NotNice();
}
function buyPresent(address presentReceiver) external {
personHasCollected[msg.sender] = true;
i_santaToken.burn(presentReceiver);
_mintAndIncrement();
}
Updates

Lead Judging Commences

inallhonesty Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

Weak Already Collected Check

Relying on balanceOf > 0 in collectPresent() allows the msg.sender to send their present to another address and then collect again.

Support

FAQs

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