reNFT - mussucal's results

Lines of code

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

Vulnerability details

Impact

The enabled module, if buggy or otherwise, needs to be disabled immediately else may lead to loss of assets or unintended consequences.

Proof of Concept

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

import {Actions} from "@src/Kernel.sol";
import {ISafe} from "@src/interfaces/ISafe.sol";
//
import {BaseTestWithoutEngine} from "@test/BaseTest.sol";
import {StopHarness} from "@test/mocks/harnesses/StopHarness.sol";
import {SafeUtils} from "@test/utils/GnosisSafeUtils.sol";

// Tests functionality on the stop policy related to reclaiming items from a rental wallet
contract Stop_Disable_Module_Test is BaseTestWithoutEngine {
    
    // Stop Policy Harness contract
    StopHarness public stopHarness;

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

        // set up a stop policy contract that exposes the internal functions
        stopHarness = new StopHarness(kernel);

        // impersonate the deployer which is the kernel admin
        vm.startPrank(deployer.addr);

        // enable the stop policy harness
        kernel.executeAction(Actions.ActivatePolicy, address(stopHarness));

        // enable the stop policy harness as a module. This is so the stop policy
        // harness can be added to a rental safe.
        admin.toggleWhitelistExtension(address(stopHarness), true);

        // stop impersonating
        vm.stopPrank();

    }

    function _enableStopHarnessModuleOnSafe() public {
        // create safe transaction for enabling a module
        bytes memory transaction = abi.encodeWithSelector(
            ISafe.enableModule.selector,
            address(stopHarness)
        );

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

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

        // Execute the transaction on Alice's safe
        SafeUtils.executeTransaction(
            address(alice.safe),
            address(alice.safe),
            transaction,
            transactionSignature
        );
    }
    function _disableStopHarnessModuleOnSafe() public {
        // create safe transaction for disabling a module
        bytes memory transaction = abi.encodeWithSelector(
            ISafe.disableModule.selector,
            address(0x1),address(stopHarness)
        );

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

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

        // Execute the transaction on Alice's safe
        SafeUtils.executeTransaction(
            address(alice.safe),
            address(alice.safe),
            transaction,
            transactionSignature
        );
    }
    

    function test_Bug_1() public {

        // add the module to alice's safe
        _enableStopHarnessModuleOnSafe();

        // remove the module to alice's safe
        _disableStopHarnessModuleOnSafe();

        // check after updating RentalConstants.sol and ISafe.sol
        assertEq(ISafe(address(alice.safe)).isModuleEnabled(address(stopHarness)), false);
    }
}

## Tools Used
Foundry

## Recommended Mitigation Steps
In the file RentalConstant.sol, constant gnosis_safe_disable_module_offset = 0x24. This is incorrect. According to function signature of disable_module(), the constant value should be 0x44.


## Assessed type

Context