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

DoS Risk: `BidBeastsNFTMarket::unlistNFT` Can Be Blocked by Malicious Contract

0xzhd

Root + Impact

  • Root Cause: The unlistNFT function uses BBERC721.transferFrom to return an NFT to the seller (msg.sender), but it lacks a mechanism to handle cases where msg.sender is a malicious smart contract that reverts in its onERC721Received implementation. This revert causes the entire transaction to fail, preventing the NFT from being unlisted.

  • Impact: Sellers cannot unlist their NFTs if the recipient is a malicious contract, potentially locking NFTs in the marketplace indefinitely. This creates a denial-of-service (DoS) risk, disrupting seller operations and risking financial loss due to inaccessible assets, while undermining trust in the platform.

Description:

The BidBeastsNFTMarket::unlistNFT function transfers an NFT back to the seller using:

function unlistNFT(uint256 tokenId) external isListed(tokenId) isSeller(tokenId, msg.sender) {
require(bids[tokenId].bidder == address(0), "Cannot unlist, a bid has been placed");
Listing storage listing = listings[tokenId];
listing.listed = false;
​
@> BBERC721.transferFrom(address(this), msg.sender, tokenId);
​
emit NftUnlisted(tokenId);
}

If msg.sender is a malicious smart contract, it can revert the transfer (e.g., via a malicious onERC721Received implementation). This will cause the entire unlistNFT transaction to revert, effectively blocking the seller from unlisting their NFT.

Risk

  • Likelihood: Medium. The vulnerability depends on a seller interacting with the function via a malicious contract, which requires deliberate action (e.g., a compromised wallet or targeted attack). The likelihood increases in environments with untrusted or automated contract interactions.

  • Impact: Medium. While not causing immediate asset theft, the inability to unlist locks NFTs, leading to potential financial loss and operational disruption. The impact is significant for affected sellers and could damage platform reputation if exploited widely.

Proof of Concept:

  1. Deploy a malicious contract that reverts in onERC721Received.

  2. List an NFT and attempt to unlist it with the malicious contract as msg.sender.

  3. Observe that unlistNFT reverts every time, preventing the NFT from being returned.

contract MaliciousReceiver {
function onERC721Received(
address operator,
address from,
uint256 tokenId,
bytes memory data
) public pure returns (bytes4) {
revert("Malicious revert");
return this.onERC721Received.selector;
}
}

Recommended Mitigation:

  • Use a pull pattern: record unlisted NFTs in a mapping and allow sellers to withdraw them manually.

  • Optionally, restrict recipients to EOAs or trusted contracts.

  • Consider using try/catch around transferFrom for safety when interacting with untrusted contracts.
    Example Fix:

mapping(uint256 => address) public pendingWithdrawals;
​
function unlistNFT(uint256 tokenId) external isListed(tokenId) isSeller(tokenId, msg.sender) {
require(bids[tokenId].bidder == address(0), "Cannot unlist, a bid has been placed");
Listing storage listing = listings[tokenId];
listing.listed = false;
pendingWithdrawals[tokenId] = msg.sender;
​
emit NftUnlisted(tokenId);
}
​
function withdrawNFT(uint256 tokenId) external {
require(pendingWithdrawals[tokenId] == msg.sender, "Not authorized");
delete pendingWithdrawals[tokenId];
BBERC721.transferFrom(address(this), msg.sender, tokenId);
}

References:

  1. OpenZeppelin ERC721 Safe Transfers
    Explains the behavior of safeTransferFrom and potential reverts when interacting with contracts.
    https://docs.openzeppelin.com/contracts/4.x/api/token/erc721#ERC721-safeTransferFrom-address-address-uint256-

  2. Solidity Design Pattern: Pull Over Push
    Discusses why the pull-over-push pattern is safer for sending tokens or ETH to potentially untrusted contracts.
    https://solidity-by-example.org/pull-over-push/

  3. ERC721 Receiver Interface (IERC721Receiver)
    Details how contracts can implement onERC721Received and how a revert can block transfers.
    https://eips.ethereum.org/EIPS/eip-721#erc-721-token-receiver

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.