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

Unsafe ERC721 transfers to arbitrary contracts

fffuuuming

Root + Impact

Description

  • The market uses transferFrom rather than safeTransferFrom for outgoing transfers

  • If bidder is a contract that cannot handle ERC721, tokens can be effectively lost for that bidder

function _executeSale(uint256 tokenId) internal {
Listing storage listing = listings[tokenId];
Bid memory bid = bids[tokenId];
​
listing.listed = false;
delete bids[tokenId];
@> // transfer token out without safety check
BBERC721.transferFrom(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);
}

Risk

Likelihood: Medium

  • Any non-IERC721Receiver contract that wins a bid may trigger the issue.

Impact: High

  • NFT become unrecoverable, permanently locking a high-value token

  • user asset loss

Proof of Concept

Add the following test, then run this command: forge test --match-test test_unsafeERC721Transfer

function test_unsafeERC721Transfer() public {
vm.deal(address(rejector), STARTING_BALANCE);
​
_mintNFT();
_listNFT();
​
vm.prank(address(rejector));
market.placeBid{value: MIN_PRICE + 1 ether}(TOKEN_ID);
​
vm.prank(SELLER);
market.takeHighestBid(TOKEN_ID);
​
assertEq(nft.ownerOf(TOKEN_ID), address(rejector), "NFT should be held by the rejector");
assertEq(market.getListing(TOKEN_ID).listed, false, "Listing should be marked as unlisted");
}

Recommended Mitigation

  • Use safeTransferFrom(address(this), bid.bidder, tokenId) for outgoing trasnfer, so it will revert if the recipient is non-IERC721Receiver

function _executeSale(uint256 tokenId) internal {
Listing storage listing = listings[tokenId];
Bid memory bid = bids[tokenId];
​
listing.listed = false;
delete bids[tokenId];
+ BBERC721.safeTransferFrom(address(this), bid.bidder, tokenId);
- BBERC721.transferFrom(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);
}
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.