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

Canceled listings can wrongfully collect “sale proceeds” because collectUsdcFromSelling() only checks isActive == false

Author Revealed upon completion

Root + Impact

Description

Under normal behavior, collectUsdcFromSelling() should only be callable after a listing has been successfully sold. A canceled listing should not unlock sale proceeds, because no buyer payment was ever made for that listing.

In the current implementation, collectUsdcFromSelling() does not verify that a sale occurred. It only verifies that the listing is inactive.
However, both buy() and cancelListing() set isActive to false. This means the function treats two fundamentally different terminal states as equivalent:

  • sold listing

  • canceled listing

Because of that, a seller can create a listing, cancel it, and still call collectUsdcFromSelling() as though the NFT had been sold. The function will then compute a payout from the stored listing.price and collateralForMinting[tokenId], even though no sale took place.

function buy(uint256 _listingId) external payable {
...
s_listings[_listingId].isActive = false; // @> inactive because sold
}
​
function cancelListing(uint256 _listingId) external {
...
s_listings[_listingId].isActive = false; // @> inactive because canceled
}
​
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive to collect USDC");
// @> assumes inactive implies sold, which is false
}

Risk

Likelihood:

  • The bug occurs whenever a seller cancels a listing and then calls collectUsdcFromSelling().

  • The exploit path is simple because cancellation directly creates the same inactive state that the payout function accepts.

Impact:

  • A seller can receive “sale proceeds” for a listing that was never sold.

  • The payout amount is derived from the stored listing price and can drain pooled USDC from the contract.

  • Legitimate users can be harmed because the contract uses a shared USDC balance rather than isolated per-sale accounting.

Proof of Concept

Paste this inside NFTDealersTest.t.sol:

function testCanceledListingCanStillCollectSaleProceeds() public revealed {
uint256 tokenId = 1;
uint32 fakeSalePrice = 1000e6;
​
mintAndListNFTForTesting(tokenId, fakeSalePrice);
​
// Cancel the listing. No buyer ever paid for this listing.
vm.prank(userWithCash);
nftDealers.cancelListing(tokenId);
​
// After cancellation, collateral has been returned, so fund the contract
// to demonstrate that collectUsdcFromSelling() still pays out based only
// on inactive listing state.
uint256 fees = nftDealers.calculateFees(fakeSalePrice);
uint256 fakeProceeds = uint256(fakeSalePrice) - fees;
usdc.mint(address(nftDealers), fakeProceeds);
​
uint256 sellerBalanceBefore = usdc.balanceOf(userWithCash);
​
vm.prank(userWithCash);
nftDealers.collectUsdcFromSelling(tokenId);
​
uint256 sellerBalanceAfter = usdc.balanceOf(userWithCash);
​
// Seller receives proceeds even though no sale happened.
assertEq(sellerBalanceAfter - sellerBalanceBefore, fakeProceeds);
}

Recommended Mitigation

Track whether a listing was actually sold, and gate collectUsdcFromSelling() on that state instead of using !isActive as a proxy.

struct Listing {
address seller;
uint32 price;
address nft;
uint256 tokenId;
bool isActive;
+ bool wasSold;
}

Support

FAQs

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

Give us feedback!