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

buyPresent is wired to the wrong parties — it burns the receiver's SantaTokens and mints the NFT to the caller, so anyone can destroy any holder's tokens and mint themselves a free NFT

hopeh

Description

buyPresent is meant to let a token holder spend their own SantaToken to gift an NFT to someone else ("Buy a present for someone else ... trades 2e18 of SantaToken for an NFT"). The implementation is wired to the wrong parties in both directions:

function buyPresent(address presentReceiver) external {
i_santaToken.burn(presentReceiver); // burns the RECEIVER's tokens
_mintAndIncrement(); // mints the NFT to msg.sender
}
​
function _mintAndIncrement() private {
_safeMint(msg.sender, s_tokenCounter++);
}

And SantaToken.burn destroys tokens from whatever address SantasList passes, with no allowance check — the caller of buyPresent never needs the receiver's approval:

function burn(address from) external {
if (msg.sender != i_santasList) {
revert SantaToken__NotSantasList();
}
_burn(from, 1e18); // unconditional burn of `from`'s balance
}

So a call to buyPresent(victim):

  1. Burns the victim's SantaToken (the presentReceiver argument is used as the burn source), not the caller's.

  2. Mints the NFT to msg.sender (the caller), not to presentReceiver.

The payer and the recipient are inverted. The caller pays nothing from their own balance, receives the NFT themselves, and destroys an arbitrary chosen holder's tokens. There is no access control and, crucially, no transferFrom/allowance involved — burn is a direct privileged _burn, so the victim's prior "approve the SantasList contract" (which the README instructs users to do) is irrelevant; any holder can be targeted whether or not they approved anything.

This is a direct theft/griefing primitive: an attacker repeatedly calls buyPresent(victim) to drain a victim's entire SantaToken balance in 1e18 chunks while minting themselves free NFTs.

Risk

Impact: High. Any account can (a) destroy any other user's SantaToken balance without consent and (b) mint itself the NFT for free. Both the token accounting and the "gift to someone else" semantics are fully broken, and honest holders' balances are exposed to unconstrained burning.

Likelihood: High. Single unprivileged call with an attacker-chosen presentReceiver; no approval, no status, and no cost required.

Proof of Concept

function test_buyPresentBurnsVictimAndMintsToAttacker() public {
// give the victim some SantaTokens (e.g. they were EXTRA_NICE and collected).
address victim = makeAddr("victim");
address attacker = makeAddr("attacker");
_fundSantaTokens(victim, 2e18); // helper: victim holds 2 SANTA
​
uint256 victimBefore = santaToken.balanceOf(victim);
uint256 attackerNftPre = santasList.balanceOf(attacker);
​
// attacker "buys a present" naming the victim — attacker spends nothing of their own
vm.prank(attacker);
santasList.buyPresent(victim);
​
// victim's tokens were burned; attacker got the NFT for free
assertEq(santaToken.balanceOf(victim), victimBefore - 1e18);
assertEq(santasList.balanceOf(attacker), attackerNftPre + 1);
assertEq(santaToken.balanceOf(attacker), 0); // attacker never paid
​
// repeat to drain the rest of the victim's balance
vm.prank(attacker);
santasList.buyPresent(victim);
assertEq(santaToken.balanceOf(victim), 0);
}

Expected: the caller spends their own 2e18 and presentReceiver gets the NFT. Actual: the receiver's tokens are burned and the caller gets the NFT for free.

Recommended Mitigation

Charge the caller and deliver the NFT to the receiver — the addresses must swap. Also require the caller actually holds the cost:

function buyPresent(address presentReceiver) external {
i_santaToken.burn(msg.sender); // caller pays
_safeMint(presentReceiver, s_tokenCounter++); // receiver gets the present
}

(See the companion finding on the burn amount: SantaToken.burn should destroy PURCHASED_PRESENT_COST = 2e18, not a hardcoded 1e18.) If unconditional burning is undesirable, switch SantaToken.burn to pull via transferFrom/allowance from msg.sender so a spend cannot be forced on an address that did not consent.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 2 hours ago
Submission Judgement Published
Validated
Assigned finding tags:

[H-03] SantasList::buyPresent burns token from presentReceiver instead of caller and also sends present to caller instead of presentReceiver.

## Description 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. ```cpp /* * @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: ```cpp forge test --mt test_AttackerCanMintNft_ByBurningTokensOfOtherUsers ``` ```cpp 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. ## Recommendations - Burn the SantaToken from the caller i.e., `msg.sender` - Mint NFT to the `presentReceiver` ```diff + 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.

Support

FAQs

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

Give us feedback!