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

Default value of `Status` enum is `NICE`

0xbrivan2

Summary

The zero-value of the Status enum is NICE

Vulnerability Details

the default value of an enum is its first element (the value 0), and if a person's status has not been explicitly set, it will be considered as NICE. This means that anyone can potentially make actions restricted to NICE people if their status has not been explicitly set.

Impact

The vulnerability in the code has two critical implications. The first one pertains to the validation process in the checkTwice function:

function checkTwice(address person, Status status) external onlySanta {
if (s_theListCheckedOnce[person] != status) {
revert SantasList__SecondCheckDoesntMatchFirst();
}
s_theListCheckedTwice[person] = status;
emit CheckedTwice(person, status);
}

In cases where Santa intends to perform a second check on a person with a status of NICE, there is a potential vulnerability. The check ensuring that the person has been previously checked can be inadvertently bypassed if their status was not explicitly set. This poses a risk that a person can be checked twice without being checked at the first time.

The second concern is more severe; any individual whose status has not been explicitly set can invoke the collectPresent function after the period of Christmas and obtain an NFT.

function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
if (balanceOf(msg.sender) > 0) {
revert SantasList__AlreadyCollected();
}
// audit-danger ANYONE can collect NICE presents due to the default value of the enum
if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) {
_mintAndIncrement();
return;
}
...
}

The heightened risk arises from the default value of the enum, which enables any individual whose status is unspecified to execute the collectPresent function and receive an NFT. This poses a substantial security concern that needs to be addressed to ensure the intended access restrictions are maintained.

PoC

First vulnerability:

// audit-info - PoC of potential check bypass on `checkTwice` function
function testCheckBypassOnCheckTwice() public {
address addr = makeAddr("0xbrivan");
vm.prank(santa);
// audit-info: The address can be checked twice as `NICE` person, even if it wasn't checked first
santasList.checkTwice(addr, SantasList.Status.NICE);
assertEq(uint256(SantasList.Status.NICE), uint256(santasList.getNaughtyOrNiceTwice(addr)));
}

Second Vulnerability:

// audit-info - PoC of that any individual whose status was not set explicitly can collectPresent
function testAnyOneCanCollectPresent() public {
address addr = makeAddr("0xbrivan");
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
vm.prank(addr);
santasList.collectPresent();
assertEq(santasList.balanceOf(addr), 1);
}
[PASS] testCheckBypassOnCheckTwice() (gas: 19764)
[PASS] testAnyOneCanCollectPresent() (gas: 86432)

Tools Used

Manual Analysis

Recommendations

Consider changing the order of Status enum elements. Because the first element is zero-value, the first element should be UNKOWN

Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year 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.