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 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.