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

Unchecked users are treated as NICE by default and can collect presents without being checked twice

hongphat19

Root + Impact

Description

The intended behavior is that only users checked twice by Santa as NICE or EXTRA_NICE can collect a present. The code relies on two mappings to store those statuses.

The Status enum declares NICE as the first value. In Solidity, the default value for an unset enum is its first value, which is numeric value 0. Unset mapping entries also return the default value. As a result, any address that has never been checked has both list values equal to Status.NICE.

enum Status {
// @> Default enum value is 0, so unset mapping entries are NICE
NICE,
EXTRA_NICE,
NAUGHTY,
NOT_CHECKED_TWICE
}
​
mapping(address person => Status naughtyOrNice) private s_theListCheckedOnce;
mapping(address person => Status naughtyOrNice) private s_theListCheckedTwice;
​
function collectPresent() external {
...
if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) {
// @> Unchecked users satisfy this branch by default
_mintAndIncrement();
return;
}
...
}

Risk

Likelihood:

  • This occurs for every address that has never been written to either status mapping.

  • After Christmas, any unchecked address can directly call collectPresent().

Impact:

  • Anyone can mint a present NFT without being approved by Santa.

  • The protocol's core eligibility rule, "must be checked twice", is bypassed.

Proof of Concept

Add this test to test/unit/SantasListTest.t.sol:

function testUncheckedUserCanCollectPresentBecauseDefaultStatusIsNice() public {
address uncheckedUser = makeAddr("uncheckedUser");
​
assertEq(uint256(santasList.getNaughtyOrNiceOnce(uncheckedUser)), uint256(SantasList.Status.NICE));
assertEq(uint256(santasList.getNaughtyOrNiceTwice(uncheckedUser)), uint256(SantasList.Status.NICE));
​
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME());
​
vm.prank(uncheckedUser);
santasList.collectPresent();
​
assertEq(santasList.balanceOf(uncheckedUser), 1);
}

The user was never checked by Santa, but both mappings return Status.NICE by default, allowing collection.

Recommended Mitigation

Make the default enum value an ineligible status, and require explicit Santa updates for eligible states.

enum Status {
+ UNKNOWN,
NICE,
EXTRA_NICE,
NAUGHTY,
NOT_CHECKED_TWICE
}

Alternatively, track whether each mapping entry has been explicitly set.

+mapping(address person => bool checkedOnce) private s_hasBeenCheckedOnce;
+mapping(address person => bool checkedTwice) private s_hasBeenCheckedTwice;
​
function checkList(address person, Status status) external onlySanta {
s_theListCheckedOnce[person] = status;
+ s_hasBeenCheckedOnce[person] = true;
emit CheckedOnce(person, status);
}

Then require both explicit flags in collectPresent().

​
Updates

Lead Judging Commences

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