NFT Dealers

Root + Impact

Description

In cancelListing() (L157-169), the state update collateralForMinting[listing.tokenId] = 0 at L166 occurs after the external call usdc.safeTransfer(listing.seller, collateralForMinting[listing.tokenId]) at L165. This violates the Checks-Effects-Interactions (CEI) pattern — a well-established Solidity best practice for preventing reentrancy.

Notably, the function partially follows CEI: s_listings[_listingId].isActive = false at L162 and activeListingsCounter-- at L163 are both set before the external call. But collateralForMinting[listing.tokenId] is zeroed after it:

During the external call at L165, collateralForMinting[listing.tokenId] is still non-zero. If the USDC token had transfer hooks (e.g., ERC-777 tokensReceived), a malicious seller contract could re-enter the protocol during the callback. At that point, collectUsdcFromSelling() (L171-183) would read the still-non-zero collateralForMinting[listing.tokenId] at L177 and add it to the seller's payout at L180 — on top of the collateral already being transferred by cancelListing.

The re-entrant call path during the callback would be:

1. cancelListing() transfers collateral at L165 (callback fires here)

2. During callback: seller calls collectUsdcFromSelling(_listingId)

3. onlySeller modifier (L72-75): s_listings[_listingId].seller == msg.sender — passes (seller field not cleared)

4. !listing.isActive at L173 — passes (set to false at L162 before the external call)

5. collateralForMinting[listing.tokenId] at L177 — still non-zero (not yet zeroed at L166)

6. Seller receives listing.price - fees + collateral — the collateral is double-counted

With the current MockUSDC (standard ERC-20, L5 imports OpenZeppelin's SafeERC20), this is not directly exploitable because ERC-20 transfer() does not invoke callbacks on the recipient. The safeTransfer wrapper (from OpenZeppelin's SafeERC20 at L10) calls IERC20.transfer(), which has no recipient hooks.

Compare with collectUsdcFromSelling() (L171-183), which has the same pattern — collateralForMinting is never zeroed at all, which is a separate accepted finding (H-01). The CEI issue in cancelListing is distinct: it does zero the mapping, but does so in the wrong order relative to the external call.

Risk

Likelihood:

• The CEI violation is present in every cancelListing() call — collateralForMinting is zeroed at L166 after the external usdc.safeTransfer at L165

• With standard ERC-20 USDC (no transfer hooks), the reentrancy window cannot be exploited

• If the protocol were deployed with a token that has transfer hooks (ERC-777, or a future USDC upgrade with hooks), the window would become exploitable

Impact:

• With standard USDC: no direct fund loss — this is a best-practice violation only

• With a hooked token (theoretical): a malicious seller could double-claim collateral by re-entering collectUsdcFromSelling during the callback, stealing from other users' funds

Proof of Concept

The following test confirms the CEI ordering issue — collateralForMinting is non-zero during the external call window. Run with forge test --match-test test_collateralNonZeroDuringExternalCall -vv:

Recommended Mitigation

Cache the collateral value, zero the mapping, then perform the external transfer — following the CEI pattern consistently for all state updates in the function.

This fix:

1. Caches collateralForMinting[listing.tokenId] in a local variable before zeroing

2. Zeros the mapping before the external usdc.safeTransfer call — closing the reentrancy window

3. Passes the cached value to safeTransfer — functionally equivalent, no change in payout amount

4. Aligns all state updates (isActive, activeListingsCounter, and now collateralForMinting) before the external call, achieving full CEI compliance