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

Poor implementation of buyPresent allows thief to mint NFT

rocknet

Summary

The buyPresent function is implemented contrary to the README (which says it requires 2e18 SantaToken), and the function comments, which says it should only be callable by those with SantaTokens AND that you'll need to approve SantasList to spend your SantaTokens.

Apart from the implementation seeming to be contrary to what's intended, it allows anyone to mint NFTs from an EXTRA_NICE user, which is very naughty.

Vulnerability Details

buyPresent does not check for token approval by itself, and requiring approval for calling burn on an ERC20 token is not part of the standard, so it's not in solmate. Further buyPresent does not check for any balance of SantaToken from msg.sender, nor does it burn any SantaToken from msg.sender. Instead, it burns the SantaToken from a selected victim, while sadly the parameter is called presentReceiver, this address is the victim, and the NFT receiver is msg.sender. There is a constant for the purchase cost, but it's never used. Note: if it were used, it should probably also be in SantaToken (or passed into it), so it didn't use magic numbers in SantaToken.mint and SantaToken.burn

Impact

The biggest issue here is that it allows anyone to mint NFTs from those that have 1e18 SantaToken, while burning the victim's 1e18 SantaToken.

There's a somewhat smaller impact in that the cost of the NFT is only 1e18, contrary to the stated 2e18. This is somewhat mitigated by the fact that only 1e18 SantaToken are issued to EXTRA_NICE users, so they could still (in theory) buy an NFT for a friend (if it weren't stolen before then as above).

Similar to the issue with the rug-pull user being able to transfer all SantaToken, thieves would be most effective at this by listening for the Transfer event on SantaToken, to be able to mint NFTs very quickly after the token were minted.

Tools Used

forge test. Wrote a test where the msg.sender a) doesn't have any SantaToken, b) didn't approve spend (no one did) c) presentReceiver didn't have 2e18 (contrary to README). The thief ended up with the NFT, and the EXTRA_NICE user ended up with just their own NFT.

address thief = makeAddr("thief");
​
modifier beExtraNice() {
vm.startPrank(santa);
santasList.checkList(user, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(user, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
_;
}
​
modifier collectPresent() {
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
vm.startPrank(user);
santasList.collectPresent();
vm.stopPrank();
_;
}
​
function testThiefCanStealNft() public beExtraNice collectPresent {
assertEq(1e18, santaToken.balanceOf(user));
assertEq(0, santasList.balanceOf(thief));
​
vm.startPrank(thief);
santasList.buyPresent(user);
vm.stopPrank();
​
assertEq(0, santaToken.balanceOf(user));
assertEq(1, santasList.balanceOf(thief));
assertEq(1, santasList.balanceOf(user));
}

Recommendations

  • In buyPresent, Check for required SantaToken balance from msg.sender, utilize the PURCHASED_PRESENT_COST constant.

  • Use the PURCHASED_PRESENT_COST constant in SantaToken as well, or pass in the value to the burn and mint functions.

  • Check for token approval before burning if that's what is intended

  • Burn SantaToken from msg.sender, not from presentReceiver

  • Don't call _mintAndIncrement, as that mints the NFT to the msg.sender, where the intention here is to mint to someone else, call _safeMint directly, passing presentReceiver as the to address.

  • Write tests for all intended functionality, as well as test proving that contrary conditions don't allow unintended functionality.

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.

Price is not enforced in buyPresent

This line indicates that the intended cost of presents for naughty people should be 2e18: https://github.com/Cyfrin/2023-11-Santas-List/blob/6627a6387adab89ae2ba2e82b38296723261c08a/src/SantasList.sol#L87 PURCHASE_PRESENT_COST should be implemented to enforce the cost of presents.

buyPresent should send to presentReceiver

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.