First Flight #5: Santa's List

Summary

collectPresent() has the following check to ensure the msg.sender is assigned the NICE member of the Status enum:

The problem is that every person in the s_theListCheckedOnce mapping is assigned the NICE enum member by default, meaning they will pass the above check allowing them to mint an NFT.

Vulnerability Details

As per the Solidity documentation:

Enums require at least one member, and its default value when declared is the first member.

Looking at the Status enum:

We can see that NICE is the first member and therefore is the default value. Let’s take a look at the s_theListCheckedOnce mapping:

There is a key (address person) that maps to a value (Status naughtyOrNice). By default the value will be NICE for the reason previously stated. Now when someone calls collectPresent() and the following check is executed:

It passes as the person, i.e. the msg.sender, is assigned the NICE member of the Status enum by default.

Impact

Users do not need to be assigned the NICE member of the Status enum and therefore can call collectPresent() to mint an NFT. Imagine a scenario whereby Sam Bankman-Fried (SBF) is able to mint an NFT because he was assigned the NICE member by default. I believe the general consensus would be against such a scenario as we can all agree such individuals are more suited to the NAUGHTY member.

Tools Used

Manual Review.

Recommendations

Add an additional member to the Status enum and ensure it is the first member. For instance, adding UNKNOWN to Status as the first member will ensure the default value that the s_theListCheckedOnce and s_theListCheckedTwice mappings map to is UNKNOWN: