reNFT - oakcobalt's results

Lines of code

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

Vulnerability details

Impact

Hooks can be added by Guard_Admin role through Guard.sol. And each hook can have different combination of options including hookOnStart, hookOnStop, hookOnTransaction.

When a hook previoulsy added by an Guard_Admin and then restricted or disabled by the admin due to discovered vulnerabilities or evolving project needs or external protocol risks, then any existing rental order that uses the hook will be effectively DOSsed. These rental orders cannot be stopped, and assets and erc20 payments cannot be transferred.

Proof of Concept

A rental order can use as many hooks as needed, and these hooks might perform different utilities such as additional incentives provided by an external protocol.

When rental order is created in the protocol with these hooks, each hook will be run at the start of the rental creation (validateOrder()), and also at the stop of the rental(stopRent()).

The vulnerability is in the Stop.sol - _removeHooks() function, which will revert the entire stopRent() transaction if any hook from the order is no longer supporting hookOnStop. This is done through !STORE.hookOnStop(target) check.

Suppose a popular hook was registered by Guard_Admin that provides incentives for rental, and a lot of rental orders are created with the hook. Later on, there is a vulnerability discovered in the hook which ties into a compromise of an external protocol that offers incentives. The Guard_Admin would have to disable the hook entirely due to the vulnerability though Guard.sol - updateHookStatus() and setting the hook uint8 bitmap to zero.

Although this reduces further risks posed by the hook, this will effectively DOS the stopRent() flow for any active Rental Orders that uses the hook.

When any of the rental order is due, stopRent() can be called to transfer assets, due to the Rental Order is created with the hook, all of the hooks within the rental order needs to be passed through _removeHooks(). This means that , the disabled hook will fail the STORE.hookOnStop(target) check and directly revert the entire transaction. No assets can be transferred.

//src/policies/Stop.sol
    function stopRent(RentalOrder calldata order) external {
...
        if (order.hooks.length > 0) {
            //@audit all the order hooks will be passed to _removeHooks()
|>          _removeHooks(order.hooks, order.items, order.rentalWallet);
        }
    }


    function _removeHooks(
        Hook[] calldata hooks,
        Item[] calldata rentalItems,
        address rentalWallet
    ) internal {
...
        for (uint256 i = 0; i < hooks.length; ++i) {
...
            //@audit this reverts the entire stopRent() flow DOS funds transfer, if a single hook is subsequently disabled for hookOnStop()
            if (!STORE.hookOnStop(target)) {
|>              revert Errors.Shared_DisabledHook(target);
            }
...

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

Tools Used

Manual Review

Recommended Mitigation Steps

In _removeHooks(), instead of reverting if a hook is not support on hookOnStop(), simply skip that iteration, and continue to the next hook.

Assessed type

Other

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/Kernel.sol#L285 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/Kernel.sol#L397-L403

Vulnerability details

Impact

There are two ways to upgrade module contracts (such as PaymentEscrow.sol and Storage.sol). One is to upgrade only the implementation contracts through Admin.sol following UUPS standards, the other way is to upgrade both the proxy and implementation on Kernel.sol through executeAction() -> Actions.UpgradeModule.

The latter will immediately cause protocol-wide DOS on key functions such as stopRent() / stopRentBatch() and freezing protocol funds.

Proof of Concept

Kernel.sol - executeAction() -> Actions.UpgradeModule directly change the module address in Kernel storage and will also update the module address stored in all dependent policy contracts (Admin.sol, Create.sol, Stop.sol, etc)

The problem is it doesn't matter at what circumstance _upgradeModule() is called, it will always DOS key protocol functions and cause protocol funds to freeze, making this functionality always dangerous to the protocol and self-destructing.

There are two vulnerabilities in _upgradeModule(): (1) _upgradeModule() ignores any storage variable values in the old module contract, and will do nothing to check if there are critical storage values stored in the old contract before directly erasing the contract address in Kernel; (2) _upgradeModule() ignores any protocol funds that might be in the old module contract, and will do nothing to check or transfer funds before directly erasing the old contract address in Kernel.

In addition, current module contracts such as PaymentEscrow.sol and Storage.sol have no methods to transfer existing funds and key storage values to a new contract. These in combination will immediately result in a fund freeze and DOS.

//src/Kernel.sol
    function executeAction(Actions action_, address target_) external onlyExecutor {
...
        } else if (action_ == Actions.UpgradeModule) {
            ensureContract(target_);
            ensureValidKeycode(Module(target_).KEYCODE());
            //@audit UpgradeModule performs no checks on existing funds or critical storage values in the old module, _upgradeModule will directly erase old module address
|>          _upgradeModule(Module(target_));
...

(https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/Kernel.sol#L282-L285)

//src/Kernel.sol
    function _upgradeModule(Module newModule_) internal {
        // The old module no longer points to the keycode.
        //@audit this directly erase the old module address
|>      getKeycodeForModule[oldModule] = Keycode.wrap(bytes5(0));

        // The new module points to the keycode.
        //@audit this directly adds the new module address
|>      getKeycodeForModule[newModule_] = keycode;

        // The keycode points to the new module.
        //@audit this directly adds the new module address
|>      getModuleForKeycode[keycode] = newModule_;
...
        _reconfigurePolicies(keycode);
    }

    function _reconfigurePolicies(Keycode keycode_) internal {
        // Get an array of all policies that depend on the keycode.
        Policy[] memory dependents = moduleDependents[keycode_];
        uint256 depLength = dependents.length;

        // Loop through each policy.
        for (uint256 i; i < depLength; ++i) {
            // Reconfigure its dependencies.
            //@audit this directly changes the module address in all dependent policy contracts
 |>         dependents[i].configureDependencies();
        }
    }

(https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/Kernel.sol#L397-L403)

Note that although executeAction() is an onlyExecutor function, the upgrade module action will always immediately cause damage to the protocol without accomplishing the upgrade in a way that sustains basic protocol functions. And current PaymentEscrow.sol and Storage.sol don't have any functions that supports protocol funds transfer or key rental data transfer to a new contract.

As an example of DOS and fund freeze, the Stop policy's stopRent() and stopRentBatch() will fail following _upgradeModule, because the new Storage.sol contract will not have the rental order hash in storage, and the new PaymentEscrow.sol will not have ERC20 funds for payout.

Tools Used

Manual Review

Recommended Mitigation Steps

In executeAction(), consider disabling the Action.UpgradeModule and only allow UUPS upgrade to module contracts. Alternatively, extra checks need to be added in _upgradeModule to ensure there will no funds freeze or DOS, which also requires that PaymentEscrow.sol and Storage.sol to be re-designed to be compatible with Action.UpgradeModule.

Assessed type

Upgradable

Lines of code

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

Vulnerability details

Impact

Some ERC-721 projects do implement external burn methods that allow token owners to burn their own NFTs. Note that this doesn't violate ERC-721 standard, and neither is it considered malicious.

When the offered assets of an order contains an ERC-721/ERC-1155 token that has an external burn function that allows token owner to burn their own NFTs, the NFT that is supposed to be locked in the rental safe can be burned before rental order expires, causing the lender(offerer) losing their NFT permanently.

In addition, when the rental order is BaseOrder type, the lender(offerer) will also lose their ERC20 payment permanently.

Proof of Concept

Guard.sol is a policy contract that is set as a guard for all rental safe contract(Safe.sol) deployed. Guard.sol will run checkTransactions() for any execution from the order fulfiller's rental safe.

When a rental order is created in reNFT, the offer assets (ERC721/ERC1155) will be transferred and 'locked' in the fulfiller(renter)'s rental safe contract until rental is stopped by lender or expires. This locking mechanism is enforced through Guard.sol by checking the function signatures to be executed from safe and reverting when sensitive function signatures such as e721_safe_transfer_from_1_selector,e721_safe_transfer_from_2_selector, etc are part of the calldata.

However, the vulnerability is that Guard.sol's checkTransactions() only check for transfer related function signatures but doesn't check for burn related signatures. This allows any ERC721/ERC1155 that implements external burn methods to be burned freely from the rental safe contract.

Note that even through READ_ME excluded Dishonest ERC721/ERC1155 Implementations that adds an additional function to transfer the token to another wallet. An ERC721 that implements an external burn function to allow token owner to burn their own NFT is not considered dishonest, and neither does it violate ERC721 spec. Therefore, Guard.sol should consider including checks for burn related token methods to ensure that NFT's locked and protected in fulfiller's safe contract.

//src/policies/Guard.sol
    function _checkTransaction(address from, address to, bytes memory data) private view {
...
     if (selector == e721_safe_transfer_from_1_selector) {
...
            // Check if the selector is allowed.
            _revertSelectorOnActiveRental(selector, from, to, tokenId);
    } else if (selector == e721_safe_transfer_from_2_selector) {
...
            _revertSelectorOnActiveRental(selector, from, to, tokenId);
        } else if (selector == e721_transfer_from_selector) {
...
            _revertSelectorOnActiveRental(selector, from, to, tokenId);
        } else if (selector == e721_approve_selector) {
...
            _revertSelectorOnActiveRental(selector, from, to, tokenId);
...
        } else {
...
           //@audit There is no checking whether the selector is burn or burnBatch
           if (selector == shared_set_approval_for_all_selector) {
                revert Errors.GuardPolicy_UnauthorizedSelector(
...
            if (selector == e1155_safe_batch_transfer_from_selector) {
                revert Errors.GuardPolicy_UnauthorizedSelector(
...
            if (selector == gnosis_safe_set_guard_selector) {
                revert Errors.GuardPolicy_UnauthorizedSelector(

(https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L270-L287)

As seen above, Guard.sol's _checkTransaction() allows it when a rental safe is executing a transaction that either calls burn() on an ERC721 or ERC1155 that allows the token owner to burn their token. The fulfiller who's the owner of the rental safe can execute transactions from the safe to burn their ERC721/ERC1155 asset. This way, when the rental order is stopped by the lender, the NFT will not be transferred back to the lender.

Suppose that the rentalOrder is BaseOrder type. A base order can only be stopped when the rental duration passes, when the lender call Stop.sol - stopRent() to stop rental, two transfers are supposed to take place: (1) the offered assets (ERC721/ERC1155) 'locked' in the fulfiller's rental safe will be transferred to the lender; (2) the ERC20 payments sent by the fulfiller at order creation and now stored in PaymentEscrow proxy will be transferred to the lender as well.

When the fulfiller already burned the ERC721 asset in their rental safe when stopRent() is called. (1) will revert. Due to the rental safe will make a delegate call to Recalimer.sol - reclaimRentalOder() which will call IERC721(item.token).safeTransferFrom(address(this), recipient, item.identifier). And this will revert due to item.identifier (tokenId) has already been burned by the fulfiller.

In addition, (2) will also revert, which causes the lender losing both their NFT asset and their ERC20 payments from the fulfiller.

Note that the fulfiller can choose to burn the lender's NFT at or close to the rental expiration timestamp, such that fulfiller will get the full value of rented NFT.

//src/packages/Reclaimer.sol
    function reclaimRentalOrder(RentalOrder calldata rentalOrder) external {
...
//@audit-info note: this is a delegate call, _transferERC721() will transfer ERC721 out of safe contract back to the lender
            if (item.itemType == ItemType.ERC721)
                _transferERC721(item, rentalOrder.lender);
...
}
    function _transferERC721(Item memory item, address recipient) private {
//@audit when the fulfiller's safe contract already burned the token, this will revert, and DOS Stop.sol - stopRent() flow, this will also revert settlePayment flow from the PaymentEscrow
|>      IERC721(item.token).safeTransferFrom(address(this), recipient, item.identifier);
    }

(https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Reclaimer.sol#L95)

Tools Used

Manual Review

Recommended Mitigation Steps

In Guard.sol, _checkTransaction() also add additional check for burn() selectors to prevent malicious asset token burning from the fulfiller.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Policy contracts such as Guard.sol can be upgraded when needed. However, there is a vulnerability in existing implementation of Guard policy, that allows a rental safe contract to use the outdated Guard.sol for an unlimited amount of time.

This compromises the security of the rental wallet because unsafe transactions that are checked and reverted in the new guard policy contract can be directly bypassed and permitted in rental safe contract using an old guard policy.

Proof of Concept

Policy contracts can be upgraded by protocol executor on Kernel.sol through executeAction() -> _activatePolicy() -> _deactivePolicy(). When Guard.sol needs to be upgraded, the intended flow is a) _activatePolicy(newGuard) b) _deactivePolicy(oldGuard), c) admin will whitelist a temporary migration contract which allows any rental safe wallet to make a delegate call to migration contract to upgrade the guard address in storage. This flow can also be confirmed in test: test/integration/upgradeability/GuardPolicyUpgrade.t.sol.

Note in this intended flow, the rental safe contract owner needs to initiate a delegate call to the whitelisted migration contract to update the pointer to the new guard address.

However, the issue is (1) there is no enforcement that an existing rental safe contract owner will call the migration contract to upgrade their guard address; (2) even if the safe owner doesn't call to update their guard address, the rental safe will be able to continue to use the old guard contract to execute transactions, bypassing any new restrictions that might be enforced in the new guard.

This is because current Guard.sol's key user flows that involve _checkTransactions(), _forwardToHook(),_revertNonWhiteListedExtensions() won't check if the existing Guard.sol address stored in the safe contract is still active, and these flows will only call Storage.sol's view functions or public variables that have no access control to revert when the caller Guard.sol is no longer active in Kernel.

As a result, even though the active Guard.sol in Kernel.sol is a different address and the old guard doesn't exist in Kernel's storage, all the rental safe flows can still use the old guard to check transactions.

//src/policies/Guard.sol
    function checkTransaction(
        address to,
        uint256 value,
        bytes memory data,
        Enum.Operation operation,
        uint256,
        uint256,
        uint256,
        address,
        address payable,
        bytes memory,
        address
    ) external override {
        // Disallow transactions that use delegate call, unless explicitly
        // permitted by the protocol.
|>      //@audit There are no checks to see if address(this) (guard) is still active in Kernel.sol
        //@audit note: when Guard.sol is no longer active, STORE.whitelistedDelegates(to) will still pass, because it's a view funciton with no access control
|>      if (operation == Enum.Operation.DelegateCall && !STORE.whitelistedDelegates(to)) {
            revert Errors.GuardPolicy_UnauthorizedDelegateCall(to);
        }
        // Require that a function selector exists.
        if (data.length < 4) {
            revert Errors.GuardPolicy_FunctionSelectorRequired();
        }
        // Fetch the hook to interact with for this transaction.
        //@audit note: when Guard.sol is no longer active, STORE.contractToHook(to) and STORE.hookOnTransaction(hook) because they are view functions with no access control
|>      address hook = STORE.contractToHook(to);
|>      bool isActive = STORE.hookOnTransaction(hook);
        // If a hook exists and is enabled, forward the control flow to the hook.
        if (hook != address(0) && isActive) {
            _forwardToHook(hook, msg.sender, to, value, data);
        }
        // If no hook exists, use basic tx check.
        else {
            _checkTransaction(msg.sender, to, data);
        }
    }

(https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L324-L331)

//src/policies/Guard.sol
    function _checkTransaction(address from, address to, bytes memory data) private view {
...
            //@audit when Guard.sol is no longer active in Kernel, _revertSelectorOnActiveRental() will still work because it only calls view functions with no access control.
|>          _revertSelectorOnActiveRental(selector, from, to, tokenId);
...
            //@audit when Guard.sol is no longer active in Kernel, _revertNonWhitelistedExtension() will still work because it only calls view function with no access control
|>          _revertNonWhitelistedExtension(extension);
    }

    function _revertSelectorOnActiveRental(
        bytes4 selector,
        address safe,
        address token,
        uint256 tokenId
    ) private view {
        // Check if the selector is allowed.
        //@audit when Guard.sol is no longer active in Kernel, STORE.isRentedOut() will still work because it only calls view function with no access control
|>      if (STORE.isRentedOut(safe, token, tokenId)) {
            revert Errors.GuardPolicy_UnauthorizedSelector(selector);
        }
    }

    function _revertNonWhitelistedExtension(address extension) private view {
        // Check if the extension is whitelisted.
        //@audit when Guard.sol is no longer active in Kernel, STORE.whitelistedExtensions() will still work because it only calls view function with no access control
|>       if (!STORE.whitelistedExtensions(extension)) {
            revert Errors.GuardPolicy_UnauthorizedExtension(extension);
        }
    }

(https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L133) (https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L145)

In addition, this creates a condition where the behaviors of different rental safe contracts can be uneven, depending on whether a specific rental safe is using a new guard or old guard, they might have different levels of restrictions in executions, which undermines the fairness in the rental process. For example, some new function selectors can be prohibited from execution in the new guard based on the protocol needs, but these selectors will continue to be allowed in rental safes that use the old guard.

For comparison, a rental safe using an old Stop policy (Stop.sol) will cause revert at stopRent() or stopRentBatch() flow due to failing permissioned function sig checks, which will force safe owners to migrate to the new Stop.sol. But an old Guard policy allows safe owners to continue using an outdated checkTransaction() logics, which I consider it high severity.

Tools Used

Manual Review

Recommended Mitigation Steps

In checkTransaction() , call Kernal.sol to check whether address(this) ( Guard.sol that the safe is pointing to) is still active. This can be done by calling uint256 index = kernel.getPolicyIndex(address(this)) and then check if the index corresponds to address(this) in kernel.activePolicies.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

If the fulfiller is USDC blacklisted and the ERC20 payment is in USDC, the offerer cannot get their NFT back

Proof of Concept

In Stop.sol, stopRent() will handle transfer both ERC20 payment and assets(ERC721/ERC1155) when a rental order expires.

The vulnerability is that when the fulfiller is USDC blacklisted and if the ERC20 token in question is USDC, the offerer will not abe able to get their asset token back, due to the fact that ECRW.settlePayment(order) which handles ERC20 payment will revert during token transfer, and reverting the entire stopRent() transaction.

//src/policies/Stop.sol
    function stopRent(RentalOrder calldata order) external {
...
|>       ESCRW.settlePayment(order);
...

(https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L296)

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding try-catch black inside the ESCRW.settlePayment() token transfer flow when the erc20 is USDC or other ERC20 with blacklisting features.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Protocol admin can whitelist extensions which will allow all rental safe contract (Safe.sol) to enable the whitelisted extension as a module (Safe.sol - 'enableModule()'). An enabled module in the safe contract can directly execute transactions bypassing multi-sig votes or other checks by Guard.sol.

A problem will occur when a whitelisted extension is later de-listed by the protocol admin. If the extension is delisted due to a vulnerability, the rental safe contract cannot disable the module and will be exposed to the same risks of from the vulnerable extension.

Proof of Concept

In Gaurd.sol - checkTransactions(), enableModule(extension) cannot be executed by the safe contract(Safe.sol) unless the extension is whitelisted by the admin. An extension is a contract that can be added to a rental safe as a module. In checkTransactions(), the whitelisted extension check is done through _revertNonWhitelistedExtension(extension) which checks STORE.whitelistedExtensions(extension) in Storage.sol.

Suppose a rental safe enabled the whitelisted extension as a module (the safe contract execTransaction() to call enableModule(extension) to self). This means the whitelisted extension can execute any transactions directly from the rental safe bypassing multi-sig signatures and Guard.sol.(Note: In Safe.sol, this is done through execTransactionFromModule() from ModuleManager.sol which bypasses the typical execTransaction() flow)

However, if this whitelisted extension is then de-listed by the protocol admin (the admin calls toggleWhitelistExtensioin() on Admin.sol) due to a vulnerability has been detected in this extension, then STORE.whitelistedExtensions(extension) will return false.

Now since the whitelisted extension has been delisted in the protocol, to avoid any exposure to the vulnerable extension, the rental safe needs to disable the extension as a module immediately.

However, the rental safe will not be able to disable this module. The issue is in Guard.sol-checkTransactions(), disableModule(extension) function signature is also checked against _revertNonWhitelistedExtension(extension). And because the whitelisted extension is already delisted in the protocol, STORE.whitelistedExtensions(extension) returns false which will revert the entire transaction. As a result, a rental safe that had a previously whitelisted extension enabled will not be able to disable it.

POC:

• Protocol Admin whitelist an extensionA. (Admin.sol - toggleWhitelistExtension(extensionA, true))
• Rental safe enable extensionA as a module. The owner of rental safe (fulfiller) calls exeTransaction() by passing enableModule(extensionA) as calldata.
• A vulnerability is detected in extensionA. Protocol admin delists extensionA. (Admin.sol - toggleWhitelistExtension(extensionA, false))
• Rental safe owner tries to disable extensionA to avoid exposure to the vulnerable extension. The owner of rental safe (fulfiller) calls exeTransaction() by passing disableModule(extensionA) as calldata.
• Rental safe owner's transaction reverted, the vulnerable extensionA cannot be disabled on the rental safe and can execute transactions bypassing Guard.sol and multi-sig signatures.

//src/policies/Guard.sol
    function _checkTransaction(address from, address to, bytes memory data) private view {
...
        } else if (selector == gnosis_safe_enable_module_selector) {
...
            _revertNonWhitelistedExtension(extension);
        } else if (selector == gnosis_safe_disable_module_selector) {
...
            //@audit A subsequently disabled extension cannot be disabled in safe
|>           _revertNonWhitelistedExtension(extension);
...}


    function _revertNonWhitelistedExtension(address extension) private view {
        // Check if the extension is whitelisted.
        //@audit when an admin later disabled the extension, STORE.whitelistedExtensions(extension) will return false, which will revert disableModule flow
|>      if (!STORE.whitelistedExtensions(extension)) {
            revert Errors.GuardPolicy_UnauthorizedExtension(extension);
        }
    }

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

Tools Used

Manual Review

Recommended Mitigation Steps

In Guard.sol - _checkTransactions() - else if (selector == gnosis_safe_disable_module_selector) {, consider only reverting when mandatory extensions are being disabled. Such mandatory extensions currently include Stop policy (Stop.sol). Because enableModule() would already ensure that only whitelisted modules can be enabled in the safe, we only need to ensure that mandatory policies cannot be disabled.

Assessed type

Other

Low-01: Consider adding a realistic cap for protocol fees, current fee percentage can be set to 100%

The protocol fee is a percentage of the total ERC20 payment for a rental order. The fee will be deducted from ERC20 payment amount at the time of settlement of a rental order.

The problem is the fee percentage can be set to 100% based on current implementation, which means the offerer or fulfiller will get zero ERC20 payment at the end of a rental order.

//src/modules/PaymentEscrow.sol
    function setFee(uint256 feeNumerator) external onlyByProxy permissioned {
        // Cannot accept a fee numerator greater than 10000.
        //@audit fee can be set to 100%
|>      if (feeNumerator > 10000) {
            revert Errors.PaymentEscrow_InvalidFeeNumerator();
        }
        // Set the fee.
        fee = feeNumerator;
    }

    function _settlePayment(
        Item[] calldata items,
        OrderType orderType,
        address lender,
        address renter,
        uint256 start,
        uint256 end
    ) internal {
...
                if (fee != 0) {
                    // Calculate the new fee.
                    uint256 paymentFee = _calculateFee(paymentAmount);

                    // Adjust the payment amount by the fee.
                    //@audit when fee is set to 100%, paymentAmount will be 0
|>                  paymentAmount -= paymentFee;
                }

(https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L382)

Recommendations: Consider adding a realistic cap percentage variable.

Low-02: Accumulator.sol can be a library contract to reduce the inheritance chain.

Accumulator.sol is currently an abstract contract that needs to be inherited. But Accumulator.sol has no virtual functions to be overridden, and it only contains pure functions.

Accumulator.sol contains _insert() and _converToStatic(), which can be considered utility functions that manage dynamic bytes arrays. Also, _insert() and _converToStatic() are not virtual and cannot be overridden.

In addition, Accumulator.sol's utility functions are also reused in multiple contracts such as Create.sol and Stop.sol.

All of the above suggests that it's better to convert Accumulator.sol into a library contract to reduce contract inheritance chains and also for code reuse.

//src/packages/Accumulator.sol
    function _insert(
        bytes memory rentalAssets,
        RentalId rentalId,
        uint256 rentalAssetAmount
    ) internal pure {
...

    function _convertToStatic(
        bytes memory rentalAssetUpdates
    ) internal pure returns (RentalAssetUpdate[] memory updates) {
...

(https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Accumulator.sol#L32-L36) (https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Accumulator.sol#L96-L98)

Recommendations: Consider converting Accumulator.sol as a library contract.

Low-03: A hook that is only enabled to run on start, will DOS the stopRent() flow

In the protocol, a rental order can have as many hooks as needed, and the entire list of hooks of a rental order will be run both at rental order creation (Create.sol - validateOrder()) and at rental stopping (Stop.sol - stopRent()).

And a hook can be authorized to run at different phases throughout a rental order. For example, hookOnStart, hookOnTransaction and hookOnStop. Each hook that is registered in Storage.sol by the protocol admin, might have a different combination of these options enabled through their bitmap value(uint8 enabled).

A problem will occur if a hook is only intended to run at the start of a rental order creation. There can be two scenarios: (1) The hook contract implements onStart(), onTransaction(), and onStop() interface but only has an implementation for onStart(). And the protocol admin only enabled hookOnStart in Storage.sol; (2) The hook only implements onStart() methods, and do not have onTransation() or onStop() methods.

In both scenarios, validateOrder() will pass and the rental order will be created. However, they will revert when stopRent() is called at the end of the rental order. In (1), revert happens at Stop.sol - _removeHooks()- if (!STORE.hookOnStop(target)) checks, where the hook's bitmap is not turned on for hookOnStop(), causing stopRent() reverts; In (2), the revert happens at Stop.sol - _removeHooks() - try IHook(target).onStop(...) {} catch ...{revert. In _removeHooks(), even though try-catch block is used, any error during the call to onStop() will explicitly revert the entire transaction.

//src/policies/Stop.sol

    function stopRent(RentalOrder calldata order) external {
...
        if (order.hooks.length > 0) {
            //@audit A rentalOrder hook has to be enabled for both hookOnStart and hookOnStop, otherwise the rental cannot be stopped.
|>          _removeHooks(order.hooks, order.items, order.rentalWallet);
        }

  }

    function _removeHooks(
        Hook[] calldata hooks,
        Item[] calldata rentalItems,
        address rentalWallet
    ) internal {
...
        for (uint256 i = 0; i < hooks.length; ++i) {
...
            //@audit if a hook is only enabled for `hookOnStart()`, this will revert the `stopRent()` flow
|>          if (!STORE.hookOnStop(target)) {
                revert Errors.Shared_DisabledHook(target);
            }
...
            try
                IHook(target).onStop(
                    rentalWallet,
                    item.token,
                    item.identifier,
                    item.amount,
                    hooks[i].extraData
                )
            {} catch Error(string memory revertReason) {
                // Revert with reason given.
                revert Errors.Shared_HookFailString(revertReason);

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

As seen above, even if a hook is only intended to run on a rental creation, not on rental stop, the hook bitmap stored in mapping(address hook => uint8 enabled) public hookStatus has to be enabled for both hookOnStart() and hookOnStop() for it be used in a rental order. In any case if a hook is not enabled for hookOnStop(), a rental order cannot be stopped, and the lent assets cannot be returned to lender, and the erc20 payment will be freezed in PaymentEscrow.sol as well.

Recommendations: In _removeHooks(), consider only try-catch call IHook(target).onStop() when STORE.hookOnStop(target) and if !STORE.hookOnStop(target), simply skip the iteration instead of reverting.

Low-04: constructor(Kernel kernel_) has no effects when PaymentEscrow.sol is used as a logic contract.

PaymentEscrow.sol and Storage.sol are both intended be implementation contracts (not proxy contracts) because they both inherit Proxiable.sol, a contract that contains upgrade logic for implementation contracts based on UUPS standard.

In this case, storing kernel address in a logic contract will have no effect than actual flow, because kernel will be stored and fetched from the proxy contract. This can also be confirmed from test (test/integration/upgradeability/StorageModuleUpgrade.t.sol) where a new storage contract will need to be deployed with address(0) as a constructor argument. (1)

//src/modules/Storage.sol
    constructor(Kernel kernel_) Module(kernel_) {}

(https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/Storage.sol#L79) (2)

//src/modules/PaymentEscrow.sol
    constructor(Kernel kernel_) Module(kernel_) {}

(https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L51)

Recommendations: Consider removing the constructor.

Low-05: The old and inactive Stop.sol policy contract cannot be disabled in the safe contract (rental wallet), even though it is no longer an active policy contract in Kernel.sol

Stop.sol policy contract is enabled upon initialization of a rental safe contract through a delegate call to enableModule() and has authority to transfer asset tokens ERC721/ERC1155 out of the rental safe contract.

However, when there is an upgrade to the stop policy,i.e. a new Stop.sol address is activated in Kernal and the old Stop.sol address is de-activated in Kernel. The old stop.sol policy contract will remain as valid module for the rental safe contract. The rental safe contract cannot disable the old stop.sol address by calling disableModule(), because Guard.sol will check any transactions from the safe and will revert when disableModule(extension) is called and the extension is not a whitelisted extension. And because Stop.sol is never intended to be a whitelisted extension (it will be enabled at initialization by default for any safe), the safe cannot disable the old Stop.sol.

Although under current implementation of Stop.sol, the old Stop.sol will not be able to execute stopRent() or stopRentBatch() due to the permission access checks on Storage.sol and PaymentEscrow.sol will fail and revert a call from a de-activated Stop.sol, still it's safer to disable an outdated module contract in Safe contracts.

//src/policies/Guard.sol

    function _checkTransaction(address from, address to, bytes memory data) private view {
        bytes4 selector;
...
        } 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.
            //@audit the safe contract cannot disable an outdated Stop.sol, due to failing `_revertNonWhitelistedExtension(extension)`. Stop.sol doesn't have to be whitelisted and will cause revert in the disable module flow.
|>          _revertNonWhitelistedExtension(extension);

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

Recommendations: Consider adding a bypass check in _checkTransactions() -> else if (selector == gnosis_safe_disable_module_selector) {, to check if the extension to be disabled is an active policy. And this can be done by querying kernel.getPolicyIndex() and kernel.activePolicies().

If the extension is not an active policy, consider allowing disabling the module. Note that this logic also removes the _revertNonWhitelistedExtension() check and assumes that as long as a module is enabled in a safe contract, the module is either a policy (stop.sol) that is enabled by default or it used to be a whitelisted module when enabled (this is already checked in enableModule bypass). if this is the case, then we only need to ensure it is not an active policy to allow disable it from a safe.

Low-06: Factory.sol might deploy a safe with old stopPolicy and guardPolicy contract addresses because it assumes stopPolicy and guardPolicy addresses will never change by storing their address as immutable.

In Factory.sol, the stop policy contract and the guard policy contract are stored as immutable variables. This assumes these two contract addresses will never change.

//src/policies/Factory.sol
    //@audit This assumes policy contract never changes, when policy contract change, there are risk of deploying safe with old policies and also factory has to be re-deployed to keep up with other policy contract update
|>  Stop public immutable stopPolicy;
|>  Guard public immutable guardPolicy;

(https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Factory.sol#L31-L32)

However, both stop policy (Stop.sol) and guard policy (Guard.sol) can be upgraded by deploying new stop and guard contracts and deactivate the old addresses from Kernel.sol. The policy upgrade flow is: Kernel.sol - executeAction()->_activatePolicy(newPolicy)->_deactivePolicy(oldPolicy).

When policy contracts are upgraded, their address changes. In this case, Factory.sol will still deploy a safe with the old policy contract addresses. This opens up potential for DOS or other vulnerabilities that the outdated policy contracts may pose to a rental safe contract. Even though the new safe contract can also migrate to the new policy contracts, the owner might not do so or is not aware because deploying safe is a permissionless process.

To avoid the risks above, the admin would have to deploy new Factory.sol every time there is upgrade to Stop.sol or Guard.sol, and also ensure that Factory upgrade is done in the same transaction as Stop.sol and Guard.sol, which is cumbersome and wasteful.

Recommendations: In Factory.sol, do not use immutable variables to hold Stop.sol and Guard.sol addresses. Instead, consider adding an onlyKernel function to allow update of Stop and Guard addresses stored in Factory.sol.