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

Denial of Service (DoS) via External NFT Transfer

Author Revealed upon completion

Root + Impact

Description

The buy function assumes that the NFTDealers contract still has the authority to transfer the NFT when a buyer calls the function. However, the contract does not escrow the NFT (it doesn't take custody) when a user calls list().

  • Normal Behavior: A user lists an NFT, and when a buyer pays the specified price, the NFT is transferred from the seller (or the contract) to the buyer.

  • Specific Issue: Because the NFT remains in the seller's wallet after listing, the seller can transfer the NFT to another wallet or sell it on another marketplace (like OpenSea) while the listing on NFTDealers remains "active." When a buyer attempts to purchase the listing, the _safeTransfer call will fail because the contract no longer has the rights to move an NFT that the "seller" no longer owns.

function buy(uint256 _listingId) external payable {
Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
// ... payment logic ...
​
// @> Root Cause: If the seller transferred the NFT elsewhere, this reverts
// @> blocking the entire buy transaction and locking the buyer's attempt.
_safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
​
s_listings[_listingId].isActive = false;
}

Risk

Likelihood: High

  • A seller can intentionally or accidentally move the NFT after listing it.

  • There is no synchronization between the blockchain's ownerOf state and the contract's s_listings state until the buy function is called.

Impact: Medium

  • Denial of Service: Buyers will experience reverted transactions and wasted gas when trying to buy "ghost" listings.

  • Protocol Integrity: The activeListingsCounter will become permanently inflated as these "dead" listings can never be successfully bought or cleared by anyone other than the seller (via cancellation).

Proof of Concept

Paste this test function in NFTDealersTest.t.sol

function test_externalNftTransferCauseBuyDoS() public revealed {
uint256 tokenId = 1;
uint32 initialPrice = 10e6;
mintAndListNFTForTesting(tokenId, initialPrice);
​
address user = makeAddr("user");
vm.prank(userWithCash);
IERC721(address(nftDealers)).transferFrom(userWithCash, user, tokenId);
​
address buyer = makeAddr("buyer");
deal(address(usdc), buyer, 100e6);
vm.prank(buyer);
usdc.approve(address(nftDealers), 100e6);
vm.prank(buyer);
vm.expectRevert();
nftDealers.buy(tokenId);
}

Recommended Mitigation

The standard practice for marketplaces is to escrow the NFT. When list() is called, the contract should take custody of the NFT. This ensures the NFT is guaranteed to be available when a buyer appears.

function list(uint256 _tokenId, uint32 _price) external onlyWhitelisted {
// ... validation ...
​
+ // Transfer NFT to contract to escrow it
+ transferFrom(msg.sender, address(this), _tokenId);
​
s_listings[_tokenId] = Listing({
seller: msg.sender,
price: _price,
nft: address(this),
tokenId: _tokenId,
isActive: true
});
}
​
function buy(uint256 _listingId) external payable {
// ... logic ...
- _safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
+ _safeTransfer(address(this), msg.sender, listing.tokenId, "");
}

Support

FAQs

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

Give us feedback!