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

Unsafe NFT transfers: marketplace uses transferFrom instead of safeTransferFrom → tokens can be irrecoverably stuck in non-receiver contracts

berkep1914

Root + Impact

Description

  • Normal behavior: Marketplaces should transfer NFTs with safeTransferFrom so that, when the recipient is a contract, the call reverts unless it implements IERC721Receiver. This prevents NFTs from becoming stuck in contracts that can’t handle ERC-721.

  • Issue: Both settlement and unlisting use plain transferFrom(...). Transfers to contracts without onERC721Received succeed silently, assigning ownership to an address that may not expose any method to move the NFT, effectively bricking the asset.

function _executeSale(uint256 tokenId) internal {
...
- BBERC721.transferFrom(address(this), bid.bidder, tokenId); // @> unsafe
+ // should be safeTransferFrom(...)
...
}
​
function unlistNFT(uint256 tokenId) external isListed(tokenId) isSeller(tokenId, msg.sender) {
...
- BBERC721.transferFrom(address(this), msg.sender, tokenId); // @> unsafe
+ // should be safeTransferFrom(...)
...
}
​

Risk

Likelihood:

  • Occurs whenever the recipient is a contract without onERC721Received (common for minimal wallets, proxy-less test contracts, or third-party integrations).

Impact:

  • Asset lock/bricking: NFT ownership moves to a contract that cannot forward/operate it.

Operational breakage: Users cannot reclaim or resell; support burden and reputational risk.

Proof of Concept

Goal: Show that selling/unlisting to a non-receiver contract “succeeds” yet leaves the NFT stuck.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;
​
import {Test} from "forge-std/Test.sol";
import {BidBeastsNFTMarket} from "../src/BidBeastsNFTMarketPlace.sol";
import {BidBeasts} from "../src/BidBeasts_NFT_ERC721.sol";
​
// No onERC721Received -> cannot safely receive NFTs; exposes no forwarder
contract NonReceiver { }
​
contract UnsafeTransferPoC is Test {
BidBeasts nft;
BidBeastsNFTMarket market;
NonReceiver nonReceiver;
​
address OWNER = address(0xA1);
address SELLER = address(0xB2);
uint256 constant TOKEN_ID = 0;
uint256 constant MIN_PRICE = 1 ether;
​
function setUp() public {
vm.startPrank(OWNER);
nft = new BidBeasts();
market = new BidBeastsNFTMarket(address(nft));
vm.stopPrank();
​
vm.deal(SELLER, 100 ether);
nonReceiver = new NonReceiver();
​
// mint to seller & list
vm.prank(OWNER);
nft.mint(SELLER);
​
vm.startPrank(SELLER);
nft.approve(address(market), TOKEN_ID);
// enable buy-now at 1 ether; recipient will be nonReceiver
market.listNFT(TOKEN_ID, MIN_PRICE, MIN_PRICE);
vm.stopPrank();
}
​
function test_BuyNowSendsToNonReceiver_tokenBecomesStuck_preFix() public {
// Act: buy-now from NonReceiver (EOA funds it then call via pranks)
vm.deal(address(nonReceiver), 2 ether);
vm.prank(address(nonReceiver));
market.placeBid{value: MIN_PRICE}(TOKEN_ID); // triggers _executeSale
​
// Assert: token now owned by NonReceiver
assertEq(nft.ownerOf(TOKEN_ID), address(nonReceiver));
​
// There is no way to move it out (NonReceiver has no functions / no receiver hook)
// With safeTransferFrom this call would have reverted instead of bricking the NFT.
}
}
​

Explanation: Pre-fix, transferFrom happily moves the NFT to NonReceiver. Because the contract cannot receive (no hook) and exposes no method to forward, the asset is stuck. With safeTransferFrom, the transaction would revert, protecting users.


Recommended Mitigation

  • safeTransferFrom will revert when the recipient is a contract without onERC721Received, preventing accidental bricking.

  • No interface changes; settlement/unlist will fail fast in unsafe scenarios, keeping user assets safe.

  • If you intentionally allow raw transferFrom only to EOAs, add a guard:
    require(to.code.length == 0, "recipient contract must implement IERC721Receiver"); (but safeTransferFrom is the standard, safer default).

--- a/src/BidBeastsNFTMarketPlace.sol
+++ b/src/BidBeastsNFTMarketPlace.sol
@@ function _executeSale(uint256 tokenId) internal {
- BBERC721.transferFrom(address(this), bid.bidder, tokenId);
+ // Prevent bricking NFTs in contracts that can't receive ERC-721
+ BBERC721.safeTransferFrom(address(this), bid.bidder, tokenId);
@@ function unlistNFT(uint256 tokenId) external isListed(tokenId) isSeller(tokenId, msg.sender) {
- BBERC721.transferFrom(address(this), msg.sender, tokenId);
+ // Use safe transfer when returning the NFT to the seller
+ BBERC721.safeTransferFrom(address(this), msg.sender, tokenId);
}
​
Updates

Lead Judging Commences

cryptoghost Lead Judge 3 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!