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

Incorrect implementation of `buyPresent()`

t0x1c

Summary

Incorrect implementation of buyPresent() causes presentReceiver to lose tokens.

Vulnerability Details

buyPresent() burns the tokens of presentReceiver instead of the msg.sender. Effectively the msg.sender receives free NFT by spending presentReceiver's tokens, if presentReceiver had approved the SantasList contract to spend their SantaTokens.

Also, only 1e18 tokens are burnt. It should be 2e18 as defined here.
Note: This constant PURCHASED_PRESENT_COST goes unused in the code. (Added in my recommendation)

function test_t0x1c_buyPresent() public {
vm.startPrank(santa);
santasList.checkList(user, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(user, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
​
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
address presentReceiver = makeAddr("PresentReceiver");
deal(address(santaToken), presentReceiver, 1e18);
​
vm.startPrank(presentReceiver);
santaToken.approve(address(santasList), type(uint256).max);
​
changePrank(user);
santasList.collectPresent();
assertEq(santasList.balanceOf(user), 1, "user balance NFT mismatch1");
assertEq(santaToken.balanceOf(user), 1e18, "user balance Token mismatch1");
assertEq(santasList.balanceOf(presentReceiver), 0);
assertEq(santaToken.balanceOf(presentReceiver), 1e18);
​
santasList.buyPresent(presentReceiver);
// incorrect balances seen
assertEq(santaToken.balanceOf(presentReceiver), 0, "user balance Token mismatch2"); // all spent
assertEq(santaToken.balanceOf(user), 1e18); // still the same
assertEq(santasList.balanceOf(user), 1 + 1); // got NFT
assertEq(santasList.balanceOf(presentReceiver), 0); // did not get NFT
vm.stopPrank();
}

Impact

Instead of receiving a present, the presentReceiver loses santa tokens.

Tools Used

Manual inspection

Recommendations

function buyPresent(address presentReceiver) external {
- i_santaToken.burn(presentReceiver);
+ uint256 balanceBefore = i_santaToken.balanceOf(msg.sender);
+ i_santaToken.burn(msg.sender); // 1e18 burnt
+ i_santaToken.burn(msg.sender); // another 1e18 burnt
+ require(i_santaToken.balanceOf(msg.sender) == balanceBefore - PURCHASED_PRESENT_COST);
_mintAndIncrement();
santasList.transferFrom(msg.sender, presentReceiver, s_tokenCounter- 1);
}
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.