Using transferFrom in _executeSale and unlistNFT risks sending NFTs to non-ERC721-compliant contracts, causing permanent asset loss

Description

• The contract uses transferFrom to transfer NFTs in both the BidBeastsNFTMarketPlace::_executeSale and BidBeastsNFTMarketPlace::unlistNFT functions (lines 213 and 97, respectively):

  // In unlistNFT (line 98)

  @> BBERC721.transferFrom(address(this), msg.sender, tokenId);

  ​

  // In _executeSale (line 213)

  @> BBERC721.transferFrom(address(this), bid.bidder, tokenId);
  
  

• Unlike safeTransferFrom, transferFrom does not verify if the recipient contract implements onERC721Received, as required by the ERC721 standard. If the recipient is a non-compliant contract (e.g., lacking onERC721Received or NFT management logic), the transfer succeeds, but the NFT becomes permanently locked, as the contract cannot approve or transfer it out.

• This violates best practices (e.g., OpenZeppelin's recommendation to use safeTransferFrom) and risks user asset loss, especially for bidders using smart contract wallets or sellers unlisting to their own non-compliant contracts.

Risk

Likelihood: Medium

• Most bidders use EOAs or ERC721-compliant wallets, but smart contract wallets (e.g., misconfigured Gnosis Safe) are plausible.

Impact: High

• Affected users lose their NFTs permanently, with no protocol-wide impact but significant loss for individuals.

Proof of Concept

• Add this NonCompliantReceiver contract in the test file:

  contract NonCompliantReceiver {

  // Intentionally does not implement onERC721Received

  }

• After that, add this particular test_NFTLockedInNonCompliantContract test in the test file:

  function test_NFTLockedInNonCompliantContract() public {

  // Mint and list NFT

  _mintNFT();

  _listNFT();

  ​

  // Deploy non-compliant receiver

  NonCompliantReceiver receiver = new NonCompliantReceiver();

  vm.deal(address(receiver), 10 ether);

  ​

  // Bid from non-compliant contract

  vm.prank(address(receiver));

  market.placeBid{value: BID_AMOUNT}(TOKEN_ID);

  ​

  // Warp time to end auction

  vm.warp(block.timestamp + 16 minutes);

  ​

  // Settle auction to transfer NFT to receiver

  vm.prank(OWNER);

  market.settleAuction(TOKEN_ID);

  ​

  // Verify NFT is owned by receiver

  assertEq(

  nft.ownerOf(TOKEN_ID),

  address(receiver),

  "NFT should be owned by non-compliant contract"

  );

  ​

  // Now NFT is stuck in the receiver contract as it doesn't implement onERC721Received or any function to transfer it out

  }

• Run the above test using the command:

  forge test --mt test_NFTLockedInNonCompliantContract -vv

Recommended Mitigation

• Although the straightforward suggestion would be to replace transferFrom with safeTransferFrom, but it has some caveats. With this direct replacement, the contract is at risk of either some reentrancy or DoS attacks, or both. This is because the recipient contract's onERC721Received function could contain malicious code that exploits the transfer process. Here's a good write-up about safeTransferFrom by RareSkills: Safe Transfers: safeTransferFrom, _safeMint, and the onERC721Received function, definitely worth a read.

• Thus, it's better to implement a pull-based transfer mechanism, along with safeTransferFrom, where the contract credits the NFT to the recipient (by using mapping), and the recipient can then call a function to claim the NFT. This way, the transfer is initiated by the recipient, reducing the risk of reentrancy or DoS attacks.

  + mapping(uint256 => address) public nftClaims;

  ​

  function unlistNFT(uint256 tokenId) external isListed(tokenId) isSeller(tokenId, msg.sender) {

  ...

  - BBERC721.transferFrom(address(this), msg.sender, tokenId);

  + nftClaims[tokenId] = msg.sender;

  ...

  }

  ​

  function _executeSale(uint256 tokenId) internal {

  ...

  - BBERC721.transferFrom(address(this), bid.bidder, tokenId);

  + nftClaims[tokenId] = bid.bidder;

  ...

  }

  ​

  + function claimNFT(uint256 tokenId) external {

  + address claimer = nftClaims[tokenId];

  + require(claimer == msg.sender, "Not authorized to claim this NFT");

  + require(claimer != address(0), "No NFT to claim");

  +

  + nftClaims[tokenId] = address(0); // Clear claim before transfer to prevent reentrancy

  + BBERC721.safeTransferFrom(address(this), claimer, tokenId);

  + }