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 allows malicious actors to steal unlimited NFTs

MikeDougherty

Summary

The SantasList::buyPresent() and SantaToken::burn() functions do not implement logic which would enforce the rules described in the natspec or the README. A malicious actor can burn a SANTA token from any holder to mint themselves an NFT.

Vulnerability Details

The SantasList::buyPresent() does not contain logic that would ensure the requirements are met:

  • does not spend the required 2 SANTA for the NFT

  • spends only 1 SANTA, but from the presentReceiver rather than the msg.sender

  • mints the NFT to the msg.sender rather than presentReceiver

Additionally, the SantaToken::burn() hard codes only 1 token for burning rather than providing the means to burn 2.

Adding the following test case to SantasListTest.t.sol demonstrates this behavior.

function testThiefCanStealNFT() public {
vm.startPrank(santa);
santasList.checkList(user, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(user, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
​
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
vm.startPrank(user);
santasList.collectPresent();
assertEq(santasList.balanceOf(user), 1, "user did not collect 1 NFT");
assertEq(santaToken.balanceOf(user), 1e18, "user did not collect 1 SANTA");
assertEq(santasList.ownerOf(0), user, "user owns NFT at index = 0");
​
address thief = makeAddr("thief");
vm.startPrank(thief);
​
santasList.buyPresent(user);
​
assertEq(santaToken.balanceOf(user), 0, "user does not have their 1 SANTA anymore");
assertEq(santasList.balanceOf(thief), 1, "thief did not steal NFT");
}

Impact

The SantasList::buyPresent() and SantaToken::burn() functions as they are written allow a malicious actor to learn the addresses of all SANTA holders with a balance of 1 or more and then mint themselves an NFT for each 1 SANTA token they can find, potentially receiving a large quantity of NFT.

Tools Used

Manual Review and Foundry

Recommendations

Update the logic of the SantasList::buyPresent() function to check that the msg.sender has a balance of 2 or more SANTA tokens, burn 2 SANTA and mint the NFT to the presentReceiver.

function buyPresent(address presentReceiver) external {
if (i_santaToken.balanceOf(msg.sender) < PURCHASED_PRESENT_COST) {
revert("Giver does not have enough SANTA");
}
i_santaToken.burn(msg.sender, PURCHASED_PRESENT_COST);
_safeMint(presentReceiver, s_tokenCounter++);
}

And the SantaToken::burn() function to burn SantasList::PURCHASED_PRESENT_COST SANTA tokens.

function burn(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.