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

Bypassing the balance check

y0ng0p3

Summary

A user can collect present multiple times.

Vulnerability Details

SantasList::CollectPresent() expects only users who have no SantaList ECR721 token to be able to collect presents.
A user can bypass this rule by transferring all their balance to another address before calling SantasList::CollectPresent().

Below are the steps to follow to get around the rule:

  • Collect present by calling SantasList::CollectPresent().

  • Transfer the SantasList ERC721 token received previously to another address.

  • Then call SantasList::CollectPresent() again and collect a new NFT.

The user can go through those steps as many times as the want.

Proof of Concept

Place the code for the following test function in test/unit/SantasListTest.t.sol.

function test_CantCollectPresent_MoreThanOnce() 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();
address user2 = makeAddr("user_2");
santasList.transferFrom(user, user2, 0);
santasList.collectPresent();
vm.stopPrank();
​
assertEq(santasList.balanceOf(user), 1);
assertEq(santasList.balanceOf(user2), 1);
}

In the terminal, run the following command:

  • forge test --mt test_CantCollectPresent_MoreThanOnce

Impact

EXTRA_NICE users can collect as many presents as they want.

Tools Used

Manual review, Foundry

Recommendations

1- Keep track of users who already collected a present

Add a state variable that maps address to bool, in order to keep track of addresses that have already collected presents.

File: src/SantasList.sol
​
81: mapping(address => bool) s_collected;

Move from balance check to collected check. And update s_collected before the return instruction in if-else statements

File: src/SantasList.sol
​
+ mapping(address => bool) s_collected;
​
​
function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
- if (balanceOf(msg.sender) > 0) {
+ if (s_collected[msg.sender]) {
revert SantasList__AlreadyCollected();
}
if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) {
_mintAndIncrement();
+ s_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);
+ s_collected[msg.sender] = true;
return;
}
revert SantasList__NotNice();
}

2- Use ERC721 Snapshot instead

Implement a Snapshot behavior to SantasList.
Take a snapshot at the CHRISTMAS_2023_BLOCK_TIME. Perform a check on balanceOfAt (user's balance at the time the snapshot was taken, with the snapshotId) instead of a check on balanceOf. Then record the snapshotId for this user at the end of the first SantasList::collectPresent() run for this user.

Note that this solution can be expensive in terms of gas, as retrieving values from snapshots costs a lot of gas.

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!