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

Bypassable NFT Collection Check in collectPresent Function

luiscfaria

Summary

A notable vulnerability exists in the collectPresent function of the SantasList smart contract. The function's intent is to prevent users from collecting more than one NFT. However, the current implementation only checks if the caller's balance is greater than zero, neglecting the possibility that a user could transfer their initially collected NFT to another address and then collect an additional NFT. This oversight enables users to potentially collect multiple NFTs by exploiting this loophole.

Vulnerability Details

The issue arises in the conditional check if (balanceOf(msg.sender) > 0), which is designed to ensure a user does not collect more than one NFT. However, this check can be easily circumvented. A user who has already collected an NFT can transfer it to a different address and then call collectPresent again, as their balance would be back to zero, satisfying the function's condition.

In addition to the original issue, a malicious contract could exploit the _checkOnERC721Received callback, which is typically called during the transfer of an ERC721 token (NFT). This callback can be used to trigger reentrancy attacks. A reentrancy attack occurs when a contract makes an external call to another untrusted contract, which then calls back into the calling contract before the first execution is complete.

In this specific context, a malicious contract could receive an NFT from the collectPresent function and, during the transfer process, use the _checkOnERC721Received callback to transfer the received NFT and re-enter the collectPresent function. This reentrancy could potentially allow the malicious contract to collect multiple NFTs in a single transaction before the state of the original contract is updated to reflect the first collection, bypassing the intended one-NFT-per-user limitation.

Impact

Users can unfairly claim more than their entitled single NFT, undermining the fairness and intended mechanics of the reward distribution.

Proof of Concept

Working test case

In the following test case a user successfully collects the present, transfers it to bob's address and collects another one, which is not the intended system.

function test_CanCollectPresentIfAlreadyCollectedAndTransfered() public {
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);
santasList.collectPresent();
vm.stopPrank();
​
assertEq(santasList.balanceOf(user), 1);
vm.startPrank(user);
santasList.safeTransferFrom(user, bob, 0);
santasList.collectPresent();
vm.stopPrank();
​
assertEq(santasList.balanceOf(user), 1);
assertEq(santasList.balanceOf(bob), 1);
}

Terminal:

Running 1 test for test/unit/SantasListTest2.t..sol:SantasListTest
[PASS] test_CanCollectPresentIfAlreadyCollectedAndTransfered() (gas: 160668)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.42ms

Tools Used

Foundry

Recommended Mitigation

Introduce a mechanism to track whether a user has already claimed their NFT, ensuring that each eligible user can only collect once, regardless of their current NFT balance.

Add a new mapping to track whether an address has collected an NFT:

mapping(address => bool) private hasCollectedNFT;

Use the mapping to check if already collected:

function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
​
if (hasCollectedNFT[msg.sender]) {
revert SantasList__AlreadyCollected();
}
​
<!-- Add effects before interactions to avoid reentrancy attacks -->
hasCollectedNFT[msg.sender] = true;
​
if (
s_theListCheckedOnce[msg.sender] == Status.NICE &&
s_theListCheckedTwice[msg.sender] == Status.NICE
) {
_mintAndIncrement();
} else if (
s_theListCheckedOnce[msg.sender] == Status.EXTRA_NICE &&
s_theListCheckedTwice[msg.sender] == Status.EXTRA_NICE
) {
_mintAndIncrement();
i_santaToken.mint(msg.sender);
} else {
revert SantasList__NotNice();
}
​
}
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.