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

NFT holders cannot collect present

y0ng0p3

Summary

Users who already own SantasList ERC721 tokens cannot collect present.

Vulnerability Details

SantasList::collectPresent() doesn't allow a user to collect present more than once. To reenforce this rule, it reverts if the SantasList ERC721 balance of the caller exceeds zero. Which means a SantasList ERC721 tokens holder who already have an asset is not allowed to collect present even if they have not yet collected any present and their status is NICE or EXTRA_NICE.

Proof of Concept

Create a new file: test/libraries/harnesses/SantasListHarness.sol. Copy the following code and paste it in the new created SantasListHarness.sol.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.22;
​
import {SantasList} from "../../../src/SantasList.sol";
​
contract SantasListHarness {
function mintAndIncrement() public {
SantasList._mintAndIncrement();
}
}

In test/unit/SantasListTest.t.sol import SantasListHarness from ../libraries/harnesses/SantasListHarness.sol and update the setUp method as follows:

File: test/unit/SantasListTest.t.sol
​
function setUp() public {
vm.startPrank(santa);
+ santasListHarness = new SantasListHarness();
santasList = new SantasList();
santaToken = SantaToken(santasList.getSantaToken());
vm.stopPrank();
}

Place the code for the following test functions in test/unit/SantasListTest.t.sol.

function test_CantCollectPresent_IfAlreadyHoldToken() public {
vm.startPrank(user);
santasListHarness.mintAndIncrement();
vm.stopPrank();
​
vm.startPrank(santa);
santasList.checkList(user, SantasList.Status.NICE);
santasList.checkTwice(user, SantasList.Status.NICE);
vm.stopPrank();
​
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
vm.startPrank(user);
vm.expectRevert(SantasList.SantasList__AlreadyCollected.selector);
santasList.collectPresent();
}

In the terminal, run the following command:

  • forge test --mt test_CantCollectPresent_IfAlreadyHoldToken

Impact

Users who already have SantasList ERC721 tokens cannot collect a present, even if they are entitled to do so.

Tools Used

Manual review, Foundry

Recommendations

Add a state variable that maps address to bool, in order to keep track of addresses that have already collected presents.

File: src/SantasList.sol
​
81: mapping(address => bool) s_collected;

Move from balance check to collected check. And update s_collected before the return instruction in if-else statements

File: src/SantasList.sol
​
+ mapping(address => bool) s_collected;
​
​
function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
- if (balanceOf(msg.sender) > 0) {
+ if (s_collected[msg.sender]) {
revert SantasList__AlreadyCollected();
}
if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) {
_mintAndIncrement();
+ s_collected[msg.sender] = true;
return;
} else if (
s_theListCheckedOnce[msg.sender] == Status.EXTRA_NICE
&& s_theListCheckedTwice[msg.sender] == Status.EXTRA_NICE
) {
_mintAndIncrement();
i_santaToken.mint(msg.sender);
+ s_collected[msg.sender] = true;
return;
}
revert SantasList__NotNice();
}
Updates

Lead Judging Commences

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

Weak Already Collected Check

Relying on balanceOf > 0 in collectPresent() allows the msg.sender to send their present to another address and then collect again.

Support

FAQs

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

Give us feedback!