NFT Dealers

Reentrancy in buy() allows external callbacks to observe stale listing state, causing inconsistent marketplace accounting

Risk

• Impact: Low

• Likelihood: Medium

Description:

The buy() function performs external interactions before fully finalizing the listing state in storage.

The root cause is that activeListingsCounter is decremented before the external calls, while s_listings[_listingId].isActive is only set to false after usdc.transferFrom(...) and _safeTransfer(...) complete.

This violates the Checks-Effects-Interactions pattern.

Because _safeTransfer(...) may invoke onERC721Received(...) when the buyer is a contract, external code can execute while the protocol is in an inconsistent intermediate state.

During that callback, the following condition can hold simultaneously:

• totalActiveListings() == 0

• s_listings[_listingId].isActive == true

This creates a real reentrancy window where external code can observe stale listing state before the purchase flow is fully finalized.

Impact:

No direct theft of funds was demonstrated from this issue alone.

However, the issue causes inconsistent marketplace state during external execution:

• the active listing counter is already decremented

• the corresponding listing still remains active in storage

This can break assumptions made by listing-dependent logic and increases the attack surface for cross-function reentrancy and unsafe state observation.

Since the demonstrated PoC does not show direct or indirect fund loss, the most appropriate classification is:

Likelihood is medium because exploitation requires a contract buyer capable of receiving the NFT through _safeTransfer(...) and executing logic in onERC721Received(...), but the reentrancy surface is real and reproducible.

Proof of Concept:

A malicious buyer contract implementing onERC721Received(...) was used to observe protocol state during _safeTransfer(...).

The test setup:

1. A seller lists token #1

2. A malicious buyer contract calls buy(1)

3. During _safeTransfer(...), the buyer’s onERC721Received(...) callback reads:

   • totalActiveListings()

   • s_listings(1).isActive

PoC test:

Observed execution:

1. Before buy(1)

   • totalActiveListings() = 1

   • s_listings(1).isActive = true

2. During onERC721Received(...)

   • totalActiveListings() = 0

   • s_listings(1).isActive = true

3. After buy() finishes

   • totalActiveListings() = 0

   • s_listings(1).isActive = false

Illustrative output:

This confirms that external code executes while the protocol exposes stale listing state, validating the reentrancy surface reported by Slither.

Recommended Mitigation:

Apply Checks-Effects-Interactions by fully closing the listing before making any external calls.

A safer implementation is:

Additionally, using nonReentrant provides defense in depth against future reentrancy paths.