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

Solidity default enum value of 0 maps to Status.NICE, allowing any unchecked address to collect presents

phylax

Solidity default enum value of 0 maps to Status.NICE, allowing any unchecked address to collect presents

Description

  • i_santa sets the status of a person twice to NAUGHTY, NICE, EXTRA_NICE, or UNKNOWN, through function checkListand checkTwice. Only if a persons status has been set 2 times to NICE or EXTRA_NICE can the person collect a NFT by calling collectPresent.

  • In Solidityvalues get initialized at 0and since NICE is the first position in the enum Statusany person who has not been checked by i_santawill automatically have the Status.NICEwhich makes them eligible to collect the NFT.

enum Status {
@> NICE,
EXTRA_NICE,
NAUGHTY,
NOT_CHECKED_TWICE // This should be UNKNOWN as described in docs
}
​

The collectPresentfunction checks:

if (s_theListCheckedOnce[msg.sender] == Status.NICE
&& s_theListCheckedTwice[msg.sender] == Status.NICE) {
_mintAndIncrement();
return;
}

Risk

Likelihood:

  • All addresses that has not been checked by i_santawill pass as Status.NICEmaking almost every address eligable to call collectPresent.

Impact:

  • All addresses that are not checked by is_santacan collect a NFT which inflates the NFT and makes it valueless. This negates the whole point of the protocol of only giving presents to NICEand EXTRA_NICEpersons. Protocol is broken.

Proof of Concept

A user successfully collects a NFT without being checked by i_santa. If not checked when calling collectPresentit will pass as Status.EXTRA_NICE:

function testUncheckedPersonCanCollectNFT() public {
// Fast forward time to after Christmas 2023
vm.warp(list.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
// User tries to collect NFT without being checked
// This should revert with SantasList__NotNice
vm.prank(user);
list.collectPresent();
​
// Asserts that the user collects the NFT successfully without being checked
assert(list.balanceOf(user) == 1);
}

Recommended Mitigation

Change positions in the enum Statusso first postion(position 0) is UNKNOWNso that it does not initialize to NICEbut instead UNKNOWN.

enum Status {
+ NOT_CHECKED_TWICE // This should be UNKNOWN as described in docs
NICE,
EXTRA_NICE,
NAUGHTY,
- NOT_CHECKED_TWICE // This should be UNKNOWN as described in docs
}
​
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 3 hours ago
Submission Judgement Published
Validated
Assigned finding tags:

[H-02] All addresses are considered `NICE` by default and are able to claim a NFT through `collectPresent` function before any Santa check.

## Description `collectPresent` function is supposed to be called by users that are considered `NICE` or `EXTRA_NICE` by Santa. This means Santa is supposed to call `checkList` function to assigned a user to a status, and then call `checkTwice` function to execute a double check of the status. Currently, the enum `Status` assigns its default value (0) to `NICE`. This means that both mappings `s_theListCheckedOnce` and `s_theListCheckedTwice` consider every existent address as `NICE`. In other words, all users are by default double checked as `NICE`, and therefore eligible to call `collectPresent` function. ## Vulnerability Details The vulnerability arises due to the order of elements in the enum. If the first value is `NICE`, this means the enum value for each key in both mappings will be `NICE`, as it corresponds to `0` value. ## Impact The impact of this vulnerability is HIGH as it results in a flawed mechanism of the present distribution. Any unchecked address is currently able to call `collectPresent` function and mint an NFT. This is because this contract considers by default every address with a `NICE` status (or 0 value). ## Proof of Concept The following Foundry test will show that any user is able to call `collectPresent` function after `CHRISTMAS_2023_BLOCK_TIME` : ``` function testCollectPresentIsFlawed() external { // prank an attacker's address vm.startPrank(makeAddr("attacker")); // set block.timestamp to CHRISTMAS_2023_BLOCK_TIME vm.warp(1_703_480_381); // collect present without any check from Santa santasList.collectPresent(); vm.stopPrank(); } ``` ## Recommendations I suggest to modify `Status` enum, and use `UNKNOWN` status as the first one. This way, all users will default to `UNKNOWN` status, preventing the successful call to `collectPresent` before any check form Santa: ``` enum Status { UNKNOWN, NICE, EXTRA_NICE, NAUGHTY } ``` After modifying the enum, you can run the following test and see that `collectPresent` call will revert if Santa didn't check the address and assigned its status to `NICE` or `EXTRA_NICE` : ``` function testCollectPresentIsFlawed() external { // prank an attacker's address vm.startPrank(makeAddr("attacker")); // set block.timestamp to CHRISTMAS_2023_BLOCK_TIME vm.warp(1_703_480_381); // collect present without any check from Santa vm.expectRevert(SantasList.SantasList__NotNice.selector); santasList.collectPresent(); vm.stopPrank(); } ```

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!