Failure-to-stop-and-settle-rental-if-genosis-safe-wallet-is-v1.50-or-later

[c4-bot-4]

Lines of code

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

Vulnerability details

M-Failure-to-stop-and-settle-rental-if-genosis-safe-wallet-is-v1.50-or-later

Proof of concept

this report is inspired by

Reference to c4-submissions report

after the rental expires, anyone can call stopRent

function stopRent(RentalOrder calldata order) external {
        // Check that the rental can be stopped.
        _validateRentalCanBeStoped(order.orderType, order.endTimestamp, order.lender);

        // Create an accumulator which will hold all of the rental asset updates, consisting of IDs and
        // the rented amount. From this point on, new memory cannot be safely allocated until the
        // accumulator no longer needs to include elements.
        bytes memory rentalAssetUpdates = new bytes(0);

        // Check if each item in the order is a rental. If so, then generate the rental asset update.
        // Memory will become safe again after this block.
        for (uint256 i; i < order.items.length; ++i) {
            if (order.items[i].isRental()) {
                // Insert the rental asset update into the dynamic array.
                _insert(
                    rentalAssetUpdates,
                    order.items[i].toRentalId(order.rentalWallet),
                    order.items[i].amount
                );
            }
        }

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

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

the first step is transfer the NFT back. Line of code

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

and this is calling _reclaimRentedItems

function _reclaimRentedItems(RentalOrder memory order) internal {
        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();
        }
    }

and this is calling execTransactionFromModule

   function execTransactionFromModule(
        address to,
        uint256 value,
        bytes memory data,
        Enum.Operation operation
    ) public virtual returns (bool success) {
        // Only whitelisted modules are allowed.
        require(msg.sender != SENTINEL_MODULES && modules[msg.sender] != address(0), "GS104");
        // Execute transaction without further confirmations.
        address guard = getGuard();

        bytes32 guardHash;
        if (guard != address(0)) {
            guardHash = Guard(guard).checkModuleTransaction(to, value, data, operation, msg.sender);
        }
        success = execute(to, value, data, operation, type(uint256).max);

        if (guard != address(0)) {
            Guard(guard).checkAfterExecution(guardHash, success);
        }
        if (success) emit ExecutionFromModuleSuccess(msg.sender);
        else emit ExecutionFromModuleFailure(msg.sender);
    }

if the safe wallet is create via safe wallet proxy v1.5.0+,

the guard is already set when the safe wallet is deployed

so the code would trigger the check checkModuleTransaction

if (guard != address(0)) {
            guardHash = Guard(guard).checkModuleTransaction(to, value, data, operation, msg.sender);
        }

Impact

but in the guard contract, there is no such function (checkModuleTransaction) implemented

Reference

so Guard(guard).checkModuleTransaction will revert

and execTransactionFromModule revert and stopRent revert

the guard smart contract is not upgradeable,

and in the current guard function, the owner cannot set a new guard because of the check always make the set guard revert

Line of code

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

so the impact is if the safe wallet v.1.5.0 is used, no one can call stopRent to settle the rental, and lender lose their nft and lender or renter lose the rental payment.

why protocol support safe wallet v.1.5.0?

Reference on deployed blockchain

We are going to launch on: Ethereum Mainnet, Polygon and Avalanche to begin with. We will then look 
to expand to all the chains that are supported by Safe.

the protocol does not mention which safe wallet will the code support

the dependency for safe wallet does not lock the version to v.1.4 as well, it is whatever the code in the safe master contract

so once the safe wallet v.1.5.0 PR is merged and the safe wallet v.1.5.0 is released, it is possible that the code support safe v.1.5.0

the relevant safe wallet PR is here: https://github.com/safe-global/safe-contracts/pull/571

Recommendation

only allow safe owner to whitelist the guard contract that is whitelisted or make the guard contract upgradeable.

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-sponsor]

Alec1017 (sponsor) confirmed

[Alec1017]

As a mitigation for this, we will not deploy with a version that uses checkModuleTransaction, we will use v1.3

[0xean]

I believe this to be QA as it is about future versions and future proofing vs current vulnerabilities.

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-b

[c4-judge]

0xean marked the issue as grade-a