Improper hook validation causes rentals to be stuck

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

Hooks are contracts that may be included in an order, for them to be called during rental creation and the stopping of a rental. They all have a status that represents whether they can be used for gnosis safe transactions (hookOnTransaction), start of rental order (hookOnStart), stop of rental order (hookOnStop), or a combination of the three.

If a set of hooks are specified to be included in an order they all must have the status onStart. This validation can be found in the _addHooks internal function:

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

The array of hooks, used by a rental, is specified during its creation in the function validateOrder. They are also included in an order's unique hash which is essential when a rental is stopped, below we will see why this is really important.

When a rental is stopped stopRent is called with an only parameter RentalOrder calldata order. From this parameter, the same orderHash, used when creating the rental, must be derived in order for stopRent to not revert (this is checked in removeRentals in Storage.sol). Therefore, the same hooks specified during order creation must be specified during the stoppage of the rental.

The issue is that when a rental is stopped, the same hooks from its creation are called and they are all required to have the status hookOnStop, otherwise a revert is initiated:

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

Therefore, if a hook, specified in the rental creation, only has the hookOnStart status and does not have hookOnStop the rental will be created successfully and rental tokens will be transferred to the rental safe but when the rental is stopped the call would always fail as the hooks, specified during creation, do not have the hookOnStop status, causing all tokens to be stuck.

Proof of Concept

In the dev comments above the _removeHooks function it is written:
@dev When a rental order is stopped, process each hook one by one but only if the hook's status is set to execute on a rental stop.
This statement is incorrect as it assumes that the function will skip past hooks that are not for a rental stop and only process the ones that have that purpose. In reality, the function reverts on an invalid hook.

Tools Used

Manual review

Recommended Mitigation Steps

There are a few possible solutions depending on the protocol's intentions:
1/ on rental creation make sure that all hooks also have the hookOnStop status.
2/ on rental creation make sure that all hooks have either hookOnStop or hookOnStart and do the same check on a rental stop

Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as duplicate of #501

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean changed the severity to 2 (Med Risk)