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

M-03: buy() violates the Checks-Effects-Interactions pattern — listing remains active during the ERC721 receiver callback, creating a re-entry window

Author Revealed upon completion

Description

The expected behavior of buy() is that all state changes (marking the listing as inactive) are completed before any external calls are made. This is the Checks-Effects-Interactions (CEI) pattern recommended by the Ethereum documentation and followed by all major OpenZeppelin contracts.

What actually happens is that _safeTransfer() — an external call that triggers onERC721Received() on any contract recipient — fires while s_listings[_listingId].isActive is still true. The isActive = false state update happens only after the external call returns.

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 their own NFT");
​
activeListingsCounter--; // partial effects
​
bool success = usdc.transferFrom(msg.sender, address(this), listing.price);
require(success, "USDC transfer failed");
_safeTransfer(listing.seller, msg.sender, listing.tokenId, ""); // @> interaction — calls onERC721Received()
​
s_listings[_listingId].isActive = false; // @> effect happens AFTER the external call
// ...
}

During the onERC721Received() callback, a malicious contract buyer can observe and act on the listing as still-active. While the current ERC721 ownership check incidentally prevents a double-buy (the NFT is already at the buyer's address, so a second _transfer from the seller would revert), this protection is coincidental and fragile. Any future addition of a function that reads isActive — such as a flash-loan integration, a lending feature, or a listing price oracle — could be exploited through this open window.

Risk

Likelihood:

  • Triggered any time a contract address purchases an NFT via buy()

  • No special preconditions required beyond being a contract buyer

Impact:

  • No direct fund loss is possible today due to the incidental ERC721 ownership guard

  • However, the broken CEI ordering is a latent vulnerability that can become directly exploitable if the protocol is extended — any new function checking isActive without independent access controls would be re-enterable from inside the callback

  • The listing state is inconsistent for the duration of the onERC721Received call, which is an invariant violation

Proof of Concept

/**
* @notice M-03 PoC: ReentrantBuyer is a contract that implements onERC721Received.
* During its callback it reads s_listings[listingId].isActive and stores
* the result. After the buy() call completes, we assert that isActive was
* true during the callback — proving the CEI violation.
*
* If CEI were respected, isActive would be false by the time any external call fires.
*/
function testBuyCEIViolationListingStillActiveOnCallback() public { ... }

Run with:

forge test --mt testBuyCEIViolationListingStillActiveOnCallback -vv

Expected console output:

--- CEI violation proof ---
Was listing isActive == true inside onERC721Received callback? true
Expected: true (proves isActive is set AFTER the external call)

Recommended Mitigation

Move s_listings[_listingId].isActive = false to before the _safeTransfer call. This closes the callback window and follows CEI correctly.

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);
}

Why this works: by the time _safeTransfer fires and triggers onERC721Received, the listing is already marked inactive. Any re-entrant call to buy() on the same listing ID will hit revert ListingNotActive, and cancelListing() will also revert on the same check.

Support

FAQs

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

Give us feedback!