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

Unsafe NFT transfers in the protocol

fredo182

Unsafe NFT transfers in the protocol

Description

The protocol transfers NFTs from owners to itself and from itself to other users. ERC721 defines safe functions that check whether a smart contract can safely handle NFTs in order to prevent permant locks. However, the current market place uses the unsafe transferFrom function.

Risk

Likelihood: Medium

EOA present no issue, but contracts as receivers are common.

Impact: Medium

Transferring an NFT into a contract that does not implement ERC-721 receiving semantics can leave the token irretrievable.

Proof of Concept

Create a contract that rejects NFT transfers

contract RejectNFTReceiver {
function onERC721Received(address, address, uint256, bytes calldata) external pure returns (bytes4) {
revert("RejectNFTReceiver: revert on onERC721Received");
}
}

Use it as NFT receiver

function test_unsafe_transfer() public {
RejectNFTReceiver rejectNFTReceiver = new RejectNFTReceiver();
vm.deal(address(rejectNFTReceiver), STARTING_BALANCE);
​
vm.prank(PROTOCOL_OWNER);
nft.mint(SELLER);
​
uint256 askPrice = MIN_PRICE + 1;
uint256 buyNowPrice = askPrice + 1;
​
vm.startPrank(SELLER);
nft.approve(address(market), 0);
vm.expectEmit(true, true, true, true);
emit NftListed(0, SELLER, askPrice, buyNowPrice);
market.listNFT(0, askPrice, buyNowPrice);
vm.stopPrank();
​
assertEq(nft.ownerOf(0), address(market));
BidBeastsNFTMarket.Listing memory listing = market.getListing(0);
assertEq(listing.listed, true);
assertEq(listing.seller, SELLER);
assertEq(listing.minPrice, askPrice);
assertEq(listing.buyNowPrice, buyNowPrice);
assertEq(listing.auctionEnd, 0);
​
vm.warp(1 days);
​
uint256 sellerStartingBalance = SELLER.balance;
uint256 protocolFeeStartingBalance = market.s_totalFee();
vm.startPrank(address(rejectNFTReceiver));
vm.expectEmit(true, true, true, true);
emit AuctionSettled(0, address(rejectNFTReceiver), SELLER, buyNowPrice);
market.placeBid{value: buyNowPrice}(0);
vm.stopPrank();
uint256 sellerEndingBalance = SELLER.balance;
uint256 protocolFeeEndingBalance = market.s_totalFee();
​
listing = market.getListing(0);
assertEq(listing.listed, false);
assertEq(listing.seller, SELLER);
assertEq(listing.minPrice, askPrice);
assertEq(listing.buyNowPrice, buyNowPrice);
assertEq(listing.auctionEnd, 0);
​
BidBeastsNFTMarket.Bid memory bid = market.getHighestBid(0);
assertEq(bid.bidder, address(0));
assertEq(bid.amount, 0);
​
assertEq(nft.ownerOf(0), address(rejectNFTReceiver));
}

Recommended Mitigations

Use safeTransferFrom for custody changes

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.