reNFT - hals's results

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L225-L248

Vulnerability details

Impact

• hookOnStop hook acts as a middleware contract that execute arbitrary logic when the rental order is stopped, but if the hook is malfunction and can't be executed (where its failuer is catched in the catch block and the transaction is reverted); this will result in reverting the Stop._removeHooks function that's called when the rental is stopped, which will result in permanently locking the rented assets in the renter SAFE wallet.

• Another issue with this: if a lot of hookOnStop hooks are assigned with rental order (as the renter can assign as much on-stop hooks as he can ), the Stop.stopRental transaction might revert due to out-of-gas resulting in locking the rented assets in the renter wallet, and this can be prevented by limiting the number of on-stop hooks that can be executed with each rental order.

Proof of Concept

Stop._removeHooks function / L209-L212

            // Call the hook with data about the rented item.
            try
                IHook(target).onStop(
                    rentalWallet,
                    item.token,
                    item.identifier,
                    item.amount,
                    hooks[i].extraData
                )
            {} catch Error(string memory revertReason) {
                // Revert with reason given.
                revert Errors.Shared_HookFailString(revertReason);
            } catch Panic(uint256 errorCode) {
                // Convert solidity panic code to string.
                string memory stringErrorCode = LibString.toString(errorCode);


                // Revert with panic code.
                revert Errors.Shared_HookFailString(
                    string.concat("Hook reverted: Panic code ", stringErrorCode)
                );
            } catch (bytes memory revertData) {
                // Fallback to an error that returns the byte data.
                revert Errors.Shared_HookFailBytes(revertData);
            }

Tools Used

Manual Review.

Recommended Mitigation Steps

Update Create._removeHooks function to ignore the hookOnStop if its execution didn't succeed without reverting the transaction.

Assessed type

DoS

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L479-L483 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L209-L212

Vulnerability details

Impact

• When an order is fulfilled; the openSea invokes Create.validateOrder to check if the created hash of the rental order parameters match the orderHash stored by the protocol when the order is created along with other processes.

• One of the processes includes checking if the order has hooks that must be executed before fulfilling it via Create._addHooks; where this function checks if the hook assigned to each order item is whitelisted to be executed on start :

      if (!STORE.hookOnStart(target)) {
                  revert Errors.Shared_DisabledHook(target);
              }

• So if any of these hooks are not allowed to be executed on-start (or allowd to be executed on-stop); the transaction will revert resulting in not fulfilling the order, so no losses will be incurred.

• But if all the items hooks are whitlisted to be executed on-start and there's no on-stop hooks; the order will be fulfilled, but this will result in locking the rented asset in the renter SAFE wallet; but how?

  1. When the rental is over (either its time passed if it's a BASE order, or if the lender decided to end it if it's a PAY order), the hooks associated with the order rented items will be looped over and checked if it's a valid/whitelisted on-stop hook via Stop._removeHooks function:

      if (!STORE.hookOnStop(target)) {
                  revert Errors.Shared_DisabledHook(target);
              }

  2. BUT this implementation will result in locking the rented assets, since all the hooks associated with the order items are ensured to be of whitelisted hookOnStart hooks when the order is first fulfilled (as explained above), and this will result in reverting the Stop.stopRent function resulting in permanently locking the rented items in the renter SAFE wallet.

Proof of Concept

Create._addHooks function / L479-L483

         // Check that the hook is reNFT-approved to execute on rental start.
            if (!STORE.hookOnStart(target)) {
                revert Errors.Shared_DisabledHook(target);
            }

Stop._removeHooks function / L209-L212

   // Check that the hook is reNFT-approved to execute on rental stop.
            if (!STORE.hookOnStop(target)) {
                revert Errors.Shared_DisabledHook(target);
            }

Tools Used

Manual Review.

Recommended Mitigation Steps

1. Update Create._addHooks function to accept the whitlisted hookOnStop , and execute it when the rental is stopped.

2. Update Create._removeHooks function to ignore the execution of hookOnStart if the item is assigned one without reverting the transaction.

Assessed type

DoS

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L209-L212 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L373-L378

Vulnerability details

Impact

• The Guard contract admin can blacklist/whitelist a hook via Guard.updateHookStatus function; where these hooks act as a middleware contracts that execute arbitrary logic when the rental order is fulfilled and when it's stopped, and the selection of which hooks to execute in each phase is specified by the lender when he creates the rental order.

• But there's a serious issue when the hooks are checked:

  1. The hookOnStop attached to each order item (if any is present) is not checked if whitelisted or not when the order is fulfilled and validated via Create.validateOrder, so items might have invalid/non-whitelited hooks attached to them.

  2. The Guard contract admin can change the status of the hookOnStop from being whitelisted to non-whitelisted at any time.

• This will result in reverting the Stop._removeHooks function that's called when the rental is stopped, which will result in permanently locking the rented assets in the renter SAFE wallet.

Proof of Concept

Stop._removeHooks function / L209-L212

   // Check that the hook is reNFT-approved to execute on rental stop.
            if (!STORE.hookOnStop(target)) {
                revert Errors.Shared_DisabledHook(target);
            }

Guard.updateHookStatus function

   function updateHookStatus(
        address hook,
        uint8 bitmap
    ) external onlyRole("GUARD_ADMIN") {
        STORE.updateHookStatus(hook, bitmap);
    }

Tools Used

Manual Review.

Recommended Mitigation Steps

Update Create._removeHooks function to ignore the execution of hookOnStop if the item is assigned one without reverting the transaction.

Assessed type

DoS

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L195-L293

Vulnerability details

Impact

• When a rental order is fulfilled, the rented assets (ERC721/ERC1155) tokens are transferred to the renter SAFE wallet that's created and whitelisted by the protocol, where it has a Guard contract set to guard the transactions originated from the rental wallet where it prevents delegatecalls and calls that aim to transfer the assets from the wallet or approve them.

• All these checks are done in Guard.checkTransaction function by checking if the function selector is allowed or not, if allowed then the transaction can be executed, and if not; the transaction will revert.

• But it was noticed that any malicious renter can burn the rented asset as there's no limitation on calling the burn function on the rented token, since this function selector isn't standard between token contracts and usually depends on the token contract implementation (as it might be burn(uint256 tokenId) or burn(address from,uint256 tokenId),etc...).

• This will result in burning the rented assets from the renter wallet, as there's no check after the transaction execution if the assets still exist in the wallet or not.

Proof of Concept

Guard._checkTransaction function

function _checkTransaction(address from, address to, bytes memory data) private view {
        bytes4 selector;

        // Load in the function selector.
        assembly {
            selector := mload(add(data, 0x20))
        }

        if (selector == e721_safe_transfer_from_1_selector) {
            // Load the token ID from calldata.
            uint256 tokenId = uint256(
                _loadValueFromCalldata(data, e721_safe_transfer_from_1_token_id_offset)
            );

            // Check if the selector is allowed.
            _revertSelectorOnActiveRental(selector, from, to, tokenId);
        } else if (selector == e721_safe_transfer_from_2_selector) {
            // Load the token ID from calldata.
            uint256 tokenId = uint256(
                _loadValueFromCalldata(data, e721_safe_transfer_from_2_token_id_offset)
            );

            // Check if the selector is allowed.
            _revertSelectorOnActiveRental(selector, from, to, tokenId);
        } else if (selector == e721_transfer_from_selector) {
            // Load the token ID from calldata.
            uint256 tokenId = uint256(
                _loadValueFromCalldata(data, e721_transfer_from_token_id_offset)
            );

            // Check if the selector is allowed.
            _revertSelectorOnActiveRental(selector, from, to, tokenId);
        } else if (selector == e721_approve_selector) {
            // Load the token ID from calldata.
            uint256 tokenId = uint256(
                _loadValueFromCalldata(data, e721_approve_token_id_offset)
            );

            // Check if the selector is allowed.
            _revertSelectorOnActiveRental(selector, from, to, tokenId);
        } else if (selector == e1155_safe_transfer_from_selector) {
            // Load the token ID from calldata.
            uint256 tokenId = uint256(
                _loadValueFromCalldata(data, e1155_safe_transfer_from_token_id_offset)
            );

            // Check if the selector is allowed.
            _revertSelectorOnActiveRental(selector, from, to, tokenId);
        } else if (selector == gnosis_safe_enable_module_selector) {
            // Load the extension address from calldata.
            address extension = address(
                uint160(
                    uint256(
                        _loadValueFromCalldata(data, gnosis_safe_enable_module_offset)
                    )
                )
            );

            // Check if the extension is whitelisted.
            _revertNonWhitelistedExtension(extension);
        } else if (selector == gnosis_safe_disable_module_selector) {
            // Load the extension address from calldata.
            address extension = address(
                uint160(
                    uint256(
                        _loadValueFromCalldata(data, gnosis_safe_disable_module_offset)
                    )
                )
            );

            // Check if the extension is whitelisted.
            _revertNonWhitelistedExtension(extension);
        } else {
            // Revert if the `setApprovalForAll` selector is specified. This selector is
            // shared between ERC721 and ERC1155 tokens.
            if (selector == shared_set_approval_for_all_selector) {
                revert Errors.GuardPolicy_UnauthorizedSelector(
                    shared_set_approval_for_all_selector
                );
            }

            // Revert if the `safeBatchTransferFrom` selector is specified. There's no
            // cheap way to check if individual items in the batch are rented out.
            // Each token ID would require a call to the storage contract to check
            // its rental status.
            if (selector == e1155_safe_batch_transfer_from_selector) {
                revert Errors.GuardPolicy_UnauthorizedSelector(
                    e1155_safe_batch_transfer_from_selector
                );
            }

            // Revert if the `setGuard` selector is specified.
            if (selector == gnosis_safe_set_guard_selector) {
                revert Errors.GuardPolicy_UnauthorizedSelector(
                    gnosis_safe_set_guard_selector
                );
            }
        }
    }

Tools Used

Manual Review.

Recommended Mitigation Steps

Override checkAfterExecution function in the SAFE wallet contract to check if the assets are still owned after a transaction is execution.

Assessed type

Context

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L383-L386 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L405-L406 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L218-L239

Vulnerability details

Impact

• Signer._deriveOrderMetadataHash function is used to derive the order metadata hash, where the derived value is going to be used in:

  1. Deriving the rentalPayloadHash:

         function _deriveRentPayloadHash(
             RentPayload memory payload
         ) internal view returns (bytes32) {
             // Derive and return the rent payload hash as specified by EIP-712.
             return
                 keccak256(
                     abi.encode(
                         _RENT_PAYLOAD_TYPEHASH,
                         _deriveOrderFulfillmentHash(payload.fulfillment),
                         _deriveOrderMetadataHash(payload.metadata),
                         payload.expiration,
                         payload.intendedFulfiller
                     )
                 );
         }

  2. Validating the order metadata by comparing the derived order metadata hash with the zoneHash of the order metadata that was passed in when the Seaport order was signed:

         // Check that the zone hash is equal to the derived hash of the metadata.
             if (_deriveOrderMetadataHash(metadata) != zoneHash) {
                 revert Errors.CreatePolicy_InvalidOrderMetadataHash();
             }

• The _ORDER_METADATA_TYPEHASH immutable variable is assigned when the Create contract is deployed, and it's defined using _deriveRentalTypehashes() function, as follows:

      // Construct the OrderMetadata type string.
              bytes memory orderMetadataTypeString = abi.encodePacked(
                  "OrderMetadata(uint8 orderType,uint256 rentDuration,Hook[] hooks,bytes emittedExtraData)"
              );
  
  //some code...
  
      // Derive the OrderMetadata type hash using the corresponding type string.
              orderMetadataTypeHash = keccak256(orderMetadataTypeString);

• As can be noticed: to create a hash for the order metadata, the orderType, rentDuration, hooks and emittedExtraData must be encoded to create it; but it was noticed that the emittedExtraData and orderType are not encoded when creating the hash:

  keccak256(
    abi.encode(
      _ORDER_METADATA_TYPEHASH,
      metadata.rentDuration,
      keccak256(abi.encodePacked(hookHashes))
    )
  );

• This hash is supposed to follow EIP712 specification, but as shown above; it violates the specs, which will result in integration issues with SeaPort as the created hash is invalid and doesn't comply with EIP712.

Proof of Concept

Signer._deriveRentalTypehashes function/ L383-L386

            // Construct the OrderMetadata type string.
            bytes memory orderMetadataTypeString = abi.encodePacked(
                "OrderMetadata(uint8 orderType,uint256 rentDuration,Hook[] hooks,bytes emittedExtraData)"
            );

Signer._deriveRentalTypehashes function/ L405-L406

// Derive the OrderMetadata type hash using the corresponding type string.
orderMetadataTypeHash = keccak256(orderMetadataTypeString);

Signer._deriveOrderMetadataHash function

    function _deriveOrderMetadataHash(
        OrderMetadata memory metadata
    ) internal view returns (bytes32) {
        // Create array for hooks.
        bytes32[] memory hookHashes = new bytes32[](metadata.hooks.length);

        // Iterate over each hook.
        for (uint256 i = 0; i < metadata.hooks.length; ++i) {
            // Hash the hook
            hookHashes[i] = _deriveHookHash(metadata.hooks[i]);
        }

        // Derive and return the metadata hash as specified by EIP-712.
        return
            keccak256(
                abi.encode(
                    _ORDER_METADATA_TYPEHASH,
                    metadata.rentDuration,
                    keccak256(abi.encodePacked(hookHashes))
                )
            );
    }

Tools Used

Manual Review.

Recommended Mitigation Steps

Update _deriveOrderMetadataHash to be EIP712 compliant by including metadata.orderType and metadata.emittedExtraData in order metadata hash calculation:

    function _deriveOrderMetadataHash(
        OrderMetadata memory metadata
    ) internal view returns (bytes32) {
        // Create array for hooks.
        bytes32[] memory hookHashes = new bytes32[](metadata.hooks.length);

        // Iterate over each hook.
        for (uint256 i = 0; i < metadata.hooks.length; ++i) {
            // Hash the hook
            hookHashes[i] = _deriveHookHash(metadata.hooks[i]);
        }

        // Derive and return the metadata hash as specified by EIP-712.
        return
            keccak256(
                abi.encode(
                    _ORDER_METADATA_TYPEHASH,
+                   metadata.orderType
                    metadata.rentDuration,
                    keccak256(abi.encodePacked(hookHashes)),
+                   metadata.emittedExtraData
                )
            );
    }

Assessed type

Context

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L378-L400 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L248-L262

Vulnerability details

Impact

• Signer._deriveRentalOrderHash function is used to derive the hashes of rental orders , where the derived hash is going to be used:

  • when the rental order is fulfilled, where this hash is going to be stored in the Storage,
  • and to finalize rental order when it is over: by re-creating the order hash and checking if it's stored, then deleting it.

• The _RENTAL_ORDER_TYPEHASH immutable variable is assigned when the Create contract is deployed, and it's defined using _deriveRentalTypehashes() function, as follows:

          // Construct the OrderFulfillment type string.
              bytes memory orderFulfillmentTypeString = abi.encodePacked(
                  "OrderFulfillment(address recipient)"
              );
  
              // Construct the OrderMetadata type string.
              bytes memory orderMetadataTypeString = abi.encodePacked(
                  "OrderMetadata(uint8 orderType,uint256 rentDuration,Hook[] hooks,bytes emittedExtraData)"
              );
  
              // Construct the RentPayload type string.
              bytes memory rentPayloadTypeString = abi.encodePacked(
                  "RentPayload(OrderFulfillment fulfillment,OrderMetadata metadata,uint256 expiration,address intendedFulfiller)"
              );
  
              // Derive RentPayload type hash via combination of relevant type strings.
              rentPayloadTypeHash = keccak256(
                  abi.encodePacked(
                      rentPayloadTypeString,
                      orderMetadataTypeString,
                      orderFulfillmentTypeString
                  )
              );

• As can be noticed, to create a hash for a rental order, three structs data must be encoded:

  • RentPayload(OrderFulfillment fulfillment,OrderMetadata metadata,uint256 expiration,address intendedFulfiller)
  • OrderMetadata(uint8 orderType,uint256 rentDuration,Hook[] hooks,bytes emittedExtraData)
  • OrderFulfillment(address recipient)

• And _deriveRentPayloadHash is used to create the hash as follows:

      function _deriveRentPayloadHash(
          RentPayload memory payload
      ) internal view returns (bytes32) {
          // Derive and return the rent payload hash as specified by EIP-712.
          return
              keccak256(
                  abi.encode(
                      _RENT_PAYLOAD_TYPEHASH,
                      _deriveOrderFulfillmentHash(payload.fulfillment),
                      _deriveOrderMetadataHash(payload.metadata),
                      payload.expiration,
                      payload.intendedFulfiller
                  )
              );
      }

  Where _deriveOrderFulfillmentHash function returns :

  return keccak256(
    abi.encode(_ORDER_FULFILLMENT_TYPEHASH, fulfillment.recipient)
  );

  and _deriveOrderMetadataHash function returns the following; (where the encoded data doesn't comply with the OrderMetadata struct as it's missing encoding the emittedExtraData and orderType , this is reported in a separate issue):

    ```javascript
        return
                keccak256(
                    abi.encode(
                        _ORDER_METADATA_TYPEHASH,
                        metadata.rentDuration,
                        keccak256(abi.encodePacked(hookHashes))
                    )
                );
    ```
  

  and the payload.expiration is encoded while it's not supposed to be included.

• So as can be noticed: the arrangement of the encoded structs data and the number of the encoded data are wrong and don't comply with the _RENTAL_ORDER_TYPEHASH definition.

• While the created order hash is supposed to follow EIP712 specification, but as shown above; it violates the specs, which might result in hashes collision as the created hash is invalid and doesn't comply with EIP712.

Proof of Concept

Signer._deriveRentalTypehashes function/L378-L400

     // Construct the OrderFulfillment type string.
            bytes memory orderFulfillmentTypeString = abi.encodePacked(
                "OrderFulfillment(address recipient)"
            );

            // Construct the OrderMetadata type string.
            bytes memory orderMetadataTypeString = abi.encodePacked(
                "OrderMetadata(uint8 orderType,uint256 rentDuration,Hook[] hooks,bytes emittedExtraData)"
            );

            // Construct the RentPayload type string.
            bytes memory rentPayloadTypeString = abi.encodePacked(
                "RentPayload(OrderFulfillment fulfillment,OrderMetadata metadata,uint256 expiration,address intendedFulfiller)"
            );

            // Derive RentPayload type hash via combination of relevant type strings.
            rentPayloadTypeHash = keccak256(
                abi.encodePacked(
                    rentPayloadTypeString,
                    orderMetadataTypeString,
                    orderFulfillmentTypeString
                )
            );

Signer._deriveRentPayloadHash function

    function _deriveRentPayloadHash(
        RentPayload memory payload
    ) internal view returns (bytes32) {
        // Derive and return the rent payload hash as specified by EIP-712.
        return
            keccak256(
                abi.encode(
                    _RENT_PAYLOAD_TYPEHASH,
                    _deriveOrderFulfillmentHash(payload.fulfillment),
                    _deriveOrderMetadataHash(payload.metadata),
                    payload.expiration,
                    payload.intendedFulfiller
                )
            );
    }

Tools Used

Manual Review.

Recommended Mitigation Steps

Update _deriveRentPayloadHash to be EIP712 compliant by including the missing data and in the right order.

Assessed type

Context

Judge has assessed an item in Issue #361 as 2 risk. The relevant finding follows:

L2,L3

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Reclaimer.sol#L93-L99 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Reclaimer.sol#L32-L34 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Reclaimer.sol#L42-L50

Vulnerability details

Impact

• The assets are returned back to the lender when the rental is over, and the inherited Reclaimer contract uses safeTransferFrom method to transfer back the assets to the lender.

• But if the lender (the address who created the rental order) is a smart contract (wallet) that doesn't implement the onERC721Received or onERC1155Received (depends on whether the rented asset/assets is/are ERC721,ERC1155 or both); it will not be able to receive these assets as the transaction will revert resulting in permanently locking the assets in the renter SAFE wallet.

• This is because the safeTransferFrom checks if the receiver is a contract or an EOA, if it's a contract then it must have onERC721Received/onERC1155Received function implemented as per specs; otherwise the transaction will revert (ERC721._checkOnERC721Received function):

  function _checkOnERC721Received(address from, address to, uint256 tokenId, bytes memory data) private {
          if (to.code.length > 0) {
              try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, data) returns (bytes4 retval) {
                  if (retval != IERC721Receiver.onERC721Received.selector) {
                      revert ERC721InvalidReceiver(to);
                  }
              } catch (bytes memory reason) {
                  if (reason.length == 0) {
                      revert ERC721InvalidReceiver(to);
                  } else {
                      /// @solidity memory-safe-assembly
                      assembly {
                          revert(add(32, reason), mload(reason))
                      }
                  }
              }
          }
      }

Proof of Concept

Reclaimer.reclaimRentalOrder function / L93-L99

// Check if the item is an ERC721.
if (item.itemType == ItemType.ERC721) _transferERC721(item, rentalOrder.lender);

// check if the item is an ERC1155.
if (item.itemType == ItemType.ERC1155)
  _transferERC1155(item, rentalOrder.lender);

Reclaimer._transferERC721 function

    function _transferERC721(Item memory item, address recipient) private {
        IERC721(item.token).safeTransferFrom(address(this), recipient, item.identifier);
    }

Reclaimer._transferERC1155 function

   function _transferERC1155(Item memory item, address recipient) private {
        IERC1155(item.token).safeTransferFrom(
            address(this),
            recipient,
            item.identifier,
            item.amount,
            ""
        );
    }

Tools Used

Manual Review.

Recommended Mitigation Steps

Implement another mechanism that enables lenders from pulling their assets to a specific address they wish once the rental is over.

Assessed type

DoS

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L100-L118

Vulnerability details

Impact

• The protocol will allow the use of the same ERC20 tokens that are used by the OpenSea protocol for rental payments, and one of these tokens is USDC.

• USDC has a blacklist, where certain addresses are prohibited from receiving/transferring any amount of this token, so if any of the parties (the lender in the case of BASE order, or the renter in the case of PAY order) is blocklisted by the USDC token; then the PaymentEscrow.settlePayment will revert when it tries to transfer the fees for the entitled user.

• This will result in permanently locking the rented assets in the renter SAFE wallet as there's no way to extract these assets back.

Proof of Concept

PaymentEscrow._safeTransfer function

    function _safeTransfer(address token, address to, uint256 value) internal {
        // Call transfer() on the token.
        (bool success, bytes memory data) = token.call(
            abi.encodeWithSelector(IERC20.transfer.selector, to, value)
        );

        // Because both reverting and returning false are allowed by the ERC20 standard
        // to indicate a failed transfer, we must handle both cases.
        //
        // If success is false, the ERC20 contract reverted.
        //
        // If success is true, we must check if return data was provided. If no return
        // data is provided, then no revert occurred. But, if return data is provided,
        // then it must be decoded into a bool which will indicate the success of the
        // transfer.
        if (!success || (data.length != 0 && !abi.decode(data, (bool)))) {
            revert Errors.PaymentEscrowModule_PaymentTransferFailed(token, to, value);
        }
    }

Tools Used

Manual Review.

Recommended Mitigation Steps

Implement a pulling mechanism instead of pushing mechanism when settling rental payments; this is done by recording the claimablefees for both the renter and the lender and let them claim these fees by themselves by adding another function in the PaymentEscrow to enable them from withdrawing their entitled fees.

Assessed type

DoS

Findings Summary

Low

[L-01] Lack of minimum signers & minimum threshold check <a id="l-01" ></a>

Details

• In the current Factory.deployRentalSafe function design: for risk mitigation, it is suggested to set a minimum of 2 for the threshold and a minimum of 3 signers/owners.

• By doing this; two different signers votes will be needed before a transaction is executed; so in case of a one signer account is compromised; the other two signers will be able to mitigate his actions/change signers, also; if a signer is down, the other two signers will be able to execute transactions out of the SAFE wallet.

Proof of Concept

Factory.deployRentalSafe function

function deployRentalSafe(

Tools Used

Manual Testing.

Recommendation

Ensure that the minimum owners number is >= 3, and minimum threshold is >= 2.

[L-02] Create.validateOrder function : zoneParams can be replayed <a id="l-02" ></a>

Details

validateOrder function is invoked by the SeaPort with the zoneParams for the order to be fulfilled, where this argument holds the rental payload and the renter signature, but there's a possibility of replaying the same order again even if it has an expiry for the signature.

Proof of Concept

Create.validateOrder function

  function validateOrder(
        ZoneParameters calldata zoneParams
    ) external override onlyRole("SEAPORT") returns (bytes4 validOrderMagicValue) {

Tools Used

Manual Testing.

Recommendation

Save the executed zoneParams in a mapping, and check against it whenever validateOrder is invoked.

[L-03] Create.validateOrder function : renter signature can be replayed <a id="l-03" ></a>

Details

When validateOrder function is invoked by the SeaPort, the order fulfiller signature is recovered and validated,but there's a possibility of replaying the same siganture again even if it has an expiry for the signature.

Proof of Concept

Create.validateOrder function

   // Recover the signer from the payload.
        address signer = _recoverSignerFromPayload(
            _deriveRentPayloadHash(payload),
            signature
        );

Signer._recoverSignerFromPayload function

   function _recoverSignerFromPayload(
        bytes32 payloadHash,
        bytes memory signature
    ) internal view returns (address) {
        // Derive original EIP-712 digest using domain separator and order hash.
        bytes32 digest = _DOMAIN_SEPARATOR.toTypedDataHash(payloadHash);

        // Recover the signer address of the signature.
        return digest.recover(signature);
    }

Tools Used

Manual Testing.

Recommendation

Save the recovered signature in a mapping, and check against it whenever validateOrder is invoked.

[L-04] PaymentEscrow can be bricked by some payment tokens<a id="l-04" ></a>

Impact

• Some tokens as cUSDCv3 (Compound USDC) has a special case when the user determines the amount to be transferred to be equals to type(uint256).max while his balance is less than this amount; in this case his balance will be transferred instead of reverting the transaction:

  if (amount == type(uint256).max) {
    amount = balanceOf(src);
  }

• How could this brick the PaymentEscrow with cUSDCv3 token as the payment token and resulting in locking the rented assets?

  1. Assume that there's a BASE rental order with cUSDCv3 token as it's payment token.
  2. A malicious renter has a dust amount of this token (1 wei for example), then he fulfills this rental order by paying the protocol type(uint256).max, so the escrow will record this max amount as being deposited in it.
  3. Now the rental time is over, but if anyone tries to stop the rental by calling Stop.stopRentals; the transaction will revert as the balance of the escrow is way less than it has actually received (the escrow contract received the dust amount instead of receiving the type(uint256).max, and the recorded balanceOf[token] is set to type(uint256).max).

• This will result in permanently locking the rented assets in the renter SAFE wallet.

Proof of Concept

Create._rentFromZone function/L599-L603

    for (uint256 i = 0; i < items.length; ++i) {
                if (items[i].isERC20()) {
                    ESCRW.increaseDeposit(items[i].token, items[i].amount);
                }
            }

PaymentEscrow._increaseDeposit function

    function _increaseDeposit(address token, uint256 amount) internal {
        // Directly increase the synced balance.
        balanceOf[token] += amount;
    }

Tools Used

Manual Review.

Recommended Mitigation Steps

Prevent using such tokens in the protocol.

Audit Scope

reNFT protocol is built on the idea of facilitating ERC721/ERC1155 tokens rental, where it's built on top of SeaPort protocol and Gnosis SAFE wallets where the rented items are going to be reseted in.

Th audit scope consists of 12 contracts with ~ 1905 SLoC.

Methodlogy

Methodology adopted during the audit:

1. Gaining a high-level overflow of the protocol and what it intends to do.
2. Understanding SeaPort orders addition/fulfilling orders process and how the protocol integrates with it.
3. Understanding the architecture adopted by the protocol and how the contracts are connected with each other, then started with the entry point of the protocol which is validateOrder.
4. Main focus was to find flows in the following areas; where the invariants might be broken:
   • Integration with SeaPort.
   • Handelling NFTs ownership.
   • Balances and payment calculations are always ensured to follow the intended buisness logic.
   • Signatures replays.
   • Any possibility of hash collisions.
   • Invalid checks that may cause revert on vital actions (such as revert on hooks execution when a rental is stopped).

Invariant Violations

• The main invariant that the protocol is more concerned about is preserving the rented asset in the renter SAFE wallet and safely returning the assets back to the lender after the rental is over, but it was noticed that this invariant (where the rented tokens are at risk of being stuck in the renter SAFE wallet) can be broken in the following cases:

  1. If -upon stopRental function invocation- any of the on-stop hooks reverted or became unsupported.

  2. Knowing the the ERC20 payment tokens used in the protocol are the same ERC20 tokens used in SeaPort, and one of these tokens is USDC where it has a blocklist, then if the order type is a BASE order and the lender that's going to receive the USDC payment is blocklisted, or if the order is PAY and any of the lender or renter is blocklisted (or became after the fullfillment of the order) by the USDC, then the NFT will be stuck

  3. Knowing that USDC is an upgradeable contract that might charge fees in the future, which will result in partially stuck NFTs in the wallets (partially as anyone can send payments directly to the PaymentEscrow contract to compensate for the deducted fees and rescue the stuck rented assets).

  4. And one important thing is if the recipient is a multisig wallet (contract) that doesn't implement onERC721Received or onERC1155Received; then they can't receive their rented tokens via safeTransfer and the assets will be stuck permanently in the renter SAFE wallet (this is a different issue from the one reported by the winning bot: where the SAFE wallet is unable to receive NFTs or ERC1155 tokens as the don't inherint the OZ ERC721Holder ERC1155Holder contracts).

Other Systematic Risks

• The protocol uses EIP712 specifications to create hashes for orders and orders metadata, but it was noticed that these hashes are not compliant with the specifications.

• In cases of hardforks, the protocol will be disabled (temporarily until upgrading the Create contract) as it uses an immutable to save the EIP712 _DOMAIN_SEPARATOR that encodes the chain.id at the time of the deployment; so after a hardfork, signatures sent to verify SeaPort calls to the validateOrder will not be validated and the fulfilling orders functionality will not be working anymore.

Centralization Risks

• On the level of the on-chain contracts: almost centralization issues are limited, as the executor role is the main role in the protocol that can do upgrades, migrations, enabling and disabling policies, but there's some noted issues:
  1. If any of the on-stop hooks is disabled, then the rental order that uses that hook can never be stopped, and this will result in locking the rented assets in the renter wallet.
  2. If the kernel executor upgrades the contracts without checking if all orders are stopped; this will result in stuck rented assets.

User Flow

Starting and Fulfilling a Rental Order If Alice has an NFT that she would like to lend -> she will have to interact with the reNFT frontend to create a rentalOrder -> then the order is checked in the server-side and validated -> then this order is listed on SeaPort -> Now Bob wants to rent this NFT -> he will interact with reNFT with a signed order -> the reNFT server-side will interact with SeaPort to fulfill the order -> Then the rented assets will be moved to a SAFE wallet created and whitlisted for the renter

Stoppinga a Rental Order If the time of the rental is over or if Alice decided to stop the rental (if it's a PAY order) -> stopRental is called where the rented assets are returned back to Alice -> and the payment is paid for the intended parties (for Bob if it's a BASE order, and for Bob and Alice if it's a PAY order that hasn't ended yet).

note : to ease the following the flow, Bob acts as he interacts with SeaPort directly, while he is actually interacting with reNFT front-end, and the reNFT server-side will interact with SeaPort to fulfill the order.

Architecture Review

• The protocol adopts the default framework architectural model, where it separates contracts into internal modules (back-end) and external policies (user facing contracts/front-end) for flexible, modular protocol design, where modules manage specific data models independently, while policies handle inbound calls and route updates to Modules, acting as intermediaries.

• This separation of data management (what) from business logic (why) enhances flexibility and immutability, simplifying protocol development and any future features addition.

• All the contracts are managed by the Kernel contract, which is a registry that manages all the changes and upgrades of the protocol: adding/removing policies and modules.

Enahancement

In general, the codebase is robust and follows the security best practices and implementations, but it would be a safe play if a pulling mechanism is adopted instead of pushing mechanism when the rented assets are returned and when the payments are paid for the intended parties, as this would prevent any risks related to denial-of-service due to external un-expected behaviours.

Also following the EIP712 specifications when creating hashes would prevent any integration issues with SeaPort protocol.

Time Spent

Approximately 25 hours ; divided between reading the documentation, manually reviewing the codebase, and documenting my findings.

Time spent:

25 hours