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

Mapping value enum `SantasList::Status` default zero value is `SantasList::Status::NICE`, person who never get checked could get to collect present

hmpippo

Mapping value enum SantasList::Status default zero value is SantasList::Status::NICE, person who never get checked could get to collect present

Description

  • If Santa does not check someone on s_theListCheckedOnce, that person's status will be NICE by default!!! That's because the mapping mapping(address person => Status naughtyOrNice) value type is a enum of which is a uint under hood, which will be 0 by default if the key is absent. And 0 is Status::NICE in this case.

  • Also, SantasList::checkTwice function do not function as intended, it does not check if ppl already been marked on the s_theListCheckedOnce

enum Status {
@> NICE, // 0 is default value, so if someone never been checked, they be nice???
EXTRA_NICE,
NAUGHTY,
NOT_CHECKED_TWICE
}
...
@> function checkTwice(address person, Status status) external onlySanta {
if (s_theListCheckedOnce[person] != status) {
revert SantasList__SecondCheckDoesntMatchFirst();
}
s_theListCheckedTwice[person] = status;
emit CheckedTwice(person, status);
}

Risk

Likelihood: High

  • Any user who never get checked by Santa can just go collect the present!!!

Impact: High

  • Any user who never get checked by Santa can just go collect the present, which is severe reward issue!!!

Proof of Concept

  1. User not checked go collect the present directly

  2. Check the collectPresent function if succeed and the NFT balance of user

function testUserNeverCheckedCanCollectPresent() public {
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
vm.startPrank(user);
santasList.collectPresent();
vm.stopPrank();
​
assertEq(santasList.balanceOf(user), 1);
}

Recommended Mitigation

Put a NOT_CHECKED at the first position of enum Status, and check the NOT_CHECKED status within function SantasList::checkTwice:

enum Status {
+ NOT_CHECKED,
NICE,
EXTRA_NICE,
NAUGHTY,
NOT_CHECKED_TWICE
}
...
function checkTwice(address person, Status status) external onlySanta {
+ if (s_theListCheckedOnce[person] == Status.NOT_CHECKED) {
+ revert SantasList__StatusNotChecked();
+ }
- if (s_theListCheckedOnce[person] != status) {
- revert SantasList__SecondCheckDoesntMatchFirst();
- }
s_theListCheckedTwice[person] = status;
emit CheckedTwice(person, status);
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 23 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!