reNFT - rbserver's results

Lines of code

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Guard.sol#L195-L293 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/libraries/RentalConstants.sol#L57-L62 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Guard.sol#L108-L116 https://github.com/safe-global/safe-contracts/blob/f8bd2159b64392d5b594f4e056be258ade2fefab/contracts/base/ModuleManager.sol#L57-L70

Vulnerability details

Impact

The Guard._checkTransaction function has an error for executing _revertNonWhitelistedExtension(extension) with extension being prevModule instead of module when the selector corresponds to disableModule(address,address). Since calling _revertNonWhitelistedExtension(extension) with extension being prevModule would not revert if prevModule is whitelisted, the renter, who is the safe owner, can make the safe to call the ModuleManager.disableModule function to disable module that has become non-whitelisted and should not be allowed to be disabled.

Proof of Concept

When the selector corresponds to disableModule(address,address), calling the following Guard._checkTransaction function would execute address extension = address(uint160(uint256(_loadValueFromCalldata(data, gnosis_safe_disable_module_offset)))), where _loadValueFromCalldata(data, gnosis_safe_disable_module_offset) would be resolved to _loadValueFromCalldata(data, 0x24).

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

    function _checkTransaction(address from, address to, bytes memory data) private view {
        bytes4 selector;

        // Load in the function selector.
        assembly {
            selector := mload(add(data, 0x20))
        }

        ...
        } 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);
        ...
    }

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/libraries/RentalConstants.sol#L57-L62

// bytes4(keccak256("disableModule(address,address)"));
bytes4 constant gnosis_safe_disable_module_selector = 0xe009cfde;

...
uint256 constant gnosis_safe_disable_module_offset = 0x24;

Because the ModuleManager.disableModule function below has two inputs, which are prevModule and module, executing _loadValueFromCalldata(data, 0x24) would return the bytes32 representation of prevModule. This means that, in the Guard._checkTransaction function, extension is set to prevModule instead of module. This is an error because prevModule is the previous module in the modules linked list while module is the actual module to be removed, and _revertNonWhitelistedExtension(extension) should be executed with extension being module, not prevModule. However, _revertNonWhitelistedExtension(extension) is executed with extension being prevModule and would not revert if prevModule is a whitelisted extension. Because executing _revertNonWhitelistedExtension(extension) with extension being module would revert if module has become non-whitelisted, which does not occur, the renter, who is the safe owner, can make the safe to disable such module that should not be allowed to be disabled.

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Guard.sol#L108-L116

    function _loadValueFromCalldata(
        bytes memory data,
        uint256 offset
    ) private pure returns (bytes32 value) {
        // Load the `uint256` from calldata at the offset.
        assembly {
            value := mload(add(data, offset))
        }
    }

https://github.com/safe-global/safe-contracts/blob/f8bd2159b64392d5b594f4e056be258ade2fefab/contracts/base/ModuleManager.sol#L57-L70

    /**
     * @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 {
        // 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);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

When the selector corresponds to disableModule(address,address), the Guard._checkTransaction function can be updated to execute address extension = address(uint160(uint256(_loadValueFromCalldata(data, 0x44)))).

Assessed type

Error

Lines of code

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Signer.sol#L44-L65 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Signer.sol#L339-L408 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Signer.sol#L162-L195 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Signer.sol#L218-L239 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Create.sol#L626-L639

Vulnerability details

Impact

The encoded data returned by the Signer._deriveRentalOrderHash and Signer._deriveOrderMetadataHash functions miss encoded values for certain variables, which make them not compliant with EIP-712. Users, who follow EIP-712, can face difficulties when using this protocol. For instance, a lender, who provides an EIP-712 compliant zoneHash for creating a rental order, can never have such order be fulfilled.

Proof of Concept

As shown by the Signer.constructor and Signer._deriveRentalTypehashes functions below, _RENTAL_ORDER_TYPEHASH contains the encoded type for address rentalWallet, and _ORDER_METADATA_TYPEHASH includes the encoded types for uint8 orderType and bytes emittedExtraData.

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Signer.sol#L44-L65

    constructor() {
        ...
        // Derive name and version hashes alongside required EIP-712 typehashes.
        (
            _ITEM_TYPEHASH,
            _HOOK_TYPEHASH,
            _RENTAL_ORDER_TYPEHASH,
            _ORDER_FULFILLMENT_TYPEHASH,
            _ORDER_METADATA_TYPEHASH,
            _RENT_PAYLOAD_TYPEHASH
        ) = _deriveRentalTypehashes();

        ...
    }

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Signer.sol#L339-L408

    function _deriveRentalTypehashes()
        internal
        pure
        returns (
            bytes32 itemTypeHash,
            bytes32 hookTypeHash,
            bytes32 rentalOrderTypeHash,
            bytes32 orderFulfillmentTypeHash,
            bytes32 orderMetadataTypeHash,
            bytes32 rentPayloadTypeHash
        )
    {
        ...

        // Construct the RentalOrder type string.
        bytes memory rentalOrderTypeString = abi.encodePacked(
            "RentalOrder(bytes32 seaportOrderHash,Item[] items,Hook[] hooks,uint8 orderType,address lender,address renter,address rentalWallet,uint256 startTimestamp,uint256 endTimestamp)"
        );

        ...

        // Derive the RentalOrder type hash using the corresponding type string.
        rentalOrderTypeHash = keccak256(
            abi.encode(rentalOrderTypeString, hookTypeString, itemTypeString)
        );

        {
            ...

            // Construct the OrderMetadata type string.
            bytes memory orderMetadataTypeString = abi.encodePacked(
                "OrderMetadata(uint8 orderType,uint256 rentDuration,Hook[] hooks,bytes emittedExtraData)"
            );

            ...

            // Derive the OrderMetadata type hash using the corresponding type string.
            orderMetadataTypeHash = keccak256(orderMetadataTypeString);
        }
    }

According to https://eips.ethereum.org/EIPS/eip-712#definition-of-encodedata, The encoding of a struct instance is `enc(value₁) ‖ enc(value₂) ‖ … ‖ enc(valueₙ)`, i.e. the concatenation of the encoded member values in the order that they appear in the type.

However, the encoded data returned by the following Signer._deriveRentalOrderHash function does not include the encoded value for order.rentalWallet while _RENTAL_ORDER_TYPEHASH does contain the encoded type for address rentalWallet. Hence, the Signer._deriveRentalOrderHash function is not compliant with EIP-712.

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Signer.sol#L162-L195

    function _deriveRentalOrderHash(
        RentalOrder memory order
    ) internal view returns (bytes32) {
        // Create arrays for items and hooks.
        bytes32[] memory itemHashes = new bytes32[](order.items.length);
        bytes32[] memory hookHashes = new bytes32[](order.hooks.length);

        // Iterate over each item.
        for (uint256 i = 0; i < order.items.length; ++i) {
            // Hash the item.
            itemHashes[i] = _deriveItemHash(order.items[i]);
        }

        // Iterate over each hook.
        for (uint256 i = 0; i < order.hooks.length; ++i) {
            // Hash the hook.
            hookHashes[i] = _deriveHookHash(order.hooks[i]);
        }

        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
                )
            );
    }

Similarly, the encoded data return by the following Signer._deriveOrderMetadataHash function does not contain the encoded values for metadata.orderType and metadata.emittedExtraData while _ORDER_METADATA_TYPEHASH includes the encoded types for uint8 orderType and bytes emittedExtraData. This means that the Signer._deriveOrderMetadataHash function is not compliant with EIP-712 as well.

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Signer.sol#L218-L239

    function _deriveOrderMetadataHash(
        OrderMetadata memory metadata
    ) internal view returns (bytes32) {
        // Create array for hooks.
        bytes32[] memory hookHashes = new bytes32[](metadata.hooks.length);


        // Iterate over each hook.
        for (uint256 i = 0; i < metadata.hooks.length; ++i) {
            // Hash the hook
            hookHashes[i] = _deriveHookHash(metadata.hooks[i]);
        }


        // Derive and return the metadata hash as specified by EIP-712.
        return
            keccak256(
                abi.encode(
                    _ORDER_METADATA_TYPEHASH,
                    metadata.rentDuration,
                    keccak256(abi.encodePacked(hookHashes))
                )
            );
    }

Because of these non-compliances, users, who follow EIP-712, can face difficulties when using this protocol. For example, the zoneHash provided by the lender that is a part of her or his seaport order can include the encoded values for metadata.orderType and metadata.emittedExtraData for being compliant with EIP-712 but the encoded data returned by the Signer._deriveOrderMetadataHash function, which is not compliant with EIP-712, does not contain these encoded values. This would cause the following Create._isValidOrderMetadata function to always revert with _deriveOrderMetadataHash(metadata) != zoneHash being true. As a result, the lender's rental order can never be fulfilled even though she or he uses an EIP-712 compliant zoneHash.

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Create.sol#L626-L639

    function _isValidOrderMetadata(
        OrderMetadata memory metadata,
        bytes32 zoneHash
    ) internal view {
        ...

        // Check that the zone hash is equal to the derived hash of the metadata.
        if (_deriveOrderMetadataHash(metadata) != zoneHash) {
            revert Errors.CreatePolicy_InvalidOrderMetadataHash();
        }
    }

Tools Used

Manual Review

Recommended Mitigation Steps

The Signer._deriveRentalOrderHash function can be updated to return an encoded data that includes the encoded value for order.rentalWallet. Moreover, the Signer._deriveOrderMetadataHash function can be updated to return an encoded data that contains the encoded values for metadata.orderType and metadata.emittedExtraData.

Assessed type

Other

Lines of code

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Create.sol#L733-L775 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Create.sol#L530-L617 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Create.sol#L464-L520 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/Storage.sol#L189-L203 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Stop.sol#L265-L306 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Stop.sol#L313-L364 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Stop.sol#L194-L250

Vulnerability details

Impact

When the order's hooks should only be used for creating but not stopping the order, these hooks are only enabled for creating such order and not enabled for stopping such order; in this situation, stopping such order reverts. As a result, the lent ERC721 and ERC1155 tokens would remain in the safe and the ERC20 payment tokens would remain in the payment escrow. Hence, the lender loses the lent ERC721 and ERC1155 tokens, and the renter or lender, depending on the order type, loses the ERC20 payment tokens that she or he is entitled to.

Proof of Concept

When creating an order, the following Create.validateOrder function is called, which further calls the Create._rentFromZone function below.

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Create.sol#L733-L775

    function validateOrder(
        ZoneParameters calldata zoneParams
    ) external override onlyRole("SEAPORT") returns (bytes4 validOrderMagicValue) {
        // Decode the signed rental zone payload from the extra data.
        (RentPayload memory payload, bytes memory signature) = abi.decode(
            zoneParams.extraData,
            (RentPayload, bytes)
        );

        // Create a payload of seaport data.
        SeaportPayload memory seaportPayload = SeaportPayload({
            orderHash: zoneParams.orderHash,
            zoneHash: zoneParams.zoneHash,
            offer: zoneParams.offer,
            consideration: zoneParams.consideration,
            totalExecutions: zoneParams.totalExecutions,
            fulfiller: zoneParams.fulfiller,
            offerer: zoneParams.offerer
        });

        ...

        // Initiate the rental using the rental manager.
        _rentFromZone(payload, seaportPayload);

        ...
    }

When calling the Create._rentFromZone function, if payload.metadata.hooks.length > 0 is true, the Create._addHooks function below would be called to use the specified hooks if they are enabled for creating the order.

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Create.sol#L530-L617

    function _rentFromZone(
        RentPayload memory payload,
        SeaportPayload memory seaportPayload
    ) internal {
        ...

        // PAYEE orders are considered mirror-images of a PAY order. So, PAYEE orders
        // do not need to be processed in the same way that other order types do.
        if (
            payload.metadata.orderType.isBaseOrder() ||
            payload.metadata.orderType.isPayOrder()
        ) {
            ...

            // Generate the rental order.
            RentalOrder memory order = RentalOrder({
                seaportOrderHash: seaportPayload.orderHash,
                items: items,
                hooks: payload.metadata.hooks,
                orderType: payload.metadata.orderType,
                lender: seaportPayload.offerer,
                renter: payload.intendedFulfiller,
                rentalWallet: payload.fulfillment.recipient,
                startTimestamp: block.timestamp,
                endTimestamp: block.timestamp + payload.metadata.rentDuration
            });

            // Compute the order hash.
            bytes32 orderHash = _deriveRentalOrderHash(order);

            // Interaction: Update storage only if the order is a Base Order or Pay order.
            STORE.addRentals(orderHash, _convertToStatic(rentalAssetUpdates));

            ...

            // Interaction: Process the hooks associated with this rental.
            if (payload.metadata.hooks.length > 0) {
                _addHooks(
                    payload.metadata.hooks,
                    seaportPayload.offer,
                    payload.fulfillment.recipient
                );
            }

            // Emit rental order started.
            _emitRentalOrderStarted(order, orderHash, payload.metadata.emittedExtraData);
        }
    }

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Create.sol#L464-L520

    function _addHooks(
        Hook[] memory hooks,
        SpentItem[] memory offerItems,
        address rentalWallet
    ) internal {
        // Define hook target, offer item index, and an offer item.
        address target;
        ...

        // Loop through each hook in the payload.
        for (uint256 i = 0; i < hooks.length; ++i) {
            // Get the hook's target address.
            target = hooks[i].target;

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

            ...
        }
    }

In the Create._rentFromZone function, since payload.metadata.hooks is a part of order, orderHash contains the hashed information for payload.metadata.hooks in which orders[orderHash] for such orderHash would be set to true when the Create._rentFromZone function calls the following Storage.addRentals function.

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/Storage.sol#L189-L203

    function addRentals(
        bytes32 orderHash,
        RentalAssetUpdate[] memory rentalAssetUpdates
    ) external onlyByProxy permissioned {
        // Add the order to storage.
        orders[orderHash] = true;

        ...
    }

To stop the same order by calling the Stop.stopRent or Stop.stopRentBatch function below, the order or orders[i] input must correspond to the orderHash that was previously constructed when calling the Create._rentFromZone function and stored with the corresponding orders[orderHash] being true in the Storage contract. Because such orderHash does contain the hashed information about the hooks that were used when creating the order, the order or orders[i] input must include the same hooks as well. This means that the hooks that were used for creating the order must also be the hooks that need to be used for stopping the order.

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Stop.sol#L265-L306

    function stopRent(RentalOrder calldata order) external {
        ...

        // Interaction: process hooks so they no longer exist for the renter.
        if (order.hooks.length > 0) {
            _removeHooks(order.hooks, order.items, order.rentalWallet);
        }

        ...
    }

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Stop.sol#L313-L364

    function stopRentBatch(RentalOrder[] calldata orders) external {
        ...

        // Process each rental order.
        // Memory will become safe after this block.
        for (uint256 i = 0; i < orders.length; ++i) {
            ...

            // Interaction: Process hooks so they no longer exist for the renter.
            if (orders[i].hooks.length > 0) {
                _removeHooks(orders[i].hooks, orders[i].items, orders[i].rentalWallet);
            }

            ...
        }


        ...
    }

It is possible that the hooks that were used when creating the order are only enabled for creating the order. For instance, hookStatus for these hooks can be set to 2 specifically in the Storage contract because these hooks should only be used for creating but not stopping the order. In this case, the order can be created while using these hooks but stopping such order would revert because calling the following Stop._removeHooks function reverts if !STORE.hookOnStop(target) is true. As a result, such order cannot be stopped, which causes the lent ERC721 and ERC1155 tokens to remain in the safe and the ERC20 payment tokens to remain in the payment escrow.

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

    function _removeHooks(
        Hook[] calldata hooks,
        Item[] calldata rentalItems,
        address rentalWallet
    ) internal {
        // Define hook target, item index, and item.
        address target;
        ...

        // 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);
            }

            ...
        }
    }

Tools Used

Manual Review

Recommended Mitigation Steps

The Stop._removeHooks function can be updated to not call the corresponding hook instead of reverting if !STORE.hookOnStop(target) is true. Similarly, the Create._addHooks function can be updated to not call the corresponding hook instead of reverting if !STORE.hookOnStart(target) is true for covering the case where the hooks should only be used for stopping but not creating the order.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Since the Guard._checkTransaction function does not revert when the selector corresponds to setApprovalForScope(address,bytes32,bool), an approved operator can transfer token IDs in a specified scope out from the safe if the token is an ERC1155 token that implements ERC-1761. Hence, the lender loses the lent amount of these ERC1155 token IDs.

Proof of Concept

https://eips.ethereum.org/EIPS/eip-1155 has the following statements.

• The function `setApprovalForAll` allows an operator to manage one’s entire set of tokens on behalf of the approver. To permit approval of a subset of token IDs, an interface such as ERC-1761 Scoped Approval Interface is suggested.
• Standard token approval interfaces can be used, such as the suggested ERC-1761 Scoped Approval Interface which is compatible with ERC-1155.

Based on these statements, it is possible that an ERC1155 token implements ERC-1761. For instance, according to https://eips.ethereum.org/EIPS/eip-1761#abstract, Game developers could share an ERC-1155 contract where each developer manages tokens under a specified scope. When token IDs of an ERC1155 token are grouped under a scope, the ERC-1761's setApprovalForScope function can be used to approve an operator to manage these token IDs in the specified scope. After the operator gains such approval, she or he can successfully transfer a token ID under such scope since the ERC-1761's isApprovedForScope would return true for such operator-scope combination.

Because the following Guard._checkTransaction function does not revert when the selector corresponds to setApprovalForScope(address,bytes32,bool), when an ERC1155 token implements ERC-1761, the renter, who is the safe owner, can make the safe to call the setApprovalForScope function to approve an operator to manage the token IDs in the specified scope. The operator can then transfer such token IDs out from the safe. As a result, the lender loses the lent amount of these ERC1155 token IDs.

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

    function _checkTransaction(address from, address to, bytes memory data) private view {
        bytes4 selector;

        // Load in the function selector.
        assembly {
            selector := mload(add(data, 0x20))
        }

        if (selector == e721_safe_transfer_from_1_selector) {
            // Load the token ID from calldata.
            uint256 tokenId = uint256(
                _loadValueFromCalldata(data, e721_safe_transfer_from_1_token_id_offset)
            );

            // Check if the selector is allowed.
            _revertSelectorOnActiveRental(selector, from, to, tokenId);
        } else if (selector == e721_safe_transfer_from_2_selector) {
            // Load the token ID from calldata.
            uint256 tokenId = uint256(
                _loadValueFromCalldata(data, e721_safe_transfer_from_2_token_id_offset)
            );

            // Check if the selector is allowed.
            _revertSelectorOnActiveRental(selector, from, to, tokenId);
        } else if (selector == e721_transfer_from_selector) {
            // Load the token ID from calldata.
            uint256 tokenId = uint256(
                _loadValueFromCalldata(data, e721_transfer_from_token_id_offset)
            );

            // Check if the selector is allowed.
            _revertSelectorOnActiveRental(selector, from, to, tokenId);
        } else if (selector == e721_approve_selector) {
            // Load the token ID from calldata.
            uint256 tokenId = uint256(
                _loadValueFromCalldata(data, e721_approve_token_id_offset)
            );

            // Check if the selector is allowed.
            _revertSelectorOnActiveRental(selector, from, to, tokenId);
        } else if (selector == e1155_safe_transfer_from_selector) {
            // Load the token ID from calldata.
            uint256 tokenId = uint256(
                _loadValueFromCalldata(data, e1155_safe_transfer_from_token_id_offset)
            );

            // Check if the selector is allowed.
            _revertSelectorOnActiveRental(selector, from, to, tokenId);
        } else if (selector == gnosis_safe_enable_module_selector) {
            // Load the extension address from calldata.
            address extension = address(
                uint160(
                    uint256(
                        _loadValueFromCalldata(data, gnosis_safe_enable_module_offset)
                    )
                )
            );

            // Check if the extension is whitelisted.
            _revertNonWhitelistedExtension(extension);
        } 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);
        } else {
            // Revert if the `setApprovalForAll` selector is specified. This selector is
            // shared between ERC721 and ERC1155 tokens.
            if (selector == shared_set_approval_for_all_selector) {
                revert Errors.GuardPolicy_UnauthorizedSelector(
                    shared_set_approval_for_all_selector
                );
            }

            // Revert if the `safeBatchTransferFrom` selector is specified. There's no
            // cheap way to check if individual items in the batch are rented out.
            // Each token ID would require a call to the storage contract to check
            // its rental status.
            if (selector == e1155_safe_batch_transfer_from_selector) {
                revert Errors.GuardPolicy_UnauthorizedSelector(
                    e1155_safe_batch_transfer_from_selector
                );
            }

            // Revert if the `setGuard` selector is specified.
            if (selector == gnosis_safe_set_guard_selector) {
                revert Errors.GuardPolicy_UnauthorizedSelector(
                    gnosis_safe_set_guard_selector
                );
            }
        }
    }

Tools Used

Manual Review

Recommended Mitigation Steps

In addition to the current conditions to revert, the Guard._checkTransaction function can be updated to also revert when the selector corresponds to setApprovalForScope(address,bytes32,bool).

Assessed type

Other

Lines of code

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Signer.sol#L218-L239 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Create.sol#L733-L775 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Signer.sol#L248-L262 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Signer.sol#L107-L116

Vulnerability details

Impact

When a lender has multiple similar pay orders that correspond to the same metadata hash and a renter is provided with a protocol signer's signature for fulfilling one of these pay orders, the renter is able to replay such signature to fulfill other pay orders of the lender as long as the renter uses the same safe and such signature is not expired. Although such signature should only allow the renter to fulfill only one of these pay orders, the renter can possibly fulfill all of these pay orders without getting new signatures from the protocol signer.

Proof of Concept

It is possible that a lender has two similar pay orders, which have the same orderType, rentDuration, and hooks. For these two orders, the following Signer._deriveOrderMetadataHash function would return the same metadata hash.

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Signer.sol#L218-L239

    function _deriveOrderMetadataHash(
        OrderMetadata memory metadata
    ) internal view returns (bytes32) {
        // Create array for hooks.
        bytes32[] memory hookHashes = new bytes32[](metadata.hooks.length);

        // Iterate over each hook.
        for (uint256 i = 0; i < metadata.hooks.length; ++i) {
            // Hash the hook
            hookHashes[i] = _deriveHookHash(metadata.hooks[i]);
        }

        // Derive and return the metadata hash as specified by EIP-712.
        return
            keccak256(
                abi.encode(
                    _ORDER_METADATA_TYPEHASH,
                    metadata.rentDuration,
                    keccak256(abi.encodePacked(hookHashes))
                )
            );
    }

In order for a renter to fulfill the lender's order, the protocol signer must provide a corresponding signature. When a renter's fulfillment can match the lender's first pay order, the protocol signer can provide a signature to allow the renter to fulfill such first pay order; in this case, when calling the following Create.validateOrder function, executing _recoverSignerFromPayload(_deriveRentPayloadHash(payload), signature) would recover the signer to be the protocol signer.

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Create.sol#L733-L775

    function validateOrder(
        ZoneParameters calldata zoneParams
    ) external override onlyRole("SEAPORT") returns (bytes4 validOrderMagicValue) {
        // Decode the signed rental zone payload from the extra data.
        (RentPayload memory payload, bytes memory signature) = abi.decode(
            zoneParams.extraData,
            (RentPayload, bytes)
        );

        // Create a payload of seaport data.
        SeaportPayload memory seaportPayload = SeaportPayload({
            orderHash: zoneParams.orderHash,
            zoneHash: zoneParams.zoneHash,
            offer: zoneParams.offer,
            consideration: zoneParams.consideration,
            totalExecutions: zoneParams.totalExecutions,
            fulfiller: zoneParams.fulfiller,
            offerer: zoneParams.offerer
        });

        // Check: The signature from the protocol signer has not expired.
        _validateProtocolSignatureExpiration(payload.expiration);

        // Check: The fulfiller is the intended fulfiller.
        _validateFulfiller(payload.intendedFulfiller, seaportPayload.fulfiller);

        // Recover the signer from the payload.
        address signer = _recoverSignerFromPayload(
            _deriveRentPayloadHash(payload),
            signature
        );

        // Check: The data matches the signature and that the protocol signer is the one that signed.
        if (!kernel.hasRole(signer, toRole("CREATE_SIGNER"))) {
            revert Errors.CreatePolicy_UnauthorizedCreatePolicySigner();
        }

        // Initiate the rental using the rental manager.
        _rentFromZone(payload, seaportPayload);

        ...
    }

Because the metadata hashes of the lender's first and second pay orders are the same, the following Signer._deriveRentPayloadHash function would return the same payload hash for the lender's first and second pay orders if payload.fulfillment, payload.expiration, and payload.intendedFulfiller are kept the same. Hence, as long as the renter uses the same safe that was used for fulfilling the lender's first pay order and the protocol signer's signature used for fulfilling such first pay order is not expired, the renter can reuse such signature without getting a new signature from the protocol. As a result, the protocol signer's signature is replayed, and the renter is able to fulfill the lender's second pay order even though such signature should only allow the renter to fulfill lender's first pay order.

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Signer.sol#L248-L262

    function _deriveRentPayloadHash(
        RentPayload memory payload
    ) internal view returns (bytes32) {
        // Derive and return the rent payload hash as specified by EIP-712.
        return
            keccak256(
                abi.encode(
                    _RENT_PAYLOAD_TYPEHASH,
                    _deriveOrderFulfillmentHash(payload.fulfillment),
                    _deriveOrderMetadataHash(payload.metadata),
                    payload.expiration,
                    payload.intendedFulfiller
                )
            );
    }

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Signer.sol#L107-L116

    function _recoverSignerFromPayload(
        bytes32 payloadHash,
        bytes memory signature
    ) internal view returns (address) {
        // Derive original EIP-712 digest using domain separator and order hash.
        bytes32 digest = _DOMAIN_SEPARATOR.toTypedDataHash(payloadHash);

        // Recover the signer address of the signature.
        return digest.recover(signature);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

When the protocol signer signs a signature for a fulfiller, a nonce for this fulfiller can be added in the signed data, where such nonce would be incremented by 1 after each signature for the corresponding fulfiller is signed. Then, in the storage, a nonce can be recorded and updated when the fulfiller fulfills an order, and the Signer._deriveRentPayloadHash function can be updated to additionally hash such nonce for the corresponding fulfiller. If the recovered signer is the protocol signer, such nonce for the corresponding fulfiller in the storage would be increased by 1.

Assessed type

Other

Lines of code

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Stop.sol#L265-L306 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Stop.sol#L166-L183 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Reclaimer.sol#L71-L101 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Reclaimer.sol#L32-L34

Vulnerability details

Impact

Calling the Reclaimer._transferERC721 function, which calls the IERC721(item.token).safeTransferFrom function, would revert when the lender is a contract that does not implement the onERC721Received function even though such lender is still EIP-721 compliant. As a result, such lender's order cannot be stopped, which causes such lender to fail to get back and thus lose the lent ERC721 tokens for such order.

Proof of Concept

When stopping an order, the following Stop.stopRent function would be called, which eventually calls the Reclaimer.reclaimRentalOrder function.

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Stop.sol#L265-L306

    function stopRent(RentalOrder calldata order) external {
        ...
        // Interaction: Transfer rentals from the renter back to lender.
        _reclaimRentedItems(order);
        ...
    }

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Stop.sol#L166-L183

    function _reclaimRentedItems(RentalOrder memory order) internal {
        // Transfer ERC721s from the renter back to lender.
        bool success = ISafe(order.rentalWallet).execTransactionFromModule(
            // Stop policy inherits the reclaimer package.
            address(this),
            // value.
            0,
            // The encoded call to the `reclaimRentalOrder` function.
            abi.encodeWithSelector(this.reclaimRentalOrder.selector, order),
            // Safe must delegate call to the stop policy so that it is the msg.sender.
            Enum.Operation.DelegateCall
        );

        // Assert that the transfer back to the lender was successful.
        if (!success) {
            revert Errors.StopPolicy_ReclaimFailed();
        }
    }

When an ERC721 token is lent for this order, calling the Reclaimer.reclaimRentalOrder function would call the Reclaimer._transferERC721 function that executes IERC721(item.token).safeTransferFrom(address(this), recipient, item.identifier).

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Reclaimer.sol#L71-L101

    function reclaimRentalOrder(RentalOrder calldata rentalOrder) external {
        ...

        // Get a count for the number of items.
        uint256 itemCount = rentalOrder.items.length;

        // Transfer each item if it is a rented asset.
        for (uint256 i = 0; i < itemCount; ++i) {
            Item memory item = rentalOrder.items[i];

            // Check if the item is an ERC721.
            if (item.itemType == ItemType.ERC721)
                _transferERC721(item, rentalOrder.lender);

            // check if the item is an ERC1155.
            if (item.itemType == ItemType.ERC1155)
                _transferERC1155(item, rentalOrder.lender);
        }
    }

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Reclaimer.sol#L32-L34

    function _transferERC721(Item memory item, address recipient) private {
        IERC721(item.token).safeTransferFrom(address(this), recipient, item.identifier);
    }

https://eips.ethereum.org/EIPS/eip-721#specification does not require all ERC721 receivers to implement the onERC721Received function and does include the transferFrom external function that does not call the onERC721Received function. Hence, if the receiver is a contract and does not implement the onERC721Received function, such receiver is still EIP-721 compliant. However, when the lender is a contract that does not implement the onERC721Received function, calling the Reclaimer._transferERC721 function, which calls the IERC721(item.token).safeTransferFrom, reverts. In this case, calling the Stop.stopRent function reverts so the order cannot be stopped. As a result, the lender cannot get back and thus loses the lent ERC721 tokens for such order.

Tools Used

Manual Review

Recommended Mitigation Steps

Since the lender must send an ERC721 token to the rental safe for creating an ERC721 rental order in the first place, such lender has the capability to receive and transfer ERC721 tokens already. Therefore, https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Reclaimer.sol#L33 can be updated to the following code.

        IERC721(item.token).transferFrom(address(this), recipient, item.identifier);

Assessed type

ERC721

Lines of code

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Reclaimer.sol#L71-L101 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Reclaimer.sol#L32-L34 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Reclaimer.sol#L42-L50

Vulnerability details

Impact

When the order has multiple ERC721/ERC1155 tokens, the admin's non-malicious action of one of these tokens can prevent the lender from receiving such token. In this case, calling the Reclaimer.reclaimRentalOrder function, which tries to send each of these tokens to the lender, reverts so such order cannot be stopped. As a result, the lender cannot receive any of these ERC721/ERC1155 tokens even though she or he should be able to receive all of them except for one.

Proof of Concept

When stopping an order, the following Reclaimer.reclaimRentalOrder function would be eventually called. If the order has multiple ERC721/ERC1155 tokens, each of these tokens should be transferred to the lender through calling the Reclaimer._transferERC721 and/or Reclaimer._transferERC1155 function(s) below.

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Reclaimer.sol#L71-L101

    function reclaimRentalOrder(RentalOrder calldata rentalOrder) external {
        ...

        // Get a count for the number of items.
        uint256 itemCount = rentalOrder.items.length;

        // Transfer each item if it is a rented asset.
        for (uint256 i = 0; i < itemCount; ++i) {
            Item memory item = rentalOrder.items[i];

            // Check if the item is an ERC721.
            if (item.itemType == ItemType.ERC721)
                _transferERC721(item, rentalOrder.lender);

            // check if the item is an ERC1155.
            if (item.itemType == ItemType.ERC1155)
                _transferERC1155(item, rentalOrder.lender);
        }
    }

It is possible that some of these ERC721/ERC1155 tokens have features like the capabilities to pause and block transfers by the tokens' admins. If the admin's non-malicious action of one of these tokens prevents the lender from receiving the corresponding token, calling the Reclaimer._transferERC721 or Reclaimer._transferERC1155 function for such token would revert, which reverts the Reclaimer.reclaimRentalOrder function call so such order cannot be stopped. Hence, although the lender could successfully receive all of these ERC721/ERC1155 tokens except for one, the lender is unable to receive any of them.

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Reclaimer.sol#L32-L34

    function _transferERC721(Item memory item, address recipient) private {
        IERC721(item.token).safeTransferFrom(address(this), recipient, item.identifier);
    }

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/packages/Reclaimer.sol#L42-L50

    function _transferERC1155(Item memory item, address recipient) private {
        IERC1155(item.token).safeTransferFrom(
            address(this),
            recipient,
            item.identifier,
            item.amount,
            ""
        );
    }

Tools Used

Manual Review

Recommended Mitigation Steps

An escrow contract can be set up to temporarily receive the lent ERC721/ERC1155 tokens when orders are stopped. Such contract should record which ERC721/ERC1155 token belongs to which lender and includes functions for allowing each lender to only claim and receive her or his own tokens. Then, the Reclaimer.reclaimRentalOrder function can be updated to transfer the lent ERC721/ERC1155 tokens to such contract.

Assessed type

ERC721

Lines of code

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/PaymentEscrow.sol#L159-L179 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/PaymentEscrow.sol#L190-L202 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/PaymentEscrow.sol#L100-L118 https://etherscan.io/address/0x43506849d7c04f9138d1a2050bbf3a0c054402dd#code#F15#L50 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Stop.sol#L265-L306 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Stop.sol#L313-L364

Vulnerability details

Impact

When the ERC20 payment token, such as USDC, implements a blocklist, the renter of a pay order can perform abusive actions to make the token's admin to block her or him. Afterwards, sending any amounts of such token to the renter reverts so stopping such pay order always reverts. As a result, such pay order can never be stopped, and the lender can never get back the lent ERC721/ERC1155 tokens.

Proof of Concept

Stopping a pay order would call the following PaymentEscrow._settlePaymentProRata or PaymentEscrow._settlePaymentInFull function, which further calls the PaymentEscrow._safeTransfer function below, depending on whether such order is expired or not.

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/PaymentEscrow.sol#L159-L179

    function _settlePaymentProRata(
        address token,
        uint256 amount,
        address lender,
        address renter,
        uint256 elapsedTime,
        uint256 totalTime
    ) internal {
        // Calculate the pro-rata payment for renter and lender.
        (uint256 renterAmount, uint256 lenderAmount) = _calculatePaymentProRata(
            amount,
            elapsedTime,
            totalTime
        );

        // Send the lender portion of the payment.
        _safeTransfer(token, lender, lenderAmount);

        // Send the renter portion of the payment.
        _safeTransfer(token, renter, renterAmount);
    }

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/PaymentEscrow.sol#L190-L202

    function _settlePaymentInFull(
        address token,
        uint256 amount,
        SettleTo settleTo,
        address lender,
        address renter
    ) internal {
        // Determine the address that this payment will settle to.
        address settleToAddress = settleTo == SettleTo.LENDER ? lender : renter;

        // Send the payment.
        _safeTransfer(token, settleToAddress, amount);
    }

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/PaymentEscrow.sol#L100-L118

    function _safeTransfer(address token, address to, uint256 value) internal {
        // Call transfer() on the token.
        (bool success, bytes memory data) = token.call(
            abi.encodeWithSelector(IERC20.transfer.selector, to, value)
        );

        ...
        if (!success || (data.length != 0 && !abi.decode(data, (bool)))) {
            revert Errors.PaymentEscrowModule_PaymentTransferFailed(token, to, value);
        }
    }

If the ERC20 payment token is a token, such as USDC, that implements a blocklist, the renter can perform abusive actions to make herself or himself to become blocked by such token's admin. Afterwards, calling the PaymentEscrow._safeTransfer function to send some of such token to the renter reverts because the renter is already blocked to receive any of such token. For example, USDC implements the following notBlacklisted modifier that would revert any token-transfer function calls if the receiver of USDC is blocked. When settling the payment through transferring a token amount to the renter reverts, stopping the pay order also reverts as shown by the Stop.stopRent and Stop.stopRentBatch functions below. As a result, such pay order cannot be stopped, and the lender can never get back the lent ERC721/ERC1155 tokens.

https://etherscan.io/address/0x43506849d7c04f9138d1a2050bbf3a0c054402dd#code#F15#L50

    modifier notBlacklisted(address _account) {
        require(
            !_isBlacklisted(_account),
            "Blacklistable: account is blacklisted"
        );
        _;
    }

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Stop.sol#L265-L306

    function stopRent(RentalOrder calldata order) external {
        ...

        // Interaction: Transfer rentals from the renter back to lender.
        _reclaimRentedItems(order);

        // Interaction: Transfer ERC20 payments from the escrow contract to the respective recipients.
        ESCRW.settlePayment(order);

        ...
    }

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/policies/Stop.sol#L313-L364

    function stopRentBatch(RentalOrder[] calldata orders) external {
        ...

        // Process each rental order.
        // Memory will become safe after this block.
        for (uint256 i = 0; i < orders.length; ++i) {
            ...

            // Interaction: Transfer rental assets from the renter back to lender.
            _reclaimRentedItems(orders[i]);

            ...
        }

        // Interaction: Transfer ERC20 payments from the escrow contract to the respective recipients.
        ESCRW.settlePaymentBatch(orders);
        ...
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Instead of directly pushing the payments to the lender and renter, the PaymentEscrow contract can be updated to record the payment amounts that the lender and renter are entitled to when stopping an order. Then, the PaymentEscrow contract can be updated to add a function for the lender and renter to pull the respective recorded amount of the payment token.

Assessed type

ERC20

Lines of code

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/PaymentEscrow.sol#L215-L283 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/PaymentEscrow.sol#L88-L91 https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/PaymentEscrow.sol#L380-L388

Vulnerability details

Impact

The PaymentEscrow.setFee function can be called to increase fee when the protocol wants to charge a higher percentage fee for orders created after such fee change. However, an order can be created before and stopped after such fee increase. This is unfair to the lender and renter of such order because the payment amounts received by them are reduced by the new higher fee and are lower than expected.

Proof of Concept

When an order is stopped, calling the following PaymentEscrow._settlePayment function would charge a percentage fee according to fee so the payment amounts received by the lender and renter of such order are reduced by the fee already.

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/PaymentEscrow.sol#L215-L283

    function _settlePayment(
        Item[] calldata items,
        OrderType orderType,
        address lender,
        address renter,
        uint256 start,
        uint256 end
    ) internal {
        // Calculate the time values.
        uint256 elapsedTime = block.timestamp - start;
        uint256 totalTime = end - start;

        // Determine whether the rental order has ended.
        bool isRentalOver = elapsedTime >= totalTime;

        // Loop through each item in the order.
        for (uint256 i = 0; i < items.length; ++i) {
            // Get the item.
            Item memory item = items[i];

            // Check that the item is a payment.
            if (item.isERC20()) {
                // Set a placeholder payment amount which can be reduced in the
                // presence of a fee.
                uint256 paymentAmount = item.amount;

                // Take a fee on the payment amount if the fee is on.
                if (fee != 0) {
                    // Calculate the new fee.
                    uint256 paymentFee = _calculateFee(paymentAmount);

                    // Adjust the payment amount by the fee.
                    paymentAmount -= paymentFee;
                }

                ...

                // 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.
                    _settlePaymentProRata(
                        item.token,
                        paymentAmount,
                        lender,
                        renter,
                        elapsedTime,
                        totalTime
                    );
                }
                // If its a PAY order and the rental is over, or, if its a BASE order.
                else if (
                    (orderType.isPayOrder() && isRentalOver) || orderType.isBaseOrder()
                ) {
                    // Interaction: a pay order or base order which has ended. Payout is in full.
                    _settlePaymentInFull(
                        item.token,
                        paymentAmount,
                        item.settleTo,
                        lender,
                        renter
                    );
                } else {
                    revert Errors.Shared_OrderTypeNotSupported(uint8(orderType));
                }
            }
        }
    }

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/PaymentEscrow.sol#L88-L91

    function _calculateFee(uint256 amount) internal view returns (uint256) {
        // Uses 10,000 as a denominator for the fee.
        return (amount * fee) / 10000;
    }

The following PaymentEscrow.setFee function can be called to increase fee when the protocol wants to charge a higher percentage fee for orders created after such fee change. If the order is stopped before such fee increase, it would be charged the previous lower fee. If the order is stopped after such fee increase, even if it has expired before such fee increase, it would be charged the new higher fee; in this case, the payment amounts received by the lender and renter of such order are reduced by the new higher fee and are lower than expected. Hence, it is unfair to the lender and renter of such order that is created before but stopped after such fee increase.

https://github.com/re-nft/smart-contracts/blob/cbf5ff74e40576be72090afd99bf6f0c366bd315/src/modules/PaymentEscrow.sol#L380-L388

    function setFee(uint256 feeNumerator) external onlyByProxy permissioned {
        // Cannot accept a fee numerator greater than 10000.
        if (feeNumerator > 10000) {
            revert Errors.PaymentEscrow_InvalidFeeNumerator();
        }

        // Set the fee.
        fee = feeNumerator;
    }

Tools Used

Manual Review

Recommended Mitigation Steps

When creating an order, the fee at that moment can be recorded for such order. When stopping such order, it can be charged a fee amount that corresponds to its recorded fee value.

Assessed type

Timing