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

Listed NFTs can be transferred via ERC721, creating stale listings that block buyers and prevent the new owner from re-listing

Author Revealed upon completion

Root + Impact

Description

The list() function (L127-139) records listing metadata in s_listings[_tokenId] but does not escrow the NFT -- the seller retains full ERC721 transfer capability. If the seller transfers the listed NFT to another address via the inherited transferFrom() or safeTransferFrom(), the listing remains active in storage with a stale seller field that no longer matches the token owner.

This creates two downstream failures:

  1. buy() permanently reverts: At L150, buy() calls _safeTransfer(listing.seller, msg.sender, listing.tokenId, ""). Since listing.seller no longer owns the token, OpenZeppelin's _transfer() reverts with ERC721IncorrectOwner. The listing appears buyable (isActive == true at L130) but every purchase attempt fails. Buyers' USDC is safe because the revert rolls back the usdc.transferFrom at L148 in the same transaction.

  2. New owner cannot re-list: The list() function checks s_listings[_tokenId].isActive == false at L130. Since the stale listing remains active, the new NFT owner's call to list() reverts with "NFT is already listed". Only the original seller can call cancelListing() (L157-169) because of the listing.seller == msg.sender check at L160 -- the new owner has no way to clear the stale listing.

The root cause is that list() does not escrow the NFT and the contract does not override ERC721's _update() hook to invalidate listings on ownership changes. The NFTDealers contract inherits from OpenZeppelin's ERC721 (L9) which provides unrestricted transferFrom and safeTransferFrom.

function list(uint256 _tokenId, uint32 _price) external onlyWhitelisted { // @audit L127: no escrow
require(_price >= MIN_PRICE, "Price must be at least 1 USDC"); // @audit L128
require(ownerOf(_tokenId) == msg.sender, "Not owner of NFT"); // @audit L129: only checks at list time
require(s_listings[_tokenId].isActive == false, "NFT is already listed"); // @audit L130: blocks new owner
require(_price > 0, "Price must be greater than 0"); // @audit L131
​
listingsCounter++;
activeListingsCounter++;
​
s_listings[_tokenId] = // @audit L136-137: stores seller snapshot
Listing({seller: msg.sender, price: _price, nft: address(this), tokenId: _tokenId, isActive: true});
emit NFT_Dealers_Listed(msg.sender, listingsCounter);
}
​
function buy(uint256 _listingId) external payable { // @audit L141
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); // @audit L148: reverted if L150 fails
require(success, "USDC transfer failed");
_safeTransfer(listing.seller, msg.sender, listing.tokenId, ""); // @audit L150: reverts if seller != owner
​
s_listings[_listingId].isActive = false;
emit NFT_Dealers_Sold(msg.sender, listing.price);
}

Risk

Likelihood:

  • The seller retains full ERC721 transfer capability after listing -- standard transferFrom() and safeTransferFrom() are inherited from OpenZeppelin's ERC721 (L9) with no override

  • Transfers can occur intentionally (gift, OTC sale) or accidentally (seller forgets about active listing)

  • The stale listing persists until the original seller cooperates by calling cancelListing() (L157-169)

Impact:

  • No direct fund loss -- buyers' USDC is protected by the transaction reverting atomically at L150

  • Stale listings appear active but always revert on buy(), wasting buyer gas and degrading marketplace UX

  • New NFT owners are blocked from listing on the marketplace until the original seller cancels (dependency on third-party cooperation)

  • activeListingsCounter (L48) becomes inflated, causing totalActiveListings() (L217-219) to return misleading values

  • The original seller's collateral (collateralForMinting[tokenId]) remains locked until they cancel

Proof of Concept

The seller lists an NFT, then transfers it via ERC721. The buyer's buy() call reverts, and the new owner cannot re-list. Run with forge test --match-test test_staleListingAfterTransfer -vv:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.34;
​
import {Test, console} from "forge-std/Test.sol";
import {NFTDealers} from "../src/NFTDealers.sol";
import {MockUSDC} from "../src/MockUSDC.sol";
​
contract L01PoCTest is Test {
NFTDealers public nftDealers;
MockUSDC public usdc;
​
address public owner = makeAddr("owner");
address public seller = makeAddr("seller");
address public buyer = makeAddr("buyer");
address public recipient = makeAddr("recipient");
​
uint256 constant LOCK_AMOUNT = 20e6; // 20 USDC
​
function setUp() public {
usdc = new MockUSDC();
nftDealers = new NFTDealers(owner, address(usdc), "NFTDealers", "NFTD", "ipfs://test/", LOCK_AMOUNT);
​
vm.startPrank(owner);
nftDealers.revealCollection();
nftDealers.whitelistWallet(seller);
nftDealers.whitelistWallet(recipient);
vm.stopPrank();
​
usdc.mint(seller, 1000e6);
usdc.mint(buyer, 1000e6);
​
vm.prank(seller);
usdc.approve(address(nftDealers), type(uint256).max);
vm.prank(buyer);
usdc.approve(address(nftDealers), type(uint256).max);
}
​
function test_staleListingAfterTransfer() public {
// --- Seller mints and lists tokenId 1 ---
vm.prank(seller);
nftDealers.mintNft(); // tokenId 1
vm.prank(seller);
nftDealers.list(1, uint32(100e6)); // list for 100 USDC
​
// Listing is active
(,,,, bool isActive) = nftDealers.s_listings(1);
assertTrue(isActive);
​
// --- Seller transfers NFT to recipient via ERC721 transferFrom ---
vm.prank(seller);
nftDealers.transferFrom(seller, recipient, 1);
​
// Recipient now owns the NFT
assertEq(nftDealers.ownerOf(1), recipient);
​
// Listing is STILL active despite ownership change
(,,,, bool stillActive) = nftDealers.s_listings(1);
assertTrue(stillActive);
​
// --- Buyer tries to buy -> REVERTS ---
// buy() calls _safeTransfer(listing.seller, buyer, 1, "") at L150
// but listing.seller (seller) no longer owns tokenId 1 -> reverts
vm.prank(buyer);
vm.expectRevert();
nftDealers.buy(1);
​
// Buyer's USDC is safe (entire tx reverted)
assertEq(usdc.balanceOf(buyer), 1000e6);
​
// --- New owner cannot re-list: isActive is still true ---
vm.prank(recipient);
vm.expectRevert("NFT is already listed");
nftDealers.list(1, uint32(150e6));
​
// activeListingsCounter is inflated
assertEq(nftDealers.totalActiveListings(), 1);
​
console.log("Listing still active but unbuyable");
console.log("New owner blocked from re-listing");
console.log("Only original seller can cancel to unblock");
}
}

Recommended Mitigation

Override ERC721's _update() hook to automatically invalidate active listings when token ownership changes. This is minimally invasive and keeps the non-escrow design intact.

+function _update(address to, uint256 tokenId, address auth) internal override returns (address from) {
+ from = super._update(to, tokenId, auth);
+ if (s_listings[tokenId].isActive) {
+ s_listings[tokenId].isActive = false;
+ activeListingsCounter--;
+ emit NFT_Dealers_ListingCanceled(tokenId);
+ }
+ return from;
+}

This fix:

  1. Automatically deactivates listings when the NFT is transferred via any ERC721 method (transferFrom, safeTransferFrom, or internal _safeTransfer in buy())

  2. Keeps activeListingsCounter accurate

  3. Frees the new owner to re-list without depending on the original seller

  4. Preserves the non-escrow design -- no changes to list() or buy()

  5. Note: cancelListing() returns collateral to listing.seller (L165), so if the transfer hook deactivates the listing, the original seller would need to claim collateral separately -- consider adding a claimCollateral() function or triggering the return in the hook

Support

FAQs

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

Give us feedback!