Wrong assumption that the same hook is reNFT-approved to execute on rental `start` and `stop`.

[c4-bot-4]

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L194-L250 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L464-L520

Vulnerability details

In Create._rentFromZone() the function _addHooks() is used to process each hook but only if the hook's status is set to execute on a rental start. The array of hooks is used to generate the rental order, which is then hashed and saved to storage.

  function _addHooks(
        Hook[] memory hooks,
        SpentItem[] memory offerItems,
        address rentalWallet
    ) internal {
        // Define hook target, offer item index, and an offer item.
        address target;
        uint256 itemIndex;
        SpentItem memory offer;

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

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

When a rental order is being stopped the same RentalOrder struct generated and emitted during rental creation (that is in Create._rentFromZone()) is used as the input value for the function Stop.stopRent(), this means that the values under this order (struct) remain the same.

    function _removeHooks(
        Hook[] calldata hooks,
        Item[] calldata rentalItems,
        address rentalWallet
    ) internal {
        // Define hook target, item index, and item.
        address target;
        uint256 itemIndex;
        Item memory item;

        // 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);
            }

            // Get the rental item index for this hook.
            itemIndex = hooks[i].itemIndex;
        //More Code...
 }

Since the elements in the Hooks array remain the same the protocol assumes that the same hooks will be approved for both OnStart and OnStop transactions. In a scenario where the hook being used for the rental order is approved for OnStart but not OnStop the _removeHooks() function will keep reverting thereby preventing the function stopRent() from executing.

Impact

This scenario will lead to rentals being open forever, preventing the lender from getting their tokens back.

Proof of Concept

• Create._addHooks() is executed during rental order creation using a hook array.
• An element in the hooks array is approved for OnStart transactions but not OnStop transactions.
• After the rental period is over, stopRent() is called
• stopRent() calls _removeHooks() which runs a loop through the array of hooks checking if the hook is reNFT-approved to execute on rental stop.
• Once the hook in the array that is not approved for rental stop (OnStop) transactions is reached the function reverts.
• Since the hash of the order struct has already been saved in storage and array hooks cannot be changed.
• The function stopRent() becomes DOS preventing the rental order from being closed.

Tools Used

Manual Review

Recommended Mitigation Steps

The Create._addHooks() and Stop._removeHooks() functions should be corrected from reverting on hooks that are not approved to skipping the current iteration in the loop. This change will account for hooks not approved for both OnStart and OnStop transactions.

Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as duplicate of #501

[c4-judge]

0xean marked the issue as satisfactory