Long period self rents will block hook disabling

[c4-bot-8]

Lines of code

https://github.com/re-nft/smart-contracts/blob/main/src/policies/Stop.sol#L194-L212

Vulnerability details

Impact

The protocol has admin-owned hooks, which can be used instead of _checkTransaction from GnosisSafe to disable transferring and approving the rented assets. There can be cases where hooks can contain vulnerable code in their onTransaction function and admins should disable them. Still, a malicious user can effectively block them from disabling by creating a rent to himself with a long rent period. As there will most likely be other rents using these hooks, admins will be forced to call updateHookStatus for onStop and all funds will be locked inside the PaymentEscrow contract, with no way to be recovered.

Proof of Concept

Stop.sol#L211

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); //@audit will revert if hook onStop function is disabled
        }

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a blacklist to disable hooks only for certain renters, this will ensure that other users who are using the same hook will be able to stop their rents, without losing their funds.

Assessed type

Context

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[141345]

intentionally retain malicious hook, onStop function will be affected to lock fund

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-sponsor]

Alec1017 (sponsor) confirmed

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked the issue as selected for report

[akshaysrivastav]

Hey @0xean thanks for judging this. I think this report needs a revisit.

• This report revolves around a bug in onTransaction hook. The sponsors have already shared their input on this hook https://github.com/code-423n4/2024-01-renft-findings/issues/396#issuecomment-1910470022.
• The impact is not clearly defined in this report. The scenario is a self rent so who is loosing funds?
• Even if you consider this report as valid, its impact is similar to what's discussed in #501

Can you please recheck the impact, validity and duplication status of this report. Thanks.

[Slavchew]

Hi @akshaysrivastav,

• The impact is clear, a user can block hook removal if he rents for a long time. The protocol team wants first to close all rents with this hook and then remove the hook.
• The root case of this and #501 is very similar, but here a malicious user can block the hook from being removed, causing all other rentals that used the hook to be blocked. Otherwise, the #501 is that if only onStop is disabled not the whole hook, it blocks users from stopping the rentals. One is blocking the system by a malicious user, the other is an admin and context issue.

Now leave it to @0xean to conclude if this is another path and a different issue or if it's a duplicate, but it's definitely valid.

[0xEVom]

Agree that this is just another way of framing #501.

• 501 points out that if there are ongoing rentals with an active onStop hook and it is disabled, stopRent will always revert.

• this issue assumes that if there are ongoing rentals with an active onStop hook, the admin will refrain from disabling it because that would brick the rentals.

Seems like two sides of the same coin to me.

[c4-judge]

0xean marked the issue as duplicate of #501

[0xean]

agree, should be duped.

[c4-judge]

0xean marked the issue as not selected for report