NFT Dealers
First Flight #58
Beginner FriendlyFoundry
100 EXP
View all submissions
Previous Next
Submission Details
Impact: medium
Likelihood: medium

[M-3] CEI Violation in buy() - isActive Not Cleared Before External Calls

Author Revealed upon completion

Root + Impact

Description

  • buy() is expected to follow the Checks-Effects-Interactions pattern, updating all state before making external calls to prevent reentrancy.

  • s_listings[_listingId].isActive = false is set after two external calls: usdc.transferFrom and _safeTransfer. The _safeTransfer call triggers onERC721Received on the buyer if it is a contract. During that callback, s_listings[_listingId].isActive is still true in storage, meaning all functions that gate on isActive read stale state. The contract has no ReentrancyGuard, so this window is fully accessible.

// src/NFTDealers.sol
function buy(uint256 _listingId) external payable {
Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId); // CHECK
require(listing.seller != msg.sender, "Seller cannot buy...");
​
activeListingsCounter--; // partial EFFECT
​
bool success = usdc.transferFrom( // INTERACTION #1
msg.sender, address(this), listing.price
);
require(success, "USDC transfer failed");
​
@> _safeTransfer(listing.seller, msg.sender, listing.tokenId, ""); // INTERACTION #2
// onERC721Received fires here — isActive is still true in storage
​
@> s_listings[_listingId].isActive = false; // EFFECT — too late
emit NFT_Dealers_Sold(msg.sender, listing.price);
}

Risk

Likelihood:

  • Any buyer using a smart contract wallet or receiver contract triggers onERC721Received.

  • The window is confirmed on-chain: isActive reads as true inside the callback.

Impact:

  • Direct double-buy is blocked by the ERC721 ownership invariant (seller no longer holds the token for the second transfer).

  • The stale isActive exposes cross-function reentrancy to all other functions gated on isActive.

  • Any future function addition that a buyer can call while isActive == true is immediately exploitable with no further changes needed.

Proof of Concept

An attacker contract implementing onERC721Received confirms that isActive is still true during the callback and attempts a re-entrant buy(). The inner call reverts when _safeTransfer fails (seller no longer owns the token), but the CEI violation is confirmed.

function onERC721Received(address, address, uint256, bytes calldata)
external override returns (bytes4)
{
reentryAttempted = true;
reentryCount++;
​
if (reentryCount == 1) {
// Confirm isActive is still true — CEI violation
(, , , , bool isActive) = nftDealers.s_listings(targetListingId);
require(isActive, "Listing should still be active here"); // always passes
​
// Attempt double-buy — reverts on _safeTransfer, USDC pull also rolled back
try nftDealers.buy(targetListingId) {
// unreachable in current code
} catch {
reentryReverted = true; // always true
}
}
return IERC721Receiver.onERC721Received.selector;
}

Recommended Mitigation

Move s_listings[_listingId].isActive = false before the external calls, and add ReentrancyGuard as defence-in-depth across all state-changing functions.

function buy(uint256 _listingId) external payable {
Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
require(listing.seller != msg.sender, "Seller cannot buy their own NFT");
​
+ // EFFECTS before INTERACTIONS
+ s_listings[_listingId].isActive = false;
activeListingsCounter--;
​
bool success = usdc.transferFrom(msg.sender, address(this), listing.price);
require(success, "USDC transfer failed");
- _safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
- s_listings[_listingId].isActive = false;
+ _safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
​
emit NFT_Dealers_Sold(msg.sender, listing.price);
}

Additionally, apply nonReentrant to all functions that make external calls:

+import {ReentrancyGuard} from "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
​
-contract NFTDealers is ERC721 {
+contract NFTDealers is ERC721, ReentrancyGuard {
​
- function buy(uint256 _listingId) external payable {
+ function buy(uint256 _listingId) external payable nonReentrant {
​
- function mintNft() external payable onlyWhenRevealed onlyWhitelisted {
+ function mintNft() external payable onlyWhenRevealed onlyWhitelisted nonReentrant {
​
- function cancelListing(uint256 _listingId) external {
+ function cancelListing(uint256 _listingId) external nonReentrant {
​
- function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
+ function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) nonReentrant {
​
- function withdrawFees() external onlyOwner {
+ function withdrawFees() external onlyOwner nonReentrant {

Support

FAQs

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

Give us feedback!