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

User can collect more than once bypassing the `balanceOf` check in `SantasList::collectPresent` function

bube

Summary

The SantasList::collectPresent function should allow collecting present if the caller is nice or extra nice. Also, the addresses should not be able to collect more than once. Therefore, there is a check if the balance of msg.sender is greather than zero. The problem is that the user can collect his present, then transfer it to another address and collect again bypassing the balanceOf check.

Vulnerability Details

The user that is nice or extra nice can collect more than once in the SantasList::collectPresent function. The function uses balanceOf(msg.sender) to check that the user is already collected. But this statement can be easily manipulated by transferring the collecting present to another address. In this case the balance of msg.sender will be again 0 and the check will be passed.

function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
//@audit the use of `balanceOf` is not recommended because a malicious user can transfer his balance to another address and
// collect more than once bypassing the check
@> 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;
}

Impact

The impact of this vulnerability is that the nice and extra nice users can collect their present more than once.
Let's consider the following scenario:

  1. Bob has NICE status to be eligible for collectPresent.

  2. Bob calls collectPresent function to mint an NFT.

  3. Bob's balance is 1 after collecting.

  4. Bob transfers his NFT to another address - Alice.

  5. Bob calls collectPresent again.

In the file SantasListTest.t.sol there is a test function that shows that the user can't collect more than once. But this test function does not take into account that the user can transfer his token before calling again the collectPresent function.

The following test function demonstrates how a nice user (Bob) can bypass the balanceOf check and collect more than once:

​
function testUserCanCollectPresentMoreThanOnce() public {
vm.startPrank(santa);
santasList.checkList(bob, SantasList.Status.NICE);
santasList.checkTwice(bob, SantasList.Status.NICE);
vm.stopPrank();
​
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME());
​
// Bob collects a present
vm.startPrank(bob);
santasList.collectPresent();
​
// Check that Bob now has a balance of 1 NFT
assertEq(santasList.balanceOf(bob), 1);
​
// Bob transfers his NFT to another address for this function to the Alice address
santasList.transferFrom(bob, alice, 0);
​
// Check that the Bob's balance is now 0 and the Alices's balance is 1
assertEq(santasList.balanceOf(bob), 0);
assertEq(santasList.balanceOf(alice), 1);
​
// Bob tries to collect a present again
santasList.collectPresent();
​
// Check that Bob now has a balance of 1 NFT again, demonstrating the bypass of the balanceOf check
assertEq(santasList.balanceOf(bob), 1);
}
​

You can add this function to the SantasListTest.t.sol and execute it using Foundry and the following command: forge test --match-test testUserCanCollectPresentMoreThanOnce

Tools Used

VS Code, Foundry

Recommendations

You can add a mapping that track if an address has collected a present and a function that checks if an address has already collected:

​
// Add a new state variable to track if an address has collected a present
+ mapping(address => bool) private s_hasCollected;
.
.
.
// Modify the collectPresent function
function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
+ if (s_hasCollected[msg.sender]) {
+ revert SantasList__AlreadyCollected();
+ }
- if (balanceOf(msg.sender) > 0) {
- revert SantasList__AlreadyCollected();
- }
if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) {
_mintAndIncrement();
// Mark the user as having collected
+ s_hasCollected[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);
// Mark the user as having collected
+ s_hasCollected[msg.sender] = true;
return;
}
revert SantasList__NotNice();
}
.
.
.
// You may also want to add a function to check if an address has collected
+function hasCollected(address person) external view returns (bool) {
+ return s_hasCollected[person];
+}
​

With these changes, the collectPresent function now checks the s_hasCollected mapping to determine if the caller has already collected a present. If he has, the function reverts with the SantasList__AlreadyCollected error. After successfully collecting a present, the user's address is marked as having collected in the s_hasCollected mapping, preventing them from collecting again in the future. The additional hasCollected function is a convenience method that allows anyone to check if a particular address has already collected a present.

Updates

Lead Judging Commences

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

Give us feedback!