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

SantasList::collectPresent can be called by anyone even if they are not checked by santa due to mishandling of Status enum

shikhar229169

Summary

In the SantasList contract only those users are allowed to collect present who are checked twice by santa but even though if the user is not checked twice by santa then also users can collect their present due mishandling of the Status enum .

In enum Status, NICE is present at 0th position, also the mapping s_theListCheckedOnce and s_theListCheckedTwice store the default value 0 by default which means everyone will be NICE by default in both the mapping and they can call the collectPresent function even though they are not even checked by santa and get the NFT.

Vulnerability Details

This vulnerability exists in the SantasList::Status enum in the SantasList.sol file starting on line 69. The Status has NICE at 0th position and as both the mapping starting from line 79 and ending at 80 will have 0 as their default values which corresponds to NICE of the Status enum, therefore every user will be NICE by default and they can call the collectPresent function on SantasList contract and collect the NFT without even getting checked by Santa.

/*//////////////////////////////////////////////////////////////
TYPES
//////////////////////////////////////////////////////////////*/
enum Status {
NICE,
EXTRA_NICE,
NAUGHTY,
NOT_CHECKED_TWICE
}
​
/*//////////////////////////////////////////////////////////////
STATE VARIABLES
//////////////////////////////////////////////////////////////*/
mapping(address person => Status naughtyOrNice) private s_theListCheckedOnce;
mapping(address person => Status naughtyOrNice) private s_theListCheckedTwice;

Impact

The Santa only wants to allow those users to call collectPresent function which are checked by him as NICE or EXTRA_NICE but if a user who is not checked by him should not be able to call it and it should revert.

But due the mishandling of Status enum, everyone will be NICE by default, so all the unchecked users will be NICE and they will be able to mint the NFT by calling the collectPresent function

PoC

Refactor the import in file test/unit/SantasListTest.t.sol: (Vm is used to get array to get the Log array to store recorded event logs)

import {Test} from "forge-std/Test.sol";

to

import {Vm, Test} from "forge-std/Test.sol";

Add the test in the file test/unit/SantasListTest.t.sol
Run the test:

forge test --mt test_AnyoneCanGetPresentEvenIfTheyAreNotChecked_AsTheStatusEnumWillBeNiceByDefault
function test_AnyoneCanGetPresentEvenIfTheyAreNotChecked_AsTheStatusEnumWillBeNiceByDefault() public {
// santa does nothing and the user remains unchecked
​
// assert check for both mapping
assert(santasList.getNaughtyOrNiceOnce(user) == SantasList.Status.NICE);
assert(santasList.getNaughtyOrNiceTwice(user) == SantasList.Status.NICE);
​
// initially user holds no nft
assertEq(santasList.balanceOf(user), 0);
​
vm.recordLogs();
​
// user collects the present without even getting checked by santa
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME());
vm.prank(user);
santasList.collectPresent();
​
Vm.Log[] memory logs = vm.getRecordedLogs();
// Getting the token id from Transfer event, it will be the first event to get triggered present at idx 0
// the token id is indexed parameter which is at 3rd position
uint256 tokenId = uint256(logs[0].topics[3]);
​
uint256 userNftBalance = santasList.balanceOf(user);
address ownerOfNft = santasList.ownerOf(tokenId);
​
// user gets minted their santa nft and their balance increases from 0 to 1
assertEq(userNftBalance, 1);
assertEq(ownerOfNft, user);
}

Tools Used

Manual Review, Foundry Test

Recommendations

In the Status enum of SantasList contract, move NOT_CHECKED_TWICE to 0th position, so that every user will now be not checked by default as it will be at 0th position and both mapping s_theListCheckedOnce and s_theListCheckedTwice will now be NOT_CHECKED_TWICE by default.

enum Status {
+ NOT_CHECKED_TWICE,
NICE,
EXTRA_NICE,
NAUGHTY
- NOT_CHECKED_TWICE
}
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.