Santa's List

## 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.