Skip to content

Latest commit

 

History

History
59 lines (48 loc) · 3.32 KB

M-08.md

File metadata and controls

59 lines (48 loc) · 3.32 KB
Raw

Risk of accidental DoS while receiving NFTs from marketplaces

The implementation of onERC721Received can lead to an accidental denial of service.

Impact

The Particle protocol supports creating liens by pushing the NFT instead of the usual pull approach. This is implemented by leveraging the onERC721Received() hook that gets called during NFT transfers using safeTransferFrom().

https://github.com/code-423n4/2023-05-particle/blob/main/contracts/protocol/ParticleExchange.sol#L74-L93

74:     function onERC721Received(
75:         address operator,
76:         address from,
77:         uint256 tokenId,
78:         bytes calldata data
79:     ) external returns (bytes4) {
80:         if (data.length == 64) {
81:             if (registeredMarketplaces[operator]) {
82:                 /// @dev transfer coming from registeredMarketplaces will go through buyNftFromMarket, where the NFT
83:                 /// is matched with an existing lien (realize PnL) already. If procceds here, this NFT will be tied
84:                 /// with two liens, which creates divergence.
85:                 revert Errors.Unauthorized();
86:             }
87:             /// @dev MAX_PRICE and MAX_RATE should each be way below bytes32
88:             (uint256 price, uint256 rate) = abi.decode(data, (uint256, uint256));
89:             /// @dev the msg sender is the NFT collection (called by safeTransferFrom's _checkOnERC721Received check)
90:             _supplyNft(from, msg.sender, tokenId, price, rate);
91:         }
92:         return this.onERC721Received.selector;
93:     }

There is a particular scenario that is handled differently. Line 81 checks if the operator is a registered marketplace and reverts the operation, as long as the condition in line 80 also holds (data.length == 64).

This can be concerning while executing purchases in registered marketplaces as part of transactions to the buyNftFromMarket() function. If by coincidence a marketplace transfers the NFT and submits data in the callback with a length of 64 bytes, then the transaction will be reverted. A payload of 64 bytes can be quite common, as ABI encoding uses 32 bytes for elements, this can be two basic types (two addresses, two integers, an address and an integer, etc.), or an empty array, or an empty bytes sequence, among other cases.

The issue can lead to a potential unexpected denial of service while executing purchases in a registered marketplace, preventing and blocking the buyNftFromMarket() process.

Recommendation

This can be easily fixed by avoiding the creation of liens if the operator is a registered marketplace and simply acknowledging the callback (by returning this.onERC721Received.selector) instead of reverting.

  function onERC721Received(
      address operator,
      address from,
      uint256 tokenId,
      bytes calldata data
  ) external returns (bytes4) {
      if (data.length == 64 && !registeredMarketplaces[operator]) {
          /// @dev MAX_PRICE and MAX_RATE should each be way below bytes32
          (uint256 price, uint256 rate) = abi.decode(data, (uint256, uint256));
          /// @dev the msg sender is the NFT collection (called by safeTransferFrom's _checkOnERC721Received check)
          _supplyNft(from, msg.sender, tokenId, price, rate);
      }
      return this.onERC721Received.selector;
  }