Root + Impact

Description

mintSnowman calls _safeMint before incrementing s_TokenCounter. OpenZeppelin's _safeMint invokes onERC721Received on contract receivers before returning, meaning external code executes while s_TokenCounter still holds the pre-increment value. Any receiver contract that reads getTokenCounter() during that callback observes a stale count.

This violates the Checks-Effects-Interactions pattern, which requires all state changes to be committed before any external interaction occurs.

Risk

Likelihood:

• The stale-state window is triggered whenever mintSnowman() mints to a contract implementing IERC721Receiver, as _safeMint() automatically invokes onERC721Received(). However, OpenZeppelin's ERC721 implementation prevents duplicate token minting, and no practical attack path currently exists that allows an attacker to gain additional NFTs, steal funds, or corrupt protocol state through this callback.

  
  

Impact:

Although OpenZeppelin's ERC721 implementation prevents duplicate token IDs and no direct theft or double-mint scenario exists, external integrations may observe inconsistent state during the callback window.
Specifically:

• A receiver contract can observe a stale s_TokenCounter value during onERC721Received().

• Future protocol upgrades may accidentally introduce exploitable reentrancy if additional state-dependent logic is added around minting.

• The implementation does not follow established Solidity security best practices, increasing maintenance and upgrade risk.

Proof of Concept

The test deploys Snowman alongside a custom ReceiverThatReadsCounter contract that implements IERC721Receiver. When mintSnowman is called with this contract as the receiver, OpenZeppelin's _safeMint invokes onERC721Received on the receiver before the s_TokenCounter++ line executes. Inside that callback, the receiver reads getTokenCounter() and stores the value it observes. After the full mint completes, the test asserts that the counter value seen during the callback equals 0 — the same as the token ID just minted — confirming that s_TokenCounter was not yet incremented when external code was running. The post-mint assertion shows the counter correctly reads 1 after the function returns, proving a stale-state window exists during the entire callback execution.

To run: forge test --match-test test_StaleCounterObservedDuringCallback -vvvv

Recommended Mitigation

The minimal CEI fix is to increment s_TokenCounter inside the loop but before _safeMint fires. Cache the pre-increment value into a local tokenId variable, increment s_TokenCounter immediately, then call _safeMint using the cached value.

This ensures the storage update is committed before any external callback executes — getTokenCounter() called from inside onERC721Received will now return the correct post-mint value.