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

Incorrect handling of uninitialized Status enum leading to unauthorized access

0xrektified

Summary

The checkTwice and collectPresent methods in the SantasList contract are designed to check if users have a Status.NICE value in s_theListCheckedOnce and s_theListCheckedTwice.
However, since Status.NICE defaults to 0 and uninitialized enum values in Solidity also default to 0, these checks incorrectly pass for users whose status has not been explicitly set.

Vulnerability Details

The Status enum is defined as:

enum Status {
NICE, // <-- 0
EXTRA_NICE, // <-- 1
NAUGHTY, // <-- 2
NOT_CHECKED_TWICE // <-- 3
}

The issue arises because in Solidity, uninitialized enum values default to the first item, which in this case is NICE (0). This leads to incorrect behavior in the following scenarios:

When calling checkTwice: If Santa did not explicitly set a user's status for s_theListCheckedOnce, the user's status defaults to NICE.

When calling collectPresent: Users without an explicitly set status are treated as if they have NICE status and are allowed to collect presents.

To demonstrate this vulnerability, copy the following PoC into the SantasListTest.t.sol file and execute the command:

forge test -vvvv --match-test testUnitiliazedValueWillPassTheCheck
function testUnitiliazedValueWillPassTheCheck() public {
address user2 = makeAddr("user2");
​
// Santa forgets to set the value for checklist ONE
vm.startPrank(santa);
// Check twice passes as Status.NICE is equal to zero
santasList.checkTwice(user, SantasList.Status.NICE);
vm.stopPrank();
​
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
// Half-initialized values pass and the user can collect Present
vm.startPrank(user);
santasList.collectPresent();
assertEq(santasList.balanceOf(user), 1);
vm.stopPrank();
​
// Non-initialized values pass and the user can collect Present
vm.startPrank(user2);
santasList.collectPresent();
assertEq(santasList.balanceOf(user2), 1);
vm.stopPrank();
}

This test shows that both half-initialized and non-initialized users can collect presents, but they should not be able to.

Impact

This vulnerability is of high severity as it allows users who have not been properly checked or categorized by Santa to collect presents, undermining the contract's intended logic.

Tools Used

Forge testing framework

Recommendations

Adjust the Enum: Start the Status enum with an UNINITIALIZED value to clearly indicate an uninitialized status.

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

Update Logic: Modify checkTwice and collectPresent methods to handle the new UNINITIALIZED status correctly.

function checkTwice(address user, Status status) external {
+ require(status != Status.UNINITIALIZED, "Status must be initialized"); // Optional to avoid setting it twice to 0
if (s_theListCheckedOnce[person] != status) {
revert SantasList__SecondCheckDoesntMatchFirst();
}
s_theListCheckedTwice[person] = status;
emit CheckedTwice(person, status);
}
​
function collectPresent() external {
+ require(s_theListCheckedOnceFixed[msg.sender] != StatusFixed.UNINITIALIZED &&+
+ s_theListCheckedTwiceFixed[msg.sender] != StatusFixed.UNINITIALIZED, "User status must be initialized"); // Optional
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
if (balanceOf(msg.sender) > 0) {
revert SantasList__AlreadyCollected();
}
if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) {
_mintAndIncrement();
return;
} else if (
s_theListCheckedOnce[msg.sender] == Status.EXTRA_NICE
&& s_theListCheckedTwice[msg.sender] == Status.EXTRA_NICE
) {
_mintAndIncrement();
i_santaToken.mint(msg.sender);
return;
}
revert SantasList__NotNice();
}

Test the Fix: Implement tests to ensure that users with UNINITIALIZED status cannot pass the checks in checkTwice and collectPresent.

function testUnitiliazedValueWillPassTheCheckFixed() public {
address user2 = makeAddr("user2");
​
vm.startPrank(santa);
// Santa is not able to check twice as s_theListCheckedOnce is not defined
vm.expectRevert();
santasList.checkTwice(user, SantasList.Status.NICE);
​
// Santa set the CheckList once to test the flow
santasList.checkList(user, SantasList.Status.NICE);
vm.stopPrank();
​
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
vm.startPrank(user);
// User is not able to claim as only the first check is set
vm.expectRevert();
santasList.collectPresent();
​
// User balance is zero
assertEq(santasList.balanceOf(user), 0);
vm.stopPrank();
​
vm.startPrank(user2);
// uninitialized user are not able to claim
vm.expectRevert();
santasList.collectPresent();
​
// User2 balance is zero
assertEq(santasList.balanceOf(user), 0);
vm.stopPrank();
​
vm.startPrank(santa);
// Santa is able to set checkTwice as s_theListCheckedOnce is defined and set to NICE
santasList.checkTwice(user, SantasList.Status.NICE);
vm.stopPrank();
​
vm.startPrank(user);
// User is able to collect Present
santasList.collectPresent();
assertEq(santasList.balanceOf(user), 1);
vm.stopPrank();
}

By making these changes, the contract will properly prevent uninitialized users from collecting presents, ensuring the integrity and intended functionality of the contract.

Updates

Lead Judging Commences

inallhonesty Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

default status is nice

In Solidity the first element of an enum is the default value. In Santa's List, the means each person is mapped by default to 'NICE'.

Support

FAQs

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