reNFT - serial-coder's results

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalConstants.sol#L58 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalConstants.sol#L62 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/interfaces/ISafe.sol#L98-L101 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L255 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L260 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L266 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/test/unit/Guard/CheckTransaction.t.sol#L369 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/test/unit/Guard/CheckTransaction.t.sol#L557

Vulnerability details

The gnosis_safe_disable_module_offset constant was incorrectly specified to point at an incorrect function parameter of the disableModule(address prevModule, address module).

Specifically, the offset constant will point at the prevModule (1st param) instead of the module (2nd param).

Impact

When a safe transaction initiated from a rental safe containing a call to the safe's disableModule() is invoked, the Guard::checkTransaction() cannot verify the module expected to be removed.

If the prevModule was a non-whitelisted extension, the safe transaction will be reverted.

However, if the prevModule was a whitelisted extension, the module will be removed without verification. Removing the rental safe's module without verification can lead to other issues or attacks since the removed module can be a critical component (e.g., removing the protocol's Stop policy contract).

Proof of Concept

The snippet below presents some of the Gnosis Safe configurations of the reNFT protocol. The gnosis_safe_disable_module_selector constant contains the function selector (0xe009cfde) of the rental safe's ModuleManager::disableModule(address prevModule, address module).

Meanwhile, the gnosis_safe_disable_module_offset constant contains a memory offset (0x24) intended to point at the module param of the disableModule().

    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalConstants.sol

    /////////////////////////////////////////////////////////////////////////////////
    //                  Gnosis Safe Function Selectors And Offsets                 //
    /////////////////////////////////////////////////////////////////////////////////

    ...

    // bytes4(keccak256("disableModule(address,address)"));
@1  bytes4 constant gnosis_safe_disable_module_selector = 0xe009cfde; //@audit -- The declaration of function selector: ModuleManager::disableModule(address prevModule, address module)

    ...

@2  uint256 constant gnosis_safe_disable_module_offset = 0x24; //@audit -- The memory offset intended to point at the 'module' param of the disableModule() was incorrectly specified to the 'prevModule' param instead

• @1 -- The declaration of function selector: ModuleManager::disableModule(address prevModule, address module): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalConstants.sol#L58
• @2 -- The memory offset intended to point at the 'module' param of the disableModule() was incorrectly specified to the 'prevModule' param instead: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalConstants.sol#L62

The below snippet shows the function signature of the rental safe's ModuleManager::disableModule(): function disableModule(address prevModule, address module) external

Let's break down the value of the gnosis_safe_disable_module_offset constant (0x24): 0x24 == 36 == 32 (the calldata's array length) + 4 (the function selector)

As you can see, the gnosis_safe_disable_module_offset was incorrectly specified to point at the prevModule param (1st param) instead of the module param (2nd param) that refers to the module expected to be removed.

    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/interfaces/ISafe.sol
    /**
     * @notice Disables the module `module` for the Safe.
     *
     * @dev This can only be done via a Safe transaction.
     *
@3   * @param prevModule Previous module in the modules linked list.
@3   * @param module     Module to be removed.
     */
@3  function disableModule(address prevModule, address module) external; //@audit -- The memory offset must point at the second param because it would be the module to be removed

• @3 -- The memory offset must point at the second param because it would be the module to be removed: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/interfaces/ISafe.sol#L98-L101

With the incorrect gnosis_safe_disable_module_offset constant, once the Guard::_checkTransaction() is triggered to verify the safe transaction containing a call to the safe's disableModule(), the address of the prevModule contract will be extracted and assigned to the extension variable instead of the module contract's.

Consequently, the address of the prevModule contract will be verified for the whitelist by the Guard::_revertNonWhitelistedExtension() instead of the expected module contract address.

    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol
    function _checkTransaction(address from, address to, bytes memory data) private view {
        bytes4 selector;

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

        ... // some if-else cases
        
@4      } else if (selector == gnosis_safe_disable_module_selector) { //@audit -- Check for calls to the disableModule() initiated from Safe contracts
            // Load the extension address from calldata.
            address extension = address(
                uint160(
                    uint256(
@5                      _loadValueFromCalldata(data, gnosis_safe_disable_module_offset) //@audit -- Since the gnosis_safe_disable_module_offset constant points at the incorrect param (i.e., prevModule), the extension variable will contain an address of the prevModule
                    )
                )
            );

            // Check if the extension is whitelisted.
@6          _revertNonWhitelistedExtension(extension); //@audit -- The address of the prevModule will be checked for the whitelist instead of the expected module to be removed
        } 

        ... // else case
    }

• @4 -- Check for calls to the disableModule() initiated from Safe contracts: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L255
• @5 -- Since the gnosis_safe_disable_module_offset constant points at the incorrect param (i.e., prevModule), the extension variable will contain an address of the prevModule: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L260
• @6 -- The address of the prevModule will be checked for the whitelist instead of the expected module to be removed: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L266

Besides, I also noticed that the developer also assumed that the disableModule() would have one function parameter while writing the test functions: test_Success_CheckTransaction_Gnosis_DisableModule() and test_Reverts_CheckTransaction_Gnosis_DisableModule().

That can confirm why the test functions cannot catch up with the mistake.

    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/test/unit/Guard/CheckTransaction.t.sol
    function test_Success_CheckTransaction_Gnosis_DisableModule() public {
        // impersonate the admin policy admin
        vm.prank(deployer.addr);

        // enable this address to be added as a module by rental safes
        admin.toggleWhitelistExtension(address(mockTarget), true);

        // Build up the `disableModule(address)` calldata
        bytes memory disableModuleCalldata = abi.encodeWithSelector(
            gnosis_safe_disable_module_selector,
@7          address(mockTarget) //@audit -- In the test function: test_Success_CheckTransaction_Gnosis_DisableModule(), the developer assumed that the disableModule() would have one param (incorrect!!)
        );

        // Check the transaction
        _checkTransaction(address(this), address(mockTarget), disableModuleCalldata);
    }

    ...

    function test_Reverts_CheckTransaction_Gnosis_DisableModule() public {
        // Build up the `disableModule(address)` calldata
        bytes memory disableModuleCalldata = abi.encodeWithSelector(
            gnosis_safe_disable_module_selector,
@8          address(mockTarget) //@audit -- Also in the test function: test_Reverts_CheckTransaction_Gnosis_DisableModule(), the developer assumed that the disableModule() would have one param (incorrect!!)
        );

        // Expect revert because of an unauthorized extension
        _checkTransactionRevertUnauthorizedExtension(
            address(this),
            address(mockTarget),
            disableModuleCalldata
        );
    }

• @7 -- In the test function: test_Success_CheckTransaction_Gnosis_DisableModule(), the developer assumed that the disableModule() would have one param (incorrect!!): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/test/unit/Guard/CheckTransaction.t.sol#L369
• @8 -- Also in the test function: test_Reverts_CheckTransaction_Gnosis_DisableModule(), the developer assumed that the disableModule() would have one param (incorrect!!): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/test/unit/Guard/CheckTransaction.t.sol#L557

Tools Used

Manual Review

Recommended Mitigation Steps

To point the memory offset at the module param (2nd param), the gnosis_safe_disable_module_offset constant must be set to 0x44 (0x44 == 68 == 32 (the calldata's array length) + 4 (the function selector) + 32 (1st param)).

    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalConstants.sol

    /////////////////////////////////////////////////////////////////////////////////
    //                  Gnosis Safe Function Selectors And Offsets                 //
    /////////////////////////////////////////////////////////////////////////////////

    ...

    // bytes4(keccak256("disableModule(address,address)"));
    bytes4 constant gnosis_safe_disable_module_selector = 0xe009cfde;

    ...

-   uint256 constant gnosis_safe_disable_module_offset = 0x24;
+   uint256 constant gnosis_safe_disable_module_offset = 0x44;

Assessed type

Invalid Validation

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalStructs.sol#L140 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L185-L192 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L281 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L282 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L289 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L293 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L300 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L301 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol#L225 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol#L233 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L207 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L228 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L231 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L232

Vulnerability details

The Signer::_deriveRentalOrderHash() computes the rentalOrderHash without including the rentalWallet param, allowing an attacker to manipulate the rentalWallet param to their target victim's rental safe address.

Subsequently, the attacker can brick the victim's rental order holding at least one ERC-1155 rented item from stopping the order. In fact, the rental order can hold multiple ERC-1155, ERC-721, and ERC-20 items. All holding items will be frozen permanently.

Moreover, the attacker can even trick arbitrary (enabled) Hook contracts into processing incorrect rented items (owned by the victim) and consuming malicious extraData param.

Proof of Concept

This section explains only the Stop::stopRent() for brevity. The attack using the Stop::stopRentBatch() should have similar details.

The snippet below shows the RentalOrder struct definition. As you can see, the struct contains the rentalWallet param inside.

    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalStructs.sol
    struct RentalOrder {
        bytes32 seaportOrderHash;
        Item[] items;
        Hook[] hooks;
        OrderType orderType;
        address lender;
        address renter;
@1      address rentalWallet; //@audit -- The rentalWallet param is included in the RentalOrder struct
        uint256 startTimestamp;
        uint256 endTimestamp;
    }

• @1 -- The rentalWallet param is included in the RentalOrder struct: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalStructs.sol#L140

Nevertheless, while computing the rentalOrderHash, the Signer::_deriveRentalOrderHash() does not take the order.rentalWallet param into account. This allows an attacker to manipulate the order.rentalWallet param.

    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol
    function _deriveRentalOrderHash(
        RentalOrder memory order
    ) internal view returns (bytes32) {
        ...

        return
            keccak256(
                abi.encode(
                    _RENTAL_ORDER_TYPEHASH,
@2                  order.seaportOrderHash, //@audit -- Nevertheless, the _deriveRentalOrderHash() does not include the order.rentalWallet param while computing the rentalOrderHash
@2                  keccak256(abi.encodePacked(itemHashes)),
@2                  keccak256(abi.encodePacked(hookHashes)),
@2                  order.orderType,
@2                  order.lender,
@2                  order.renter,
@2                  order.startTimestamp,
@2                  order.endTimestamp
                )
            );
    }

• @2 -- Nevertheless, the _deriveRentalOrderHash() does not include the order.rentalWallet param while computing the rentalOrderHash: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L185-L192

Below presents the Stop::stopRent().

The vulnerability allows an attacker to create an attack rental order holding the same ERC-1155 asset token with a tokenId identical to the one contained in the victim's rental order (note that one ERC-1155's tokenId can have multiple items (amount > 1)).

Then, the attacker executes the Stop::stopRent() (or Stop::stopRentBatch()) to stop their rental order. In this step, however, the order's rentalWallet param will point at the victim's rental safe address instead of the attacker's. The manipulated order.rentalWallet param will be processed by the RentalUtils::toRentalId() incorrectly, resulting in returning the incorrect rentalId of the ERC-1155 item owned by the victim.

Besides the rentalWallet param, the attacker can even manipulate the order.items[i].amount param (ERC-1155 items) to completely brick the victim's rental order from stopping the order (see Exploit Scenario #1 section below). Moreover, the attacker can also trick arbitrary (enabled) Hook contracts into processing the incorrect rented items (owned by the victim) and consuming malicious extraData param (see Exploit Scenario #2 section below).

By exploiting this vulnerability, the attacker is guaranteed to retrieve their ERC-1155 items used as initial attack assets because they will eventually receive the ERC-1155 rented items held by the victim's rental safe instead when the stopRent() executes the _reclaimRentedItems().

Here, we come to the root cause. The protocol verifies the rental order to be stopped using the rentalOrderHash without including the order.rentalWallet param in the computation (computed by the _deriveRentalOrderHash()). This allows the attacker to manipulate the order.rentalWallet param as discussed previously.

Next, when the stopRent() executes the Storage::removeRentals(), the rentalAssetUpdates generated in steps @3 - @4 (see the below snippet) will contain a reference for removing the ERC-1155 items owned by the victim instead of the attacker's (see Exploit Scenario #1 section for more details).

    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol
    function stopRent(RentalOrder calldata order) external {
        ...

        // Check if each item in the order is a rental. If so, then generate the rental asset update.
        // Memory will become safe again after this block.
        for (uint256 i; i < order.items.length; ++i) {
            if (order.items[i].isRental()) {
                // Insert the rental asset update into the dynamic array.
                _insert(
                    rentalAssetUpdates,
@3                  order.items[i].toRentalId(order.rentalWallet), //@audit -- Attacker can manipulate the order.rentalWallet param to point to another ERC-1155 item's rentalId owned by another renter (victim)
@4                  order.items[i].amount //@audit -- Attacker can even manipulate the order.items[i].amount param (ERC-1155 item) for further exploitation (see Exploit Scenario #1)
                );
            }
        }

        // Interaction: process hooks so they no longer exist for the renter.
        if (order.hooks.length > 0) {
@5          _removeHooks(order.hooks, order.items, order.rentalWallet); //@audit -- Attacker can trick the Hook contracts' onStop() to process the rental stop event on incorrect rented items owned by the victim (see Exploit Scenario #2)
        }

        // Interaction: Transfer rentals from the renter back to lender.
@6      _reclaimRentedItems(order); //@audit -- Attacker will receive the ERC-1155 rented items owned by the victim (So, the attacker is guaranteed to retrieve their ERC-1155 items used as initial attack assets)

        ...

        // Interaction: Remove rentals from storage by computing the order hash.
        STORE.removeRentals(
@7          _deriveRentalOrderHash(order), //@audit -- Root cause: the _deriveRentalOrderHash() generates the rentalOrderHash without including the order.rentalWallet param
@8          _convertToStatic(rentalAssetUpdates) //@audit -- The rentalAssetUpdates generated in steps 3 - 4 for removing the ERC-1155 items owned by the victim is passed to the Storage::removeRentals()
        );

        ...
    }

• @3 -- Attacker can manipulate the order.rentalWallet param to point to another ERC-1155 item's rentalId owned by another renter (victim): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L281
• @4 -- Attacker can even manipulate the order.items[i].amount param (ERC-1155 item) for further exploitation (see Exploit Scenario #1): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L282
• @5 -- Attacker can trick the Hook contracts' onStop() to process the rental stop event on incorrect rented items owned by the victim (see Exploit Scenario #2): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L289
• @6 -- Attacker will receive the ERC-1155 rented items owned by the victim (So, the attacker is guaranteed to retrieve their ERC-1155 items used as initial attack assets): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L293
• @7 -- Root cause: the _deriveRentalOrderHash() generates the rentalOrderHash without including the order.rentalWallet param: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L300
• @8 -- The rentalAssetUpdates generated in steps 3 - 4 for removing the ERC-1155 items owned by the victim is passed to the Storage::removeRentals(): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L301

The following sub-sections further explain some exploit scenarios in more detail.

Exploit Scenario #1: Bricking the target victim's rental order

From step @8 described above, once the Storage::removeRentals() is invoked, the attacker's rental order (i.e., orderHash) will be removed from the protocol's internal accounting, which is a correct behavior.

In the next step, however, the victim's ERC-1155 rented items (i.e., asset.rentalId) will be maliciously removed from the protocol's internal accounting instead (incorrect behavior). Consequently, certain ERC-1155 rented items will be removed from the victim's rental order. The victim will be unable to re-construct the rental payload (from the event emitted while creating the rental order) to match the amount of the ERC-1155 rented items maliciously removed by the attacker.

With this exploit scenario, the attacker can permanently brick the victim's rental order holding at least one ERC-1155 rented item from stopping the order.

In fact, the rental order can hold multiple ERC-1155 (rented), ERC-721 (rented), and ERC-20 (payment) items at the same time. Subsequently, all items locked in the victim's rental order will be frozen permanently.

    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol
    function removeRentals(
        bytes32 orderHash,
        RentalAssetUpdate[] calldata rentalAssetUpdates
    ) external onlyByProxy permissioned {
        // The order must exist to be deleted.
        if (!orders[orderHash]) {
            revert Errors.StorageModule_OrderDoesNotExist(orderHash);
        } else {
            // Delete the order from storage.
@9          delete orders[orderHash]; //@audit -- The attacker's rental order (orderHash) will be removed from the protocol's internal accounting
        }

        // Process each rental asset.
        for (uint256 i = 0; i < rentalAssetUpdates.length; ++i) {
            RentalAssetUpdate memory asset = rentalAssetUpdates[i];

            // Reduce the amount of tokens for the particular rental ID.
@10         rentedAssets[asset.rentalId] -= asset.amount; //@audit -- However, the victim's ERC-1155 rented items (asset.rentalId) will be maliciously removed from the protocol's internal accounting instead
        }
    }

• @9 -- The attacker's rental order (orderHash) will be removed from the protocol's internal accounting: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol#L225
• @10 -- However, the victim's ERC-1155 rented items (asset.rentalId) will be maliciously removed from the protocol's internal accounting instead: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol#L233

Exploit Scenario #2: Tricking arbitrary Hook contracts to process incorrect rented items and consume malicious extraData param

The attacker can even manipulate arbitrary (enabled) Hook contracts of their choice to process the victim's rented items since the hooks array will refer to the attacker's rental order, not the victim's.

Whereas the rentalWallet param will refer to the victim's rental safe address, the Hook contracts' onStop() will be tricked into processing the victim's rented items maliciously. Besides, the item.amount param can also be manipulated, as long as 0 < item.amount <= the victim's target ERC-1155 item amount.

Finally, the attacker can trick the Hook contracts into consuming arbitrary hooks[i].extraData params because the extraData params will refer to the attacker's rental order.

The impact of this exploit scenario depends on the types of Hook contracts being triggered.

    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol
    function _removeHooks(
        Hook[] calldata hooks,
        Item[] calldata rentalItems,
        address rentalWallet
    ) internal {
        ...

        // Loop through each hook in the payload.
        for (uint256 i = 0; i < hooks.length; ++i) {
            // Get the hook address.
@11         target = hooks[i].target; //@audit -- Attacker can even manipulate the Hook contracts of their choice to process the victim's rented items (since the hooks array refers to the attacker's rental order, not the victim's order)

            ...

            // Call the hook with data about the rented item.
            try
                IHook(target).onStop(
@12                 rentalWallet, //@audit -- This rentalWallet param refers to the victim's rental safe
                    item.token,
                    item.identifier,
@13                 item.amount, //@audit -- The item.amount param can also be manipulated (as long as 0 < item.amount <= the victim's target ERC-1155 item amount)
@14                 hooks[i].extraData //@audit -- Attacker can also trick the Hook contracts to consume arbitrary hooks[i].extraData params
                )
            {} ...
            ...
        }
    }

• @11 -- Attacker can even manipulate the Hook contracts of their choice to process the victim's rented items (since the hooks array refers to the attacker's rental order, not the victim's order): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L207
• @12 -- This rentalWallet param refers to the victim's rental safe: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L228
• @13 -- The item.amount param can also be manipulated (as long as 0 < item.amount <= the victim's target ERC-1155 item amount): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L231
• @14 -- Attacker can also trick the Hook contracts to consume arbitrary hooks[i].extraData params: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L232

Impact

As explained in the Exploit Scenario #1 section:

The attacker can permanently brick the victim's rental order holding at least one ERC-1155 rented item from stopping the order.

In fact, the rental order can hold multiple ERC-1155 (rented), ERC-721 (rented), and ERC-20 (payment) items at the same time. Subsequently, all items locked in the victim's rental order will be frozen permanently.

As explained in the Exploit Scenario #2 section:

The attacker can trick arbitrary (enabled) Hook contracts into processing incorrect rented items (owned by the victim) and consuming malicious extraData param.

The impact of this exploit scenario depends on the types of Hook contracts being triggered.

Tools Used

Manual Review

Recommended Mitigation Steps

Take the order.rentalWallet param into account when computing the rentalOrderHash, like the snippet below.

    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol
    function _deriveRentalOrderHash(
        RentalOrder memory order
    ) internal view returns (bytes32) {
        ...

        return
            keccak256(
                abi.encode(
                    _RENTAL_ORDER_TYPEHASH,
                    order.seaportOrderHash,
                    keccak256(abi.encodePacked(itemHashes)),
                    keccak256(abi.encodePacked(hookHashes)),
                    order.orderType,
                    order.lender,
                    order.renter,
+                   order.rentalWallet,
                    order.startTimestamp,
                    order.endTimestamp
                )
            );
    }

Assessed type

Invalid Validation

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L205 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L227-L233 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L210-L212 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L300 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol#L222 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L257-L264 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L271-L277 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L231 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L174-L178

Vulnerability details

Once a user executes the Stop::stopRent() (or Stop::stopRentBatch()) to stop a rental order. The function must process a handful of gas-exceeding significant-risk tasks.

In the worst-case scenario, the rental stop transaction will be reverted if the function consumes gas beyond the block gas limit. Consequently, all ERC-721/ERC-1155 rented items and ERC-20 payments will be frozen permanently.

Proof of Concept

This section explains only the Stop::stopRent() for brevity. The DoS issue on the Stop::stopRentBatch() should have similar details.

The following highlights a list of gas-exceeding significant-risk tasks of the stopRent():

1. Generating the rentalAssetUpdates array from all ERC-721/ERC-1155 rented items.
2. Processing of the onStop() of all Hook contracts.
3. Transferring all ERC-721/ERC-1155 rented items from a Gnosis Safe contract to a lender.
4. Settling and transferring all ERC-20 payments from the protocol's PaymentEscrow contract to all respective recipients.
5. Removing the rental order and all rented items from the protocol's Storage contract.

This report will analyze only the most significant risk tasks, including the processing of the onStop() of all Hook contracts (Task 2 above) and settling and transferring all ERC-20 payments from the protocol's PaymentEscrow contract to all respective recipients (Task 4). Furthermore, the gas risks when deploying the protocol to multiple chains are also analyzed in this report.

The remaining of this section will be categorized into three sub-sections as follows.

• Gas Exceeding Significant Risk #1 -- Processing Hooks' onStop()
• Gas Exceeding Significant Risk #2 -- Processing Payment Settlements
• Gas Exceeding Significant Risk #3 -- Multi-Chain Protocol Deployment

Gas Exceeding Significant Risk #1 -- Processing Hooks' onStop()

The Stop::_removeHooks() is responsible for processing and triggering all Hook contracts attached to the rental order. Therefore, the more Hooks to be processed, the more gas is required.

For each Hook contract, the _removeHooks() will trigger the onStop() to process the rental stop event. Even though the onStart() of each Hook in question would have successfully been executed on the rental start transaction, that cannot guarantee that the rental stop transaction will succeed because the stop transaction will call another callback function: onStop() (not the onStart()).

Specifically, the onStop() can consume more gas than the onStart() as the function can route the execution control flow to different logic branches.

If the Hooks consume gas beyond the block gas limit, the rental order will be permanently DoS'ed. The protocol admin cannot disable some Hooks to remedy the DoS issue because the stop transaction will be reverted when the _removeHooks() executes the Storage::hookOnStop() to check for disabled Hooks.

Furthermore, we cannot remove some Hooks from the rental order payload because the resulting rentalOrderHash derived from the Stop::_deriveRentalOrderHash() will be changed. Subsequently, the Storage::removeRentals()will revert the stop transaction, as the rentalOrderHash has changed.

    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol
    function _removeHooks(
        Hook[] calldata hooks,
        Item[] calldata rentalItems,
        address rentalWallet
    ) internal {
        ...

        // Loop through each hook in the payload.
@1      for (uint256 i = 0; i < hooks.length; ++i) { //@audit -- Loop to trigger every attached hook (the more hooks to be processed, the more gas is required)
            // Get the hook address.
            target = hooks[i].target;

            ...
            ...

            // Call the hook with data about the rented item.
            try
@2              IHook(target).onStop( //@audit -- The hook's onStop() can consume more gas than the onStart(). This poses a significant risk to the issue: "gas consumption beyond the block gas limit"
@2                  rentalWallet,
@2                  item.token,
@2                  item.identifier,
@2                  item.amount,
@2                  hooks[i].extraData
@2              )
            {} ...
            ...
            ...
        }
    }

• @1 -- Loop to trigger every attached hook (the more hooks to be processed, the more gas is required): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L205

• @2 -- The hook's onStop() can consume more gas than the onStart(). This poses a significant risk to the issue: "gas consumption beyond the block gas limit": https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L227-L233

Gas Exceeding Significant Risk #2 -- Processing Payment Settlements

The PaymentEscrow::_settlePayment() is responsible for processing and transferring all ERC-20 payments from the protocol's PaymentEscrow contract to all respective recipients.

The settlement can be categorized into two scenarios.

1. Pro-rata split payment where ERC-20 token will be transferred to both lender and renter.
2. Full-amount payment where ERC-20 token will be transferred to one respective recipient.

Token transfers are external calls that can consume a considerable amount of gas. Hence, the more ERC-20 items to be transferred, the more gas is required. Moreover, in the case of a pro-rata split payment, the gas cost required for each settlement will be doubled since the token must be transferred to both lender and renter.

If the payment settlements consume gas beyond the block gas limit, the rental order will be permanently DoS'ed.

We cannot remove some items from the rental order payload because the resulting rentalOrderHash derived from the Stop::_deriveRentalOrderHash() will be changed. Subsequently, the Storage::removeRentals()will revert the stop transaction, as the rentalOrderHash has changed.

    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol
    function _settlePayment(
        Item[] calldata items,
        OrderType orderType,
        address lender,
        address renter,
        uint256 start,
        uint256 end
    ) internal {
        ...

        // Loop through each item in the order.
@3      for (uint256 i = 0; i < items.length; ++i) { //@audit -- Loop to process every ERC-20 item (the more items to be processed, the more gas is required)
            // Get the item.
            Item memory item = items[i];

            // Check that the item is a payment.
            if (item.isERC20()) {
                ...
                ...

                // If its a PAY order but the rental hasn't ended yet.
                if (orderType.isPayOrder() && !isRentalOver) {
                    // Interaction: a PAY order which hasnt ended yet. Payout is pro-rata.
@4                  _settlePaymentProRata(
@4                      item.token,
@4                      paymentAmount,
@4                      lender,
@4                      renter,
@4                      elapsedTime,
@4                      totalTime
@4                  ); //@audit -- In the case of a pro-rata split payment, 2x gas cost is required for each payment settlement (transfer token to both lender and renter)
                }
                // If its a PAY order and the rental is over, or, if its a BASE order.
                else if (
                    (orderType.isPayOrder() && isRentalOver) || orderType.isBaseOrder()
                ) {
                    // Interaction: a pay order or base order which has ended. Payout is in full.
@5                  _settlePaymentInFull(
@5                      item.token,
@5                      paymentAmount,
@5                      item.settleTo,
@5                      lender,
@5                      renter
@5                  ); //@audit -- In the case of a full-amount payment, 1x gas cost is required for each payment settlement (transfer token to one recipient)
                } else {
                    revert Errors.Shared_OrderTypeNotSupported(uint8(orderType));
                }
            }
        }
    }

• @3 -- Loop to process every ERC-20 item (the more items to be processed, the more gas is required): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L231

• @4 -- In the case of a pro-rata split payment, 2x gas cost is required for each payment settlement (transfer token to both lender and renter): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L257-L264

• @5 -- In the case of a full-amount payment, 1x gas cost is required for each payment settlement (transfer token to one recipient): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L271-L277

Gas Exceeding Significant Risk #3 -- Multi-Chain Protocol Deployment

The reNFT protocol is planned to be deployed on multiple chains. Each chain can have a different block gas limit and gas calculation scheme. Some chains have a lower block gas limit than others.

Therefore, rental orders containing certain Hooks and rental items successfully executed on one chain cannot guarantee that they will work on other chains.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate the DoS issue, limit each rental order's number of Hooks and rental items (ERC-721, ERC-1155, and ERC-20 items). Also, the protocol's contracts should be performed intensively for gas testing when deploying to a new chain.

Assessed type

DoS

Lines of code

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

Vulnerability details

The rental stop transaction will be reverted (DoS) if one of the hook contracts is disabled, resulting in freezing all ERC-721/ERC-1155 rented items and ERC-20 payments managed by the affected rental order.

Proof of Concept

This report does not raise an admin trust issue, as the admin is trusted.

This report raises an issue regarding the risky design choice of the Stop::_removeHooks().

When a user executes the Safe::stopRent() (or Safe::stopRentBatch()) to stop a rental order, one of several tasks of the stopRent() is to invoke the Safe::_removeHooks() to trigger the onStop() of all hook contracts attached to the stopping rental order.

Before invoking each hook contract, the _removeHooks() checks whether the hook is disabled. If one of the hooks is disabled, the rental stop transaction will be reverted (DoS).

The revert issue will affect every rental order attached with any disabled hook.

    function _removeHooks(
        Hook[] calldata hooks,
        Item[] calldata rentalItems,
        address rentalWallet
    ) internal {
        ...

        // Loop through each hook in the payload.
        for (uint256 i = 0; i < hooks.length; ++i) {
            // Get the hook address.
            target = hooks[i].target;

            // Check that the hook is reNFT-approved to execute on rental stop.
@>          if (!STORE.hookOnStop(target)) {
@>              revert Errors.Shared_DisabledHook(target);
@>          } //@audit -- During the rental stop transaction, if one of the Hook contracts attached to the rental order is disabled, the transaction will be reverted

            ...

            // Call the hook with data about the rented item.
            try
                IHook(target).onStop(
                    rentalWallet,
                    item.token,
                    item.identifier,
                    item.amount,
                    hooks[i].extraData
                )
            {} ...

            ...
        }
    }

• During the rental stop transaction, if one of the Hook contracts attached to the rental order is disabled, the transaction will be reverted: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L210-L212

Impact

This report does not raise an admin trust issue, as the admin is trusted.

This report raises an issue regarding the risky design choice of the Stop::_removeHooks().

The revert issue will affect every rental order attached with any disabled hook.

Users will be unable to stop their rental orders. Subsequently, all ERC-721/ERC-1155 rented items and ERC-20 payments managed by the affected rental orders will be frozen.

This DoS issue can directly damage the protocol's users and reduce the protocol's trustworthiness.

Tools Used

Manual Review

Recommended Mitigation Steps

Since the design and application of the hook contracts are outside the audit scope, recommending solutions with such limited design knowledge might be inaccurate. In other words, proper solutions should depend on the protocol developer's design choices.

The following suggests some ideas that might mitigate the issue:

1. Implement a more fine-grained option to determine which hooks can be skippable (if they are disabled) or which hooks must be explicitly executed (revert the transaction if they are disabled).
2. Implement other fallback solutions for users to stop their rental orders safely.

Assessed type

DoS

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L175 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L178 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L201

Vulnerability details

The _settlePaymentProRata() and _settlePaymentInFull() of the PaymentEscrow contract are responsible for sending ERC-20 payments to respective recipients (lender and/or renter) during the process of stopping a rental order.

Both functions apply a push model for sending the payments, which assumes that the recipients will always be available to receive the payments. However, if any recipient gets blacklisted by the token or if the token gets paused, the rental stop transaction will be blocked suddenly, refusing a rental order from being stopped.

Impact

To elaborate on the vulnerability, for instance, consider the case of the PAY rental order, where a lender pays for a renter to service something. This type of rental order allows a lender to stop the order before the scheduled order expiration. The lender will only pay for a duration that has passed for a renter, and the remaining funds locked in the PaymentEscrow contract will be refunded to the lender (after deducting a protocol fee). In other words, the payment will be a pro-rata split between the lender and renter managed by the PaymentEscrow::_settlePaymentProRata().

Imagine the renter got blacklisted by the payment token (ERC-20) or the payment token got paused. The rental stop transaction will be reverted by the _settlePaymentProRata().

The following lists some impacts to the lender:

1. The lender cannot stop the rental order before the order expiration and is forced to pay the full amount once the expiration has reached (Impact: MEDIUM).
2. If the renter gets blacklisted for a very long time or forever, the lender cannot retrieve their ERC-721/ERC-1155 rented items (Impact: may vary between MEDIUM and HIGH based on the type of services).

For the BASE rental order, the impact on the lender is similar to the 2nd impact listed above. Specifically, if a lender who is a recipient of the payment gets blacklisted, the rental order stop transaction will be reverted by the _settlePaymentInFull(). Consequently, the lender cannot retrieve their ERC-721/ERC-1155 rented items (Impact: HIGH).

Proof of Concept

The PaymentEscrow::_settlePaymentProRata() has two push-payment transfers in L175 (lender) and L178 (renter).

Meanwhile, the PaymentEscrow::_settlePaymentInFull() has one push-payment transfer in L201 (lender or renter).

    function _settlePaymentProRata(
        address token,
        uint256 amount,
        address lender,
        address renter,
        uint256 elapsedTime,
        uint256 totalTime
    ) internal {
        // Calculate the pro-rata payment for renter and lender.
        (uint256 renterAmount, uint256 lenderAmount) = _calculatePaymentProRata(
            amount,
            elapsedTime,
            totalTime
        );

        // Send the lender portion of the payment.
@1      _safeTransfer(token, lender, lenderAmount); //@audit -- If 'lender' got blacklisted or the 'token' got paused, the rental stop transaction will be reverted

        // Send the renter portion of the payment.
@2      _safeTransfer(token, renter, renterAmount); //@audit -- If 'renter' got blacklisted or the 'token' got paused, the rental stop transaction will be reverted
    }

    ...

    function _settlePaymentInFull(
        address token,
        uint256 amount,
        SettleTo settleTo,
        address lender,
        address renter
    ) internal {
        // Determine the address that this payment will settle to.
        address settleToAddress = settleTo == SettleTo.LENDER ? lender : renter;

        // Send the payment.
@3      _safeTransfer(token, settleToAddress, amount); //@audit -- If 'settleToAddress' ('lender' or 'renter') got blacklisted or the 'token' got paused, the rental stop transaction will be reverted
    }

• @1 -- If 'lender' got blacklisted or the 'token' got paused, the rental stop transaction will be reverted: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L175

• @2 -- If 'renter' got blacklisted or the 'token' got paused, the rental stop transaction will be reverted: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L178

• @3 -- If 'settleToAddress' ('lender' or 'renter') got blacklisted or the 'token' got paused, the rental stop transaction will be reverted: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L201

Tools Used

Manual Review

Recommended Mitigation Steps

Since the ERC-20 payment funds (full amount) would be locked in the PaymentEscrow contract at the start of the rental order, so no further transfer from the lender or renter is required. For this reason, the rental order can be stopped even if one recipient gets blocklisted (or when the payment token gets paused).

Suppose the first attempt of the push-payment transfers is denied. The protocol can provide a fallback solution by applying the pull-payment model, allowing users to withdraw ERC-20 payment tokens later. This solution enables the rental order to be stopped flawlessly, returning all ERC-721/ERC-1155 rented items to the lender.

Assessed type

DoS