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

The `SantasList::buyPresent()` function burns a token from the intended present receiver and mints the NFT to the present giver

Turetos

Summary

The SantasList::buyPresent() burns a token of the address intended to be the receiver of the present, and the NFT is minted to the address of the caller.

Vulnerability Details

The SantasList::buyPresent() function is currently working inversely to its intended behavior.
It is burning a token from the address intended to receive the present, and the NFT is minted to the caller's address.
Furthermore, the documentation indicates that 2e18 should be burned, but only 1e18 is currently being burned.

Impact

Unintended burning tokens for a user could harm the protocol's reputation

The provided test highlights the undesirable behavior of the function.

//Set up
address presentGiver = makeAddr("presentGiver");
function testBuyPresentIncorrect() public {
console.log("Present Giver total nfts BEFORE : ", santasList.balanceOf(presentGiver));
​
vm.startPrank(address(santasList));
santaToken.mint(user);
vm.stopPrank();
​
console.log("Present receiver tokens BEFORE : ", santaToken.balanceOf(user));
​
vm.startPrank(presentGiver);
santasList.buyPresent(user);
vm.stopPrank();
console.log("Present Giver total nfts AFTER : ", santasList.balanceOf(presentGiver));
console.log("Present receiver tokens AFTER : ", santaToken.balanceOf(user));
}

Output

Present Giver total nfts BEFORE : 0
Present receiver tokens BEFORE : 1000000000000000000
Present Giver total nfts AFTER : 1
Present receiver tokens AFTER : 0

Tools Used

Foundry

Recommendations

  1. The SantasList::buyPresent() function is intended to burn 2e18 tokens from the msg.sender and mint the NFT to the specified presentReceiver.

- function buyPresent(address presentReceiver) external {
- i_santaToken.burn(presentReceiver);
- _mintAndIncrement();
- }
​
+ function buyPresentFixed(address presentReceiver) external {
+ i_santaToken.burnWithAmount(msg.sender, 2e18);
+ _safeMint(presentReceiver, s_tokenCounter++);
+ }

  1. Add the following function to the SantaToken contract: a parameterized burning function that receives the amount to be burned.

+ function burnWithAmount(address from, uint256 amount) external {
+ if (msg.sender != i_santasList) {
+ revert SantaToken__NotSantasList();
+ }
+ _burn(from, amount);
+ }
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.

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.