Bid Beasts
First Flight #49
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

Risk of Locked NFTs on Contract Receivers

objectplayer

Root + Impact

Description

  • The normal behavior when transferring NFTs to contract addresses should be to ensure the recipient contract can properly handle ERC721 tokens by implementing the onERC721Received function, which is typically done using safeTransferFrom instead of transferFrom.

  • The specific issue in the BidBeastsNFTMarket contract is that it uses transferFrom instead of safeTransferFrom when transferring NFTs to bidders or back to sellers, without verifying if the recipient is a contract that can handle NFTs.

// In _executeSale function
BBERC721.transferFrom(address(this), bid.bidder, tokenId); @> // No check if bid.bidder is a contract
​
// In unlistNFT function
BBERC721.transferFrom(address(this), msg.sender, tokenId); @> // No check if msg.sender is a contract

Risk

Likelihood: Medium

  • This will occur whenever an NFT is transferred to a contract address that does not implement the ERC721Receiver interface.

  • Smart contracts are frequently used as escrow services, multi-sig wallets, or other NFT management tools, making transfers to contracts common.

Impact: High

  • NFTs transferred to non-compliant contracts will be permanently locked with no recovery mechanism.

  • This could result in valuable assets being irretrievably lost when the auction completes or when sellers unlist their NFTs.

  • The marketplace has no emergency recovery mechanism for such scenarios.

Proof of Concept

Consider the following scenario:

// Non-compliant contract without onERC721Received
contract SimpleWallet {
address public owner;
constructor() {
owner = msg.sender;
}
// No onERC721Received function implemented
function placeBidOnMarketplace(address marketplace, uint256 tokenId) external payable {
BidBeastsNFTMarket(marketplace).placeBid{value: msg.value}(tokenId);
}
}
​
// Attack/Error Scenario:
// 1. User creates SimpleWallet contract to manage their assets
// 2. SimpleWallet.placeBidOnMarketplace() is called to bid on an NFT
// 3. SimpleWallet becomes highest bidder
// 4. Auction ends, _executeSale() is called
// 5. BBERC721.transferFrom(address(this), SimpleWallet, tokenId) is executed
// 6. Transfer succeeds because transferFrom doesn't check receiver compatibility
// 7. NFT is now owned by SimpleWallet, but SimpleWallet cannot transfer it out
// 8. NFT is permanently locked in SimpleWallet with no recovery mechanism

This vulnerability occurs because:

  1. Missing Safety Check: Using transferFrom instead of safeTransferFrom bypasses the crucial check that verifies if the recipient contract can handle ERC721 tokens.

  2. Transfer vs. Safe Transfer: While transferFrom will successfully transfer the token to the contract address, the recipient contract needs to implement onERC721Received to properly manage the token.

  3. No Recovery Mechanism: Once the NFT is transferred to a non-compliant contract, there is no way to recover it, resulting in permanent loss.

Recommended Mitigation

// In _executeSale function
- BBERC721.transferFrom(address(this), bid.bidder, tokenId);
+ BBERC721.safeTransferFrom(address(this), bid.bidder, tokenId);
​
// In unlistNFT function
- BBERC721.transferFrom(address(this), msg.sender, tokenId);
+ BBERC721.safeTransferFrom(address(this), msg.sender, tokenId);

The mitigation replaces all instances of transferFrom with safeTransferFrom, which provides the following benefits:

  1. Recipient Validation: safeTransferFrom checks if the recipient is a contract, and if so, verifies it implements the onERC721Received interface.

  2. Prevents Locked Tokens: If the recipient contract does not implement this interface, the transfer fails, preventing tokens from being locked.

  3. Industry Standard: Using safeTransferFrom is considered best practice in the industry specifically to prevent this type of token locking.

  4. User Protection: This change protects users who may not understand the technical distinction between accounts and contracts.

This simple change significantly enhances the safety of the platform and protects valuable NFT assets from being permanently lost due to transfer to incompatible contracts.

Updates

Lead Judging Commences

cryptoghost Lead Judge 2 months ago
Submission Judgement Published
Validated
Assigned finding tags:

BidBeasts Marketplace: Risk of Locked NFTs

Non-safe transferFrom calls can send NFTs to non-compliant contracts, potentially locking them permanently.

Support

FAQs

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

Give us feedback!