collectUsdcFromSelling() does not mark a sold listing as claimed and does not clear the token collateral after payout. As a result, the recorded seller can repeatedly call settlement for the same inactive sold listing and drain shared USDC from the contract while also inflating totalFeesCollected.
Description
-
Under normal behavior, once an NFT is sold, the seller should be able to settle that sale exactly once and receive the sale proceeds minus protocol fees plus the locked collateral for that token.
-
However, collectUsdcFromSelling() does not record that settlement already happened. The function only checks that the caller is the recorded seller and that the listing is inactive. After a successful payout, neither the collateral nor any claim status is cleared, so the same seller can call the function again for the same sold listing and receive the same payout multiple times.
The root cause is that the function never transitions the sold listing into a "claimed" state and never clears the collateral after settlement:
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:
-
Every successful purchase sets the listing to inactive, which is the only lifecycle condition required by collectUsdcFromSelling() besides onlySeller.
-
The recorded seller remains authorized to call the function after the first settlement because no claimed state is ever stored.
Impact:
-
A seller can repeatedly withdraw the same sale proceeds and collateral from the contract’s pooled USDC balance.
-
* Each repeated call also increments totalFeesCollected, allowing protocol fee accounting to become overstated and enabling later owner withdrawals to consume user-backed funds.
Proof of Concept
Used developer`s set up
function test_ReplayedCollectUsdcFromSelling_PaysSameListingTwice() public revealed {
uint256 tokenId = 1;
uint32 nftPrice = 1000e6;
uint256 fees = nftDealers.calculateFees(nftPrice);
uint256 collateralToReturn = nftDealers.lockAmount();
uint256 amountToSeller = nftPrice + collateralToReturn - fees;
usdc.mint(address(nftDealers), 2000e6);
mintAndListNFTForTesting(tokenId, nftPrice);
vm.startPrank(userWithEvenMoreCash);
usdc.approve(address(nftDealers), nftPrice);
nftDealers.buy(tokenId);
vm.stopPrank();
uint256 sellerBalanceBefore = usdc.balanceOf(userWithCash);
uint256 contractBalanceBefore = usdc.balanceOf(address(nftDealers));
vm.prank(userWithCash);
nftDealers.collectUsdcFromSelling(tokenId);
uint256 sellerBalanceAfterFirstCollect = usdc.balanceOf(userWithCash);
uint256 contractBalanceAfterFirstCollect = usdc.balanceOf(address(nftDealers));
uint256 feesAfterFirstCollect = nftDealers.totalFeesCollected();
vm.prank(userWithCash);
nftDealers.collectUsdcFromSelling(tokenId);
uint256 sellerBalanceAfterSecondCollect = usdc.balanceOf(userWithCash);
uint256 contractBalanceAfterSecondCollect = usdc.balanceOf(address(nftDealers));
uint256 feesAfterSecondCollect = nftDealers.totalFeesCollected();
assertEq(sellerBalanceAfterFirstCollect - sellerBalanceBefore, amountToSeller);
assertEq(sellerBalanceAfterSecondCollect - sellerBalanceAfterFirstCollect, amountToSeller);
assertEq(contractBalanceBefore - contractBalanceAfterFirstCollect, amountToSeller);
assertEq(contractBalanceAfterFirstCollect - contractBalanceAfterSecondCollect, amountToSeller);
assertEq(feesAfterFirstCollect, fees);
assertEq(feesAfterSecondCollect, fees * 2);
}
Recommended Mitigation
+ mapping(uint256 => bool) public settlementClaimed;
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive to collect USDC");
+ require(!settlementClaimed[_listingId], "Settlement already claimed");
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
+ settlementClaimed[_listingId] = true;
+ collateralForMinting[listing.tokenId] = 0;
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
- usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
}
