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

M02. `buy()` sets `isActive` after `_safeTransfer`, violating Checks-Effects-Interactions

Author Revealed upon completion

Root + Impact

Description

  • buy() is expected to mark a listing as inactive before transferring the NFT to the buyer.

  • The function calls _safeTransfer before setting s_listings[_listingId].isActive = false. _safeTransfer invokes onERC721Received on the recipient if it is a contract. During that callback, the listing is still marked active, meaning any code in the callback can observe or act on a state that should already be finalized. This violates the Checks-Effects-Interactions (CEI) pattern.

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");
​
activeListingsCounter--;
​
bool success = usdc.transferFrom(msg.sender, address(this), listing.price);
require(success, "USDC transfer failed");
​
// @> external call fires onERC721Received on msg.sender if it is a contract
_safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
​
// @> state update happens AFTER the external call — CEI violated
s_listings[_listingId].isActive = false;
}

Risk

Likelihood: Medium

  • Exploitation requires the buyer to be a smart contract that implements onERC721Received.

  • The attacker must also have enough USDC approved to cover each re-entrant buy call, and there must be multiple active listings to target.

  • In the current codebase, re-entry into buy() for the same listing is accidentally blocked by an activeListingsCounter underflow revert (uint32 going below 0). Any future refactor that adjusts counter logic could remove this protection.

Impact: Medium

  • A contract buyer can observe the still-active listing state inside onERC721Received and take actions that rely on that stale view (e.g., immediately re-listing at a manipulated price, or calling other functions that read isActive).

  • The accidental underflow protection is not a reliable security boundary; the design is fragile and can break with minor changes.

Proof of Concept

A malicious buyer contract receives the NFT via _safeTransfer, and inside onERC721Received reads whether listing.isActive is still true. In the current implementation it is, because s_listings[_listingId].isActive = false has not executed yet.

The following contract and test are in test/ThreatModelTests.t.sol and pass with forge test --match-test test_ceiViolation.

/// Helper contract
contract ReentrantBuyerCEI is IERC721Receiver {
NFTDealers public nftDealers;
uint256 public targetListingId;
bool public attacking;
bool public sawActiveListingDuringCallback;
​
constructor(address _nftDealers) {
nftDealers = NFTDealers(_nftDealers);
}
​
function attack(uint256 listingId) external {
targetListingId = listingId;
attacking = true;
nftDealers.buy(listingId);
attacking = false;
}
​
function onERC721Received(address, address, uint256, bytes calldata)
external override returns (bytes4)
{
if (attacking) {
(, , , , bool isActive) = nftDealers.s_listings(targetListingId);
// CEI violation: isActive is still true here — state update has not run yet
sawActiveListingDuringCallback = isActive;
}
return IERC721Receiver.onERC721Received.selector;
}
}
​
/// Test
function test_ceiViolation_listingStillActiveDuringCallback() public {
uint32 nftPrice = 500e6;
​
vm.prank(seller);
nftDealers.list(1, nftPrice);
​
ReentrantBuyerCEI malicious = new ReentrantBuyerCEI(address(nftDealers));
usdc.mint(address(malicious), nftPrice);
​
vm.startPrank(address(malicious));
usdc.approve(address(nftDealers), nftPrice);
malicious.attack(1);
vm.stopPrank();
​
// Proved: listing was still marked active when onERC721Received fired
assertTrue(
malicious.sawActiveListingDuringCallback(),
"SM-1: listing.isActive was true during onERC721Received callback"
);
}

Recommended Mitigation

In buy(), move s_listings[_listingId].isActive = false to before the _safeTransfer call so that the listing is fully deactivated before any external code executes.

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");
​
activeListingsCounter--;
+ s_listings[_listingId].isActive = false; // effect before interaction
​
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;
​
emit NFT_Dealers_Sold(msg.sender, listing.price);
}

Support

FAQs

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

Give us feedback!