Beginner FriendlyFoundry
100 EXP
View results
Submission Details
Severity: high
Valid

Anyone can collect a NFT with no prior checks because all addresses are `NICE` and checked twice by default

Summary

Anyone can collect a NFT through the SantasList::collectPresent() function without having been checked twice previously because all addresses have the status NICE and are checked twice by default.

Vulnerability Details

The mappings SantasList::s_theListCheckedOnce and SantasList::s_theListCheckedTwice use address as key and SantasList::Status as values. If you retrieve a non changed or non initialized value from any of these two mappings using any address as key, you will get the default value of the SantasList::Status enum.

Enums are a custom type of enumerated unsigned integers. Its value starts at 0 and goes up to the amount of enum values - 1.

Let's use SantasList::Status as an example:

enum Status {
NICE,
EXTRA_NICE,
NAUGHTY,
NOT_CHECKED_TWICE
}

The first value, NICE, is equal to 0, EXTRA_NICE is equal to 1, NAUGHTY is equal to 2, and NOT_CHECKED_TWICE is equal to 3.

When you don't initialize a unsigned integer with a value, its default value will be 0. The same happens to enums. So the default value of the SantasList::Status enum is NICE.

Knowing this, when you retrieve the default status of an address from SantasList::s_theListCheckedOnce or SantasList::s_theListCheckedTwice you will receive the value NICE.

Proof of Concept

Apply the following diff:

modified test/unit/SantasListTest.t.sol
@@ -152,4 +152,40 @@ contract SantasListTest is Test {
cmds[1] = string.concat("youve-been-pwned");
cheatCodes.ffi(cmds);
}
+
+ function testAllAddressesAreNiceAndDoubleCheckedByDefault(address randomAddress) public {
+ // `randomAddress` cannot be equal to zero address in order to mint a NFT
+ vm.assume(randomAddress != address(0));
+
+ // Retrieve the first checked status of `randomAddress`
+ SantasList.Status firstStatus = santasList.getNaughtyOrNiceOnce(randomAddress);
+
+ // Retrieve the second checked status of `randomAddress`
+ SantasList.Status secondStatus = santasList.getNaughtyOrNiceTwice(randomAddress);
+
+ // Expected status is the default `Status` value
+ SantasList.Status expectedStatus;
+
+ // Assert if the default `Status` value is equal to NICE
+ assert(expectedStatus == SantasList.Status.NICE);
+
+ // Assert if the default status of `randomAddress` is equal to the expected status
+ assert(firstStatus == expectedStatus);
+ assert(secondStatus == expectedStatus);
+
+ // Time travel to Christmas 🎅
+ vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME());
+
+ // Impersonates `randomAddress`
+ vm.startPrank(randomAddress);
+
+ // Since `randomAdress` is double checked and NICE, we can collect a NFT
+ santasList.collectPresent();
+
+ // Stops impersonating `randomAddress`
+ vm.stopPrank();
+
+ // Check if the NFT has been collected
+ assertEq(santasList.balanceOf(randomAddress), 1);
+ }
}

And run the testAllAddressesAreNiceAndDoubleCheckedByDefault test:

forge test --match-test testAllAddressesAreNiceAndDoubleCheckedByDefault

Impact

Anyone can collect a NFT through the SantasList::collectPresent() function without having been checked twice previously.

Tools Used

  • Manual Review

  • GNU Emacs (solidity-mode + magit)

  • Foundry test

Recommendations

Add a UNKNOWN enum value above NICE inside SantasList::Status enum:

modified src/SantasList.sol
@@ -67,6 +67,7 @@ contract SantasList is ERC721, TokenUri {
TYPES
//////////////////////////////////////////////////////////////*/
enum Status {
+ UNKNOWN,
NICE,
EXTRA_NICE,
NAUGHTY,

So the default value of SantasList::Status will be UNKNOWN instead of NICE.

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.