First Flight #5: Santa's List

Summary

The logic in SantasList::buyPresent function is wrong and it burns a SantaToken from the presentReceiver instead of the caller (msg.sender) and the new NFT is present for the caller not for the presentReceiver. Also, the amount of SantaToken::burn function is hardcoded to 1e18 that may be in conflict with the intended behaviour of the protocol, if the NAUGHTY or all people should pay 2e18 of SantaToken for an NFT.

Vulnerability Details

In the notice for SantasList::buyPresent is written: @notice Buy a present for someone else. This should only be callable by anyone with SantaTokens. and in the documentation for this functions is written: A function that trades 2e18 of SantaToken for an NFT. This function can be called by anyone. The people which have SantaToken are these that have status EXTRA_NICE. The function buyPresent can be called by anyone and does not check if the caller has SantaToken or he is EXTRA_NICE or NAUGHTY. Additionally, the function burns a SantaToken from the presentReceiver instead of the caller (msg.sender) and the new NFT is present for the caller not for the presentReceiver. This is likely not the intended behaviour, as the intention seems to be to allow people to buy presents for others at the cost of their own tokens and maybe the NAUGHTY people to pay more because ot these lines which contradict to the docs:

Also, in SantaToken::burn function the amount in _burn function is hardcoded to 1e18. Therefore, the cost for the present will be for all people 1e18. If the comments in the codebase are the intended behaviour the NAUGHTY people should pay more than the other - 2e18. If the documentation is the intended behaviout all people should pay 2e18.

Impact

The logic in SantasList::buyPresent is wrong. The caller of the function should pay with his SantaToken and the input parameter presentReceiver should receive NFT. But the implementation of the function lets the presentReceiver to pay for the present for the caller of the function. That allows people without SantaToken to call the function and buy theirself NFT using the address of person with SantaToken as presentReceiver.

Let's consider the following scenario:

1. Bob is a naughty person and has no SantaToken.

2. Alice is a extra nice person and has SantaToken.

3. Bob calls SantasList::buyPresent function with input argument Alice's address.

4. The buyPresent function is executed and Alice pays with her SantaToken for the Bob's NFT.

The following test function demonstrates this scenario:

You can add the testBuyPresentFromSomeoneElse function to the file SantasListTest.t.sol and execute it using the following command: forge test --match-test testBuyPresentFromSomeoneElse.

Also, the hardoceded amount in SantaToken::burn function allows the NAUGHTY people to pay less than the intended by the code behaviour.

Tools Used

VS Code, Foundry

Recommendations

If the intended behaviour is someone with SantaToken to buy NFT for someone else, add a check if the caller has SantaToken and rewrite the function to burn SantaToken from the caller address and add the NFT to the receivePresent address. Also, rewrite the documentation and comments in the codebase. In the current version there are several places with contradictions and that leads to confusion.
Additionally, if we consider that the NAUGHTY person has SantaToken and should pay more than the other add a check that the caller is NAUGHTY and change the function SantaToken::burn to accept amount of 2e18, not to be hardcoded. For the other people it should be 1e18. If all people should pay 2e18 as the documentation said, the amount in SantaToken::burn function is wrong again and should be modified.