Bid Beasts
First Flight #49
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Impact: low
Likelihood: medium
Invalid

NFT Approval Issue in listNFT()

objectplayer

Root + Impact

The marketplace contract relies on users to externally approve NFT transfers before listing, which is a common source of confusion and transaction failures in NFT marketplaces.
Users who try to list NFTs without first approving the marketplace contract will experience failed transactions, leading to a poor user experience and increased gas costs due to failed transactions.

Description

  • The normal behavior in NFT marketplaces is to either explicitly handle the approval process within the contract or clearly document the requirement for pre-approval.

  • The specific issue is that the listNFT function in the marketplace contract attempts to transfer NFTs from the seller to the contract using transferFrom, but there is no mechanism within the contract to ensure proper approval has been granted.

function listNFT(uint256 tokenId, uint256 _minPrice, uint256 _buyNowPrice) external {
require(BBERC721.ownerOf(tokenId) == msg.sender, "Not the owner");
// ... other validations ...
BBERC721.transferFrom(msg.sender, address(this), tokenId); @> // No check or handling of approvals
// ... rest of the function ...
}

Risk

Likelihood: Medium

  • This issue affects every new listing attempt where the user hasn't previously approved the marketplace.

  • Transaction failures due to missing approvals are a common issue in NFT marketplaces

Impact: Low

  • No funds or NFTs are at risk, but users will experience failed transactions.

  • Users must understand they need to first approve the marketplace to transfer their NFTs, creating additional steps and potential confusion.

  • Failed transactions result in wasted gas fees for users.

Proof of Concept

Here's a demonstration of the issue:

async function demonstrateApprovalRequirement() {
// Setup: Deploy contracts
const nft = await BidBeasts.deploy();
const marketplace = await BidBeastsNFTMarket.deploy(nft.address);
// Mint an NFT to the user
const tokenId = await nft.mint(user.address);
console.log("Attempting to list NFT without approval...");
// Try to list the NFT without approving the marketplace
try {
await marketplace.connect(user).listNFT(
tokenId,
ethers.utils.parseEther("1.0"),
ethers.utils.parseEther("2.0")
);
console.log("Listing succeeded (This shouldn't happen!)");
} catch (error) {
console.log("Listing failed as expected:", error.message);
console.log("The error indicates the marketplace is not approved to transfer the NFT");
}
// Now approve the marketplace and try again
console.log("\nApproving marketplace and trying again...");
await nft.connect(user).approve(marketplace.address, tokenId);
try {
await marketplace.connect(user).listNFT(
tokenId,
ethers.utils.parseEther("1.0"),
ethers.utils.parseEther("2.0")
);
console.log("Listing succeeded after approval");
} catch (error) {
console.log("Listing failed even after approval:", error.message);
}
}
​
// Expected output:
// Attempting to list NFT without approval...
// Listing failed as expected: ERC721: caller is not token owner or approved
// The error indicates the marketplace is not approved to transfer the NFT
//
// Approving marketplace and trying again...
// Listing succeeded after approval

This demonstrates that:

  1. Attempting to list an NFT without first approving the marketplace will fail

  2. The error message comes from the ERC721 contract, not from the marketplace

  3. Users must perform a separate approval transaction before they can list NFTs

Recommended Mitigation

function listNFT(uint256 tokenId, uint256 _minPrice, uint256 _buyNowPrice) external {
require(BBERC721.ownerOf(tokenId) == msg.sender, "Not the owner");
+ require(
+ BBERC721.getApproved(tokenId) == address(this) ||
+ BBERC721.isApprovedForAll(msg.sender, address(this)),
+ "Marketplace not approved to transfer NFT. Call approve() or setApprovalForAll() first"
+ );
// ... other validations ...
BBERC721.transferFrom(msg.sender, address(this), tokenId);
// ... rest of the function ...
}

The benefits of these changes:

  1. Improved User Experience: Users receive clear error messages about what they need to do or can use a combined function.

  2. Reduced Failed Transactions: With proper checks, users are less likely to submit transactions that will fail.

  3. Clear Documentation: The contract's requirements are explicitly stated in the code.

  4. Gas Savings: Users don't waste gas on transactions that are guaranteed to fail.

While this isn't a security vulnerability per se, it's a significant usability concern that could lead to a poor user experience and unnecessary transaction failures. Implementing these changes would make the marketplace more user-friendly and reduce friction for users listing NFTs.

Updates

Lead Judging Commences

cryptoghost Lead Judge 2 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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

Give us feedback!