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

Unsafe NFT Transfer to Contract Addresses Can Permanently Lock NFTs

yoyi

Unsafe NFT Transfer to Contract Addresses Can Permanently Lock NFTs

Description

  • The protocol should safely transfer NFTs to winning bidders while ensuring the recipient can properly handle ERC721 tokens to prevent permanent asset loss.

  • The _executeSale function uses transferFrom instead of safeTransferFrom when transferring NFTs to auction winners, which can permanently lock NFTs in contracts that don't implement proper ERC721 receiver functions.

function _executeSale(uint256 tokenId) internal {
Listing storage listing = listings[tokenId];
Bid memory bid = bids[tokenId];
​
listing.listed = false;
delete bids[tokenId];
​
@> BBERC721.transferFrom(address(this), bid.bidder, tokenId); //Uses unsafe transfer - NFT can be locked forever
​
uint256 fee = (bid.amount * S_FEE_PERCENTAGE) / 100;
s_totalFee += fee;
uint256 sellerProceeds = bid.amount - fee;
_payout(listing.seller, sellerProceeds);
​
emit AuctionSettled(tokenId, bid.bidder, listing.seller, bid.amount);
}

Risk

Likelihood:

  • This occurs whenever a contract address wins an auction and that contract doesn't implement onERC721Received or implement it incorrectly

  • Smart contract wallets, multisig wallets, or other contract-based bidders are common in NFT marketplaces.

Impact:

  • NFTs become permanently locked in recipient contracts that cannot handle ERC721 tokens, making them unrecoverable.

  • Winners lose access to NFTs they legitimately purchased, creating disputes and potential legal issues.

Proof of Concept

First we need to make a quick fix in test/BidBeastsMarketPlaceTest.t.sol:BidBeastsNFTMarketTest::setUp()

function setUp() public {
// Deploy contracts
- vm.prank(OWNER);
+ vm.startPrank(OWNER);
nft = new BidBeasts();
market = new BidBeastsNFTMarket(address(nft));
rejector = new RejectEther();
​
vm.stopPrank();
​
// Fund users
vm.deal(SELLER, STARTING_BALANCE);
vm.deal(BIDDER_1, STARTING_BALANCE);
vm.deal(BIDDER_2, STARTING_BALANCE);
}

Please add the following test to test/BidBeastsMarketPlaceTest.t.sol:

contract RejectEther {
// Intentionally has no payable receive or fallback
function bid(BidBeastsNFTMarket market, uint256 tokenId, uint256 amount) external payable {
market.placeBid{value: amount}(tokenId);
}
}
​
contract BidBeastsNFTMarketTest is Test {
// existing code
function testBadReceiver() public {
_mintNFT();
_listNFT();
/* ------------------------------ rejector bids ----------------------------- */
vm.deal(address(rejector), 8 ether);
rejector.bid(market, TOKEN_ID, BUY_NOW_PRICE);
assertEq(address(rejector), nft.ownerOf(TOKEN_ID));
}
// existing code
}

Then run forge test --mt testBadReceiver

Output:

Ran 1 test for test/BidBeastsMarketPlaceTest.t.sol:BidBeastsNFTMarketTest
[PASS] testBadReceiver() (gas: 290096)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.75ms (160.29µs CPU time)

Recommended Mitigation

Use safeTransferFrom instead of transferFrom to ensure safe NFT transfers:

function _executeSale(uint256 tokenId) internal {
Listing storage listing = listings[tokenId];
Bid memory bid = bids[tokenId];
​
listing.listed = false;
delete bids[tokenId];
​
- BBERC721.transferFrom(address(this), bid.bidder, tokenId);
+ BBERC721.safeTransferFrom(address(this), bid.bidder, tokenId);
​
uint256 fee = (bid.amount * S_FEE_PERCENTAGE) / 100;
s_totalFee += fee;
uint256 sellerProceeds = bid.amount - fee;
_payout(listing.seller, sellerProceeds);
​
emit AuctionSettled(tokenId, bid.bidder, listing.seller, bid.amount);
}

This ensures that if the recipient is a contract, it must implement onERC721Received properly, preventing NFTs from being permanently locked.

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.