Hooks required to succeed on both `onStart` and `onStop` may prevent rental termination

[c4-bot-1]

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L480-L482 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, the current implementation requires hook to always succeed on both onStart() and onStop() calls if they are provided in an order. This means if a hook is only whitelisted for onStop(), it will not be possible to create rentals using it as the addHooks() function, called during the creation of a rental, will revert: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L480-L482

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

Conversely, if a hook is provided for onStart() but isn't whitelisted for onStop(), it will prevent the rental from being stopped. 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 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 lead to a situation where a rental cannot be stopped because a hook used in the onStart() phase is not whitelisted for onStop(), which would lock assets in the safe and prevent users from reclaiming their assets.

Proof of Concept

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

1. Create a BASE order where a lender offers an ERC1155 in exchange for some ERC20 tokens.
2. Define a whitelist and a hook for the rental. The hook is enabled for onStart() calls only.
3. Fulfill the order, which executes the token swap and starts the rental.
4. Speed up time past the rental expiration.
5. Attempt to stop the rental. Since the hook is not whitelisted for onStop(), the stopRent() function should revert with an error, confirming the issue.

The following test case follows the steps outlined above:

function test_Vuln_OnStartHookPreventsRentalFromBeingStopped() public {
    // create a BASE order where a lender offers an ERC1155 in
    // exchange for some erc20 tokens
    createOrder({
        offerer: alice,
        orderType: OrderType.BASE,
        erc721Offers: 0,
        erc1155Offers: 1,
        erc20Offers: 0,
        erc721Considerations: 0,
        erc1155Considerations: 0,
        erc20Considerations: 1
    });

    // define the whitelist
    address[] memory whitelist = new address[](2);
    whitelist[0] = address(bob.safe);
    whitelist[1] = address(carol.safe);

    // define the hook for the rental
    Hook[] memory hooks = new Hook[](1);
    hooks[0] = Hook({
        // the hook contract to target
        target: address(hook),
        // index of the item in the order to apply the hook to
        itemIndex: 0,
        // any extra data that the hook will need.
        extraData: abi.encode(whitelist)
    });

    // use an amendment to add hooks
    withHooks(hooks);

    // finalize the order creation
    (
        Order memory order,
        bytes32 orderHash,
        OrderMetadata memory metadata
    ) = finalizeOrder();

    // create an order fulfillment
    createOrderFulfillment({
        _fulfiller: bob,
        order: order,
        orderHash: orderHash,
        metadata: metadata
    });

    // finalize the base order fulfillment. This executes the token swap
    // and starts the rental.
    // ------- Identical to existing "test_Success_RenterIsWhitelisted" until here -------
    RentalOrder memory rentalOrder = finalizeBaseOrderFulfillment();

    // default rent duration is 500
    // speed up in time past the rental expiration
    vm.warp(block.timestamp + 750);

    // Alice cannot stop the rental as the hook is enabled for `onStart` calls only,
    // but it is always required to also succeed on `onStop`
    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 to this issue could be to modify the _addHooks() and _removeHooks() functions to ignore hooks that aren't whitelisted, instead of reverting. This would allow rentals to be created and stopped even if a hook is not whitelisted for both phases.

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as duplicate of #501

[c4-judge]

0xean marked the issue as satisfactory

[0xEVom]

@0xean I can definitely see how this submission was marked as a duplicate of #501 since they look very similar on the surface, but this one centers on a different issue and I would greatly appreciate it if you could consider it separately. Here's how both issues differ:

• 501 (and my duplicate #617) centers on the fact that removing a hook from the whitelist will prevent ongoing rentals with that hook from being stopped

• this issue establishes that all hooks that are whitelisted for onStop also need to be whitelisted for onStart in order to create a rental, and all hooks whitelisted for onStart need to be whitelisted for onStop in order to stop a rental. As such, it is impossible to create and whitelist hooks with the intended granularity.

[0xean]

leaving them as dupes, underlying issue is the same