Summary

With the current implementation of SantasList contract, any user is able to call the external buyPresent function:

This function calls SantaToken burn function, and finally mints an NFT on msg.sender address.
This means anyone can call this function, passing an address that owns some SantaTokens as parameter, and spend them to mint an NFT.

Kick off 48H finally decided that buyPresent function is only callable by SantaToken holders. Hence, this function lacks access control to make sure only token holders are able to make a present to someone else. It also lacks of check about the status of presentReceiver, which needs to be NAUGHTY or UNKNOWN (according to documentation).

Vulnerability Details

Logic of buyPresent function seems broken as it is currently implemented. Anyone is able to call this function, choosing any address that owns SantaTokens to mint themselves an NFT.

This function should :

• be callable only by SantaToken holders

• check that presentReceiver address is effectively registered as NAUGHTY or UNKNOWN

Without these checks, anyone can "steal" tokens from someone else, and use them to mint themselves NFTs.

Impact

The impact of this vulnerability is considered HIGH as buyPresent function allows any malicious user to use someone else's tokens to mint themselves an NFT. This is not the desired behaviour and can result in malicious actors using everyone's tokens while minting themselves many NFTs for free.

Proof of Concept

The following test shows that an attacker can pass any address that owns SantaTokens to buyPresent and mint themselves an NFT, using someone else's tokens.

Tools Used

Foundry

Recommendations

buyPresent function should be implemented differently to achieve the desired behaviour.

to recap, buyPresent function should :

• be callable only by SantaToken holders

• check that presentReceiver address is effectively registered as NAUGHTY or UNKNOWN

I suggest to implement buyPresent function as follows:

This implementation ensures that :

• only token holders can successfully call the function

• there is a check that presentReceiver is indeed NAUGHTY or UNKNOWN

• the function finally burns the token holder tokens and mints an NFT on presentReceiver address

Note that this implementation implies :

• modification of the Status enum, replacing NOT_CHECKED_TWICE by UNKNOWN, to respect documentation

• modification of _mintAndIncrement, allowing this function to take one address input parameter mintAddress

The following test shows that the correct implementation works as expected:

Note that the following line:
santasList.buyPresent(makeAddr("naughtyFriend")); // or unknnowFriend
will fail if you replace the input address by another address that is not registered as NAUGHTY or UNKNOWN.

But remember that all addresses have the first enum value by default. This means that if UNKNOWN is the first possible value in the enum, all addresses will by default be UNKNOWN and therefore eligible for present, which is the expected behavior. But if codebase keeps NICE as the default enum value, all address that Santa didn't check won't be eligible for present. (this issue is out of the scope of this finding).

Additionnal comment : natspec says "@dev You'll first need to approve the SantasList contract to spend your SantaTokens."
This comment seems wrong, as there is no need of approval to achieve this function. Indeed, SantasList contract will simply call SantaToken burn function to use the tokens of the caller. There is no need to use safetransferfrom or transferfrom functions. This burn function only checks that msg.sender is SantasList contract, and burns the tokens.
Once the token are burned ('spent by SantasList contract), _mintAndIncrement is called and the NFT is minted.
I suggest to remove this wrong comment.