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

Incorrect `buyPresent()` function logic implementation

naruto

Summary

The buyPresent() function allows any user to input the recipient's address, but the buyPresent() function burns the ERC20 tokens of the recipient to mint NFT tokens for msg.sender instead of burning msg.sender's ERC20 tokens to mint NFT tokens for the recipient."

Vulnerability Details

A user with both checks as EXTRA_NICE can receive 1 NFT token and 1e18 ERC20 tokens by collectPresent() function. An attacker can exploit the buyPresent() function by providing the address of a user with ERC20 tokens, burning the ERC20 tokens of that user, and minting NFT tokens for the attacker.

Users do not need to approve the SantaList contract to spend their own tokens because the burn permission allows the SantaList contract to burn users' tokens.

Impact

All users who own ERC20 tokens may be vulnerable to the attacker, who may burn the users' ERC20 tokens and mint NFTs for himself.

Test

function testPwnBuyPresent() public {
address user2 = makeAddr("user2");
testCollectPresentExtraNice();
​
assertEq(santasList.balanceOf(user2), 0);
assertEq(santaToken.balanceOf(user), 1e18);
​
vm.startPrank(user2);
santasList.buyPresent(user);
vm.stopPrank();
​
assertEq(santasList.balanceOf(user2), 1);
assertEq(santaToken.balanceOf(user), 0);
}

Recommendations

  • The _mintAndIncrement() function should add a parameter for the recipient's address.

  • The buyPresent() function should burn the ERC20 tokens of the caller and mint NFT tokens for the recipient.

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