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

SantasList._mintAndIncrement() does not increment s_tokenCounter in global state

datadata

Root + Impact

Description

  • The normal, expected behavior of SantasList._mintAndIncrement() is that it should increment the state variable s_tokenCounter by 1.

  • Right now, _mintAndIncrement() does not increment the state variable s_tokenCounter for the contract, but rather only increments the value of s_tokenCounter within the parameter of _safeMint(), meaning that s_tokenCounter remains at the same value it was before _mintAndIncrement() was called.

function _mintAndIncrement() private {
@> _safeMint(msg.sender, s_tokenCounter++);
}

Risk

Likelihood:

  • The likelyhood that _mintAndIncrement() will not increment s_tokenCounter is always. It is 100%.

  • s_tokenCounter will always remain the same value it was before calling _mintAndIncrement() as after calling it.

Impact:

  • The impact is that s_tokenCounter will not be incremented, and it will remain at its current value.

  • By not incrementing s_tokenCounter, it means that all NFTs will be given the same token id.

Proof of Concept

Below we show that user calls buyPresent(), which calls _mintAndIncrement(), but we see that santasList.s_tokenCounter is not incremented, and instead remains the same as before.

// This PoC uses a getter function in SantasList:
function getTokenCounter() external pure returns (uint256) {
return s_tokenCounter;
}
​
// In our testing contract
// Setup for tests
// SPDX-License-Identifier: MIT
pragma solidity 0.8.22;
​
import {SantasList} from "../../src/SantasList.sol";
import {SantaToken} from "../../src/SantaToken.sol";
import {Test} from "forge-std/Test.sol";
import {console2} from "forge-std/console2.sol"; // added this
import {_CheatCodes} from "../mocks/CheatCodes.t.sol";
​
contract SantasListTest is Test {
SantasList santasList;
SantaToken santaToken;
​
address user = makeAddr("user");
address santa = makeAddr("santa");
_CheatCodes cheatCodes = _CheatCodes(HEVM_ADDRESS);
​
function setUp() public {
vm.startPrank(santa);
santasList = new SantasList();
santaToken = SantaToken(santasList.getSantaToken());
vm.stopPrank();
}
​
function testMintAndIncrementDoesNotIncrementTokenCounter() public {
// ARRANGE
uint256 tokenCounterBefore = santasList.getTokenCounter();
// ACT
vm.startPrank(user);
// user calls buyPresent, which calls _mintAndIncrement() in addition to burning user2's tokens
santasList.buyPresent(user2);
vm.stopPrank(user);
// ASSERT
uint256 tokenCounterAfter = santasList.getTokenCounter();
assertEq(tokenCounterBefore, tokenCounterAfter);
}
​

Recommended Mitigation

First _mintAndIncrement() should properly increment s_tokenCounter with s_tokenCounter++; and then it should call _safeMint() using the updated s_tokenCounter. Additionally, buyPresent() calls _mintAndIncrement() in such a way that it presupposes that msg.sender is the recipient of the NFT, because _mintAndIncrement() does not take a recipient parameter. This dilemma can be fixed with a new function or by modifying _mintAndIncrement() to take a recipient parameter that is then passed to _safeMint().

function _mintAndIncrement() private {
- _safeMint(msg.sender, s_tokenCounter++);
+ s_tokenCounter++;
+ _safeMint(msg.sender, s_tokenCounter);
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 13 days ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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

Give us feedback!