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

Unsafe NFT Transfers Locking Tokens in Non-Compliant Contracts

empyrean

Unsafe NFT Transfers Locking Tokens in Non-Compliant Contracts

Description

  • NFT transfers to/from marketplace use transferFrom for listing, unlisting, and settlement.

  • Lacks safeTransferFrom, so tokens sent to contracts without onERC721Received hook become irretrievable.

function listNFT(uint256 tokenId, uint256 _minPrice, uint256 _buyNowPrice) external {
// ...
@>BBERC721.transferFrom(msg.sender, address(this), tokenId);@>
}
​
function unlistNFT(uint256 tokenId) external {
// ...
@>BBERC721.transferFrom(address(this), msg.sender, tokenId);@>
}
​
function _executeSale(uint256 tokenId) internal {
// ...
@>BBERC721.transferFrom(address(this), bid.bidder, tokenId);@>
}

Risk

Likelihood:

  • When bidder is a contract lacking ERC721 receiver implementation.

  • During settlement to automated or legacy smart wallets.

Impact:

  • Permanent NFT lock for winner.

  • Loss of NFT from circulation at large.

Proof of Concept

Deploys contract without receiver hook, bids via it and settles auction, asserts NFT owned by contract but can't transfer out due to no functions, showing lock.

contract NonReceiver {
// No onERC721Received
}
​
function testNFTLockedInNonCompliantReceiver() public {
_mintNFT();
_listNFT();
​
NonReceiver nonReceiver = new NonReceiver();
​
// Bid via non-receiver (simulate win)
vm.deal(address(nonReceiver), MIN_PRICE + 0.01 ether);
vm.prank(address(nonReceiver));
market.placeBid{value: MIN_PRICE + 0.01 ether}(TOKEN_ID);
​
// Settle: transfers to nonReceiver, but no hook call
vm.warp(block.timestamp + 16 minutes);
market.settleAuction(TOKEN_ID);
​
// Locked: cannot transfer out
assertEq(nft.ownerOf(TOKEN_ID), address(nonReceiver));
// nonReceiver as a contract has no function to transfer or approve any address to spend the NFT
}

Recommended Mitigation

Swaps transferFrom to safeTransferFrom for reverts on non-compliant; adds pending winner mapping and claim function with safeTransferFrom for user-controlled receipt.

function listNFT(uint256 tokenId, uint256 _minPrice, uint256 _buyNowPrice) external {
// ...
- BBERC721.transferFrom(msg.sender, address(this), tokenId);
+ BBERC721.safeTransferFrom(msg.sender, address(this), tokenId);
}
​
function unlistNFT(uint256 tokenId) external {
// ...
- BBERC721.transferFrom(address(this), msg.sender, tokenId);
+ BBERC721.safeTransferFrom(address(this), msg.sender, tokenId);
}
​
function _executeSale(uint256 tokenId) internal {
// ...
- BBERC721.transferFrom(address(this), bid.bidder, tokenId);
+ // Defer to claim; set pending winner
+ pendingWinners[tokenId] = bid.bidder;
}
​
+// Add claim function
+mapping(uint256 => address) public pendingWinners;
+
+function claimNFT(uint256 tokenId, address receiver) external {
+ require(pendingWinners[tokenId] == msg.sender, "Not winner");
+ delete pendingWinners[tokenId];
+ BBERC721.safeTransferFrom(address(this), receiver, tokenId);
+}
Updates

Lead Judging Commences

cryptoghost Lead Judge about 1 month 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.