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

collectPresent function use balanceOf to check if the user has already collect the present - can collect present multiple times

rapstyle

Summary

The SantasList::collectPresent function use balanceOf(msg.sender) to check if a user has already get the present. This is not secure because the balance of the account can be changed.

function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
@> if (balanceOf(msg.sender) > 0) {
revert SantasList__AlreadyCollected();
}

Vulnerability Details

Using balanceOf(msg.sender) to check if a user has already get the present, an attacker can move the nft on a second wallet, and can collect another present multiple times.

I created a test to verify the vulnerability.

First of all we have to create a new user inside the SantasListTest.t.sol adding a new address.

address user = makeAddr("user");
+ address user2 = makeAddr("user2");

After that we can paste this test

Code:
function testCollectPresentNiceTwice() 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();
assertEq(santasList.balanceOf(user), 1);
​
// User send token to user2 account
santasList.safeTransferFrom(address(user), address(user2), 0);
assertEq(santasList.balanceOf(user), 0);
​
// User can not collect another present
santasList.collectPresent();
assertEq(santasList.balanceOf(user), 1);
​
// Move the previous present from original address
vm.startPrank(user2);
santasList.safeTransferFrom(address(user2), address(user), 0);
​
// Now user have 2 presents
vm.startPrank(user);
assertEq(santasList.balanceOf(user), 2);
​
vm.stopPrank();
}

and then we can launch the test with the command

forge test --mt testCollectPresentNiceTwice -vvv

Impact

An attacker can collect multiple NFTs with this technique. If the user have status EXTRA_NICE he can also mint tokens multiple times.

Tools Used

Manual review and Foundry test

Recommendations

It is better to use a mapping and save the addresses that have already collect the NFT.

mapping(address person => Status naughtyOrNice) private s_theListCheckedOnce;
mapping(address person => Status naughtyOrNice) private s_theListCheckedTwice;
+ mapping(address person => uint256) private s_presentsCollected;
...
function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
- if (balanceOf(msg.sender) > 0) {
+ if (s_presentsCollected[msg.sender] > 0) {
revert SantasList__AlreadyCollected();
}
if (
s_theListCheckedOnce[msg.sender] == Status.NICE &&
s_theListCheckedTwice[msg.sender] == Status.NICE
) {
_mintAndIncrement();
+ s_presentsCollected[msg.sender] = block.timestamp;
return;
} else if (
s_theListCheckedOnce[msg.sender] == Status.EXTRA_NICE &&
s_theListCheckedTwice[msg.sender] == Status.EXTRA_NICE
) {
_mintAndIncrement();
i_santaToken.mint(msg.sender);
+ s_presentsCollected[msg.sender] = block.timestamp;
return;
}
revert SantasList__NotNice();
}
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!