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 about 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.

Give us feedback!