Disabling a hook during rent prevents rental from being stopped

[c4-bot-10]

Lines of code

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

Vulnerability details

Impact

The reNFT protocol uses hooks, which can act as middleware to a target contract or execute during rental start or stop. The updateHookStatus() function in the Storage module allows the admin to set the permissions of a hook using a bitmap consisting of 3 bits, which represent whether the onTransaction(), onStart() and onStop() functions are whitelisted for a given module, respectively: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol#L313-L326

function updateHookStatus(
    address hook,
    uint8 bitmap
) external onlyByProxy permissioned {
    // Require that the `hook` address is a contract.
    if (hook.code.length == 0) revert Errors.StorageModule_NotContract(hook);

    // 7 is 0x00000111. This ensures that only a valid bitmap can be set.
    if (bitmap > uint8(7))
        revert Errors.StorageModule_InvalidHookStatusBitmap(bitmap);

    // Update the status of the hook.
    hookStatus[hook] = bitmap;
}

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol#L144-L172

/**
 * @notice Determines whether the `onTransaction()` function is enabled for the hook.
 *
 * @param hook Address of the hook contract.
 */
function hookOnTransaction(address hook) external view returns (bool) {
    // 1 is 0x00000001. Determines if the masked bit is enabled.
    return (uint8(1) & hookStatus[hook]) != 0;
}

/**
 * @notice Determines whether the `onStart()` function is enabled for the hook.
 *
 * @param hook Address of the hook contract.
 */
function hookOnStart(address hook) external view returns (bool) {
    // 2 is 0x00000010. Determines if the masked bit is enabled.
    return uint8(2) & hookStatus[hook] != 0;
}

/**
 * @notice Determines whether the `onStop()` function is enabled for the hook.
 *
 * @param hook Address of the hook contract.
 */
function hookOnStop(address hook) external view returns (bool) {
    // 4 is 0x00000100. Determines if the masked bit is enabled.
    return uint8(4) & hookStatus[hook] != 0;
}

However, disabling a hook for onStop() will prevent any ongoing rentals in which the hook is enabled from being closed. This is because the _removeHooks() function, which is called form stopRent() and stopRentBatch() on rental termination, will revert if any of the hooks in the order is not currently whitelisted for onStop(): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L209-L212

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

This could effectively make it impossible for the protocol admin to remove hooks from the onStop() whitelist without harming users, as there may always be an ongoing rental which uses a given hook. This impacts the functioning of the protocol and is hence classified as medium severity.

Proof of Concept

We can validate the issue through an additional test case for the ERC20RewardHook.t.sol test file, which will perform the following actions:

1. Start a rental with a hook enabled for onStart and onStop calls.
2. Speed up time past the rental expiration.
3. Remove the hook from the whitelist.
4. Attempt to stop the rental, which should fail and revert with a Shared_DisabledHook error.

The following test case follows the steps outlined above:

import {Errors} from "@src/libraries/Errors.sol";

    function test_Vuln_RemoveFromWhitelistDuringRental() public {
        // start the rental. This should activate the hook and begin
        // accruing rewards while the rental is active.
        RentalOrder memory rentalOrder = _startRentalWithGameToken();

        // roll ahead by 100 blocks so that rewards can accrue
        vm.roll(block.number + 100);

        // speed up in time past the rental expiration
        vm.warp(block.timestamp + 750);

        // remove hook from whitelist
        vm.prank(deployer.addr);
        guard.updateHookStatus(address(hook), uint8(0));

        // rental cannot be stopped
        vm.prank(alice.addr);
        vm.expectRevert(
            abi.encodeWithSelector(
                Errors.Shared_DisabledHook.selector,
                address(hook)
            )
        );
        stop.stopRent(rentalOrder);
    }

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

A potential solution could be to ignore non-whitelisted hooks or even better, check if both onStart() and onStop() hooks are whitelisted when a rental is started instead of when they are called. This would ensure that once a rental has started, it can be stopped regardless of any changes to the hook whitelist.

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as duplicate of #501

[c4-judge]

0xean marked the issue as satisfactory