Root + Impact

Description

• The unlistNFT function has a few problems that can break the auction flow or even lock NFTs. When unlisting, the function blindly calls. If the contract doesn’t actually hold the token (for example NFT already transfered), the call reverts with ERC721NonexistentToken or NotOwner. This is exactly what happened in provided test, which expected "Not the owner" but instead failed with ERC721NonexistentToken.

• State updated before external call
  The code sets listing.listed = false before attempting the transfer. If the transfer fails, the contract is left in an inconsistent state: the listing is marked unlisted but the NFT is not returned.

Unsafe transfer method
Using transferFrom can permanently lock NFTs if the recipient is a contract that does not implement onERC721Received. safeTransferFrom should be used instead to guarantee safe delivery.

Risk

Likelihood:High

• Sellers or bidders are very likely to run into these scenarios during normal use (trying to unlist at the wrong time, or if bids were placed and refunded).

Impact:High

• NFTs can be stuck in the contract or lost.

• Auctions can end in an inconsistent state.

• Breaks fairness for bidders and reliability of the marketplace.

Proof of Concept

In this test, a non-owner attempts to call unlistNFT. The expectation is that the call reverts with "Not the owner". Instead, the transaction fails with ERC721NonexistentToken(0). This shows that the contract wrongly assumes it holds custody of the NFT, and the external transfer call reverts with a low-level ERC721 error. In a real scenario, this could happen if the NFT was transferred away or the auction state was corrupted, leaving the system inconsistent.

Step-by-step:

1. A non-owner calls unlistNFT.

2. The contract checks isSeller and allows execution.

3. It attempts transferFrom(address(this), msg.sender, tokenId) even though the contract does not own the NFT.

4. The call reverts with a low-level ERC721 error (ERC721NonexistentToken).

5. This demonstrates the function assumes NFT custody incorrectly and fails in practice.

In a real deployment, this could lead to inconsistent auction state or NFTs stuck inside the contract.

Recommended Mitigation

The safest fix is to only mark an auction as unlisted after the NFT is successfully returned. Using safeTransferFrom prevents NFTs from being stuck if the receiver is a contract that doesn’t handle ERC721 tokens. Finally, adding a check for listing.endTime ensures sellers cannot unlist after the auction expired, which would otherwise break settlement logic.

Add an explicit check that the auction is still active.

Use safeTransferFrom instead of transferFrom.