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

Any `NICE` or `EXTRA_NICE` user is able to call `collectPresent` function multiple times.

tadev

Summary

collectPresent function is callable by any address, but the call will succeed only if the user is registered as NICE or EXTRA_NICE in SantasList contract. In order to prevent users to collect presents multiple times, the following check is implemented:

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

Nevertheless, there is an issue with this check. Users could send their newly minted NFTs to another wallet, allowing them to pass that check as balanceOf(msg.sender) will be 0 after transferring the NFT.

Vulnerability Details

Let's imagine a scenario where an EXTRA_NICE user wants to collect present when it is Christmas time. The user will call collectPresent function and will get 1 NFT and 1e18 SantaTokens. This user could now call safetransferfrom ERC-721 function in order to send the NFT to another wallet, while keeping SantaTokens on the same wallet (or send them as well, it doesn't matter). After that, it is possible to call collectPresent function again as ``balanceOf(msg.sender)will be0` again.

Impact

The impact of this vulnerability is HIGH as it allows any NICE user to mint as much NFTs as wanted, and it also allows any EXTRA_NICE user to mint as much NFTs and SantaTokens as desired.

Proof of Concept

The following tests shows that any NICE or EXTRA_NICE user is able to call collectPresent function again after transferring the newly minted NFT to another wallet.

  • In the case of NICE users, it will be possible to mint an infinity of NFTs, while transferring all of them in another wallet hold by the user.

  • In the case of EXTRA_NICE users, it will be possible to mint an infinity of NFTs and an infinity of SantaTokens.

function testExtraNiceCanCollectTwice() external {
vm.startPrank(santa);
// Santa checks twice the user as EXTRA_NICE
santasList.checkList(user, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(user, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
​
// It is Christmas time!
vm.warp(1_703_480_381);
​
vm.startPrank(user);
// User collects 1 NFT + 1e18 SantaToken
santasList.collectPresent();
// User sends the minted NFT to another wallet
santasList.safeTransferFrom(user, makeAddr("secondWallet"), 0);
// User collect present again
santasList.collectPresent();
vm.stopPrank();
​
// Users now owns 2e18 tokens, after calling 2 times collectPresent function successfully
assertEq(santaToken.balanceOf(user), 2e18);
}

Tools Used

Foundry

Recommendations

SantasList should implement in its storage a mapping to keep track of addresses which already collected present through collectPresent function.
We could declare as a state variable :

mapping(address user => bool) private hasClaimed;

and then modify collectPresent function as follows:

function collectPresent() external {
// use SantasList__AlreadyCollected custom error to save gas
require(!hasClaimed[msg.sender], "user already collected present");
​
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
​
if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) {
_mintAndIncrement();
hasClaimed[msg.sender] = true;
return;
} else if (
s_theListCheckedOnce[msg.sender] == Status.EXTRA_NICE
&& s_theListCheckedTwice[msg.sender] == Status.EXTRA_NICE
) {
_mintAndIncrement();
i_santaToken.mint(msg.sender);
hasClaimed[msg.sender] = true;
​
return;
}
revert SantasList__NotNice();
}

We just added a check that hasClaimed[msg.sender] is false to execute the rest of the function, while removing the check on balanceOf. Once present is collected, either for NICE or EXTRA_NICE people, we update hasClaimed[msg.sender] to true. This will prevent user to call collectPresent function.

If you run the previous test with this new implementation, it wail fail with the error user already collected present.

Here is a new test that checks the new implementation works as desired:

function testCorrectCollectPresentImpl() external {
vm.startPrank(santa);
// Santa checks twice the user as EXTRA_NICE
santasList.checkList(user, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(user, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
​
// It is Christmas time!
vm.warp(1_703_480_381);
​
vm.startPrank(user);
// User collects 1 NFT + 1e18 SantaToken
santasList.collectPresent();
// User sends the minted NFT to another wallet
santasList.safeTransferFrom(user, makeAddr("secondWallet"), 0);
​
vm.expectRevert("user already collected present");
santasList.collectPresent();
vm.stopPrank();
}
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.