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

SantasList::Status.NICE equates to the same as the default value of an enum which undermines the lists

datadata

Root + Impact

Description

  • The normal behavior of an enum is that it represents one of several unique elements, which, when passed to uint256(), can be represented as a number from 0 to the length - 1 of the list of different characteristics.

  • The problem is that this enum Status has the element NICE in the 0th position, and therefore, when passed to uint256(), will return 0. The problem arises when we test an unadded address against the mapping s_theListCheckedOnce, because if the mapping does not have a given value for an address, it will return a default Status enum, and the default characteristic of an enum is its 0th position, which returns 0 when passed to uint256(). Meaning that all addresses are considered NICE unless specified by callingcheckList()on that address.

enum Status {
@> NICE,
EXTRA_NICE,
NAUGHTY,
NOT_CHECKED_TWICE
}

Risk

Likelihood:

  • This risk is ever present, because an enum's default value is always its 0th value, and in this case, Status's default value is NICE.

  • Because mappings conceptually point to all values of given types, this mapping now assumes that all addresses are NICE.

Impact:

  • With all addresses inherently set to NICE, the entire process of Santa needing to first add an address to mapping s_theListCheckedOnce via the function checkList() is undermined. Now, all addresses are eligible to be added to mapping s_theListCheckedTwice, because they have already been approved by having a Status of NICE in s_theListCheckedOnce.

  • Users or devs checking a particular address's Status in s_theListCheckedOnce is now unsure if the address has been added by checkList() or not, unless the Status differs from NICE.

Proof of Concept

Below, we see that two users, user2 and user3, start with a Status of NICE by the default behavior of an enum being its 0th value. After calling checkList() on user2 and giving them a Status of NICE explicitly, we see that afterwards user2's status is equivalent to user3's Status, which was never explicitly given with checkList().

// Testing via Foundry
​
// Setup for tests
// SPDX-License-Identifier: MIT
pragma solidity 0.8.22;
​
import {SantasList} from "../../src/SantasList.sol";
import {SantaToken} from "../../src/SantaToken.sol";
import {Test} from "forge-std/Test.sol";
import {console2} from "forge-std/console2.sol"; // added this
import {_CheatCodes} from "../mocks/CheatCodes.t.sol";
​
contract SantasListTest is Test {
SantasList santasList;
SantaToken santaToken;
​
address user = makeAddr("user");
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
address santa = makeAddr("santa");
_CheatCodes cheatCodes = _CheatCodes(HEVM_ADDRESS);
​
function setUp() public {
vm.startPrank(santa);
santasList = new SantasList();
santaToken = SantaToken(santasList.getSantaToken());
vm.stopPrank();
}
​
// Test that anyone can call checkList() and modify s_theListCheckedOnce
function testThatDefaultIsNice() public {
// Arrange
// NICE = 0; EXTRA_NICE = 1; NAUGHTY = 2; NOT_CHECKED_TWICE = 3;
uint256 user2StatusBefore = uint256(santasList.getNaughtyOrNiceOnce(user2));
uint256 user3StatusBefore = uint256(santasList.getNaughtyOrNiceOnce(user3));
console2.log("user2 status before: %", user2StatusBefore);
console2.log("user3 status before: %", user3StatusBefore);
​
// Act
vm.startPrank(santa);
santasList.checkList(user2, SantasList.Status.NICE);
vm.stopPrank();
​
// Assert
uint256 user2StatusAfter = uint256(santasList.getNaughtyOrNiceOnce(user2));
uint256 user3StatusAfter = uint256(santasList.getNaughtyOrNiceOnce(user3));
assertEq(user2StatusAfter, uint256(SantasList.Status.NICE));
assertEq(user2StatusAfter, user3StatusAfter);
}
}

Recommended Mitigation

We suggest that a different value be provided as the 0th value. We place NOT_CHECKED_TWICE as the 0th position, and NICE as the 1st. NOT_CHECKED_TWICE is deleted from the position after NAUGHTY, which now no longer has a comma trailing after it.

​
enum Status {
- NICE,
+ NOT_CHECKED_TWICE,
+ NICE,
EXTRA_NICE,
- NAUGHTY,
+ NAUGHTY
- NOT_CHECKED_TWICE
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 12 days 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!