Incorrect `gnosis_safe_disable_module_offset` constant leads to removing the rental safe's `module` without verification

criticalCode4rena

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalConstants.sol#L58 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalConstants.sol#L62 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/interfaces/ISafe.sol#L98-L101 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L255 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L260 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L266 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/test/unit/Guard/CheckTransaction.t.sol#L369 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/test/unit/Guard/CheckTransaction.t.sol#L557

Vulnerability details

The gnosis_safe_disable_module_offset constant was incorrectly specified to point at an incorrect function parameter of the disableModule(address prevModule, address module).

Specifically, the offset constant will point at the prevModule (1st param) instead of the module (2nd param).

Impact

When a safe transaction initiated from a rental safe containing a call to the safe's disableModule() is invoked, the Guard::checkTransaction() cannot verify the module expected to be removed.

If the prevModule was a non-whitelisted extension, the safe transaction will be reverted.

However, if the prevModule was a whitelisted extension, the module will be removed without verification. Removing the rental safe's module without verification can lead to other issues or attacks since the removed module can be a critical component (e.g., removing the protocol's Stop policy contract).

Proof of Concept

The snippet below presents some of the Gnosis Safe configurations of the reNFT protocol. The gnosis_safe_disable_module_selector constant contains the function selector (0xe009cfde) of the rental safe's ModuleManager::disableModule(address prevModule, address module).

Meanwhile, the gnosis_safe_disable_module_offset constant contains a memory offset (0x24) intended to point at the module param of the disableModule().

solidity
    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalConstants.sol

    /////////////////////////////////////////////////////////////////////////////////
    //                  Gnosis Safe Function Selectors And Offsets                 //
    /////////////////////////////////////////////////////////////////////////////////

    ...

    // bytes4(keccak256("disableModule(address,address)"));
@1  bytes4 constant gnosis_safe_disable_module_selector = 0xe009cfde; //@audit -- The declaration of function selector: ModuleManager::disableModule(address prevModule, address module)

    ...

@2  uint256 constant gnosis_safe_disable_module_offset = 0x24; //@audit -- The memory offset intended to point at the 'module' param of the disableModule() was incorrectly specified to the 'prevModule' param instead

@1 -- The declaration of function selector: ModuleManager::disableModule(address prevModule, address module): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalConstants.sol#L58
@2 -- The memory offset intended to point at the 'module' param of the disableModule() was incorrectly specified to the 'prevModule' param instead: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalConstants.sol#L62

The below snippet shows the function signature of the rental safe's ModuleManager::disableModule(): function disableModule(address prevModule, address module) external

Let's break down the value of the gnosis_safe_disable_module_offset constant (0x24): 0x24 == 36 == 32 (the calldata's array length) + 4 (the function selector)

As you can see, the gnosis_safe_disable_module_offset was incorrectly specified to point at the prevModule param (1st param) instead of the module param (2nd param) that refers to the module expected to be removed.

solidity
    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/interfaces/ISafe.sol
    /**
     * @notice Disables the module `module` for the Safe.
     *
     * @dev This can only be done via a Safe transaction.
     *
@3   * @param prevModule Previous module in the modules linked list.
@3   * @param module     Module to be removed.
     */
@3  function disableModule(address prevModule, address module) external; //@audit -- The memory offset must point at the second param because it would be the module to be removed

@3 -- The memory offset must point at the second param because it would be the module to be removed: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/interfaces/ISafe.sol#L98-L101

With the incorrect gnosis_safe_disable_module_offset constant, once the Guard::_checkTransaction() is triggered to verify the safe transaction containing a call to the safe's disableModule(), the address of the prevModule contract will be extracted and assigned to the extension variable instead of the module contract's.

Consequently, the address of the prevModule contract will be verified for the whitelist by the Guard::_revertNonWhitelistedExtension() instead of the expected module contract address.

solidity
    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol
    function _checkTransaction(address from, address to, bytes memory data) private view {
        bytes4 selector;

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

        ... // some if-else cases
        
@4      } else if (selector == gnosis_safe_disable_module_selector) { //@audit -- Check for calls to the disableModule() initiated from Safe contracts
            // Load the extension address from calldata.
            address extension = address(
                uint160(
                    uint256(
@5                      _loadValueFromCalldata(data, gnosis_safe_disable_module_offset) //@audit -- Since the gnosis_safe_disable_module_offset constant points at the incorrect param (i.e., prevModule), the extension variable will contain an address of the prevModule
                    )
                )
            );

            // Check if the extension is whitelisted.
@6          _revertNonWhitelistedExtension(extension); //@audit -- The address of the prevModule will be checked for the whitelist instead of the expected module to be removed
        } 

        ... // else case
    }

@4 -- Check for calls to the disableModule() initiated from Safe contracts: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L255
@5 -- Since the gnosis_safe_disable_module_offset constant points at the incorrect param (i.e., prevModule), the extension variable will contain an address of the prevModule: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L260
@6 -- The address of the prevModule will be checked for the whitelist instead of the expected module to be removed: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L266

Besides, I also noticed that the developer also assumed that the disableModule() would have one function parameter while writing the test functions: test_Success_CheckTransaction_Gnosis_DisableModule() and test_Reverts_CheckTransaction_Gnosis_DisableModule().

That can confirm why the test functions cannot catch up with the mistake.

solidity
    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/test/unit/Guard/CheckTransaction.t.sol
    function test_Success_CheckTransaction_Gnosis_DisableModule() public {
        // impersonate the admin policy admin
        vm.prank(deployer.addr);

        // enable this address to be added as a module by rental safes
        admin.toggleWhitelistExtension(address(mockTarget), true);

        // Build up the `disableModule(address)` calldata
        bytes memory disableModuleCalldata = abi.encodeWithSelector(
            gnosis_safe_disable_module_selector,
@7          address(mockTarget) //@audit -- In the test function: test_Success_CheckTransaction_Gnosis_DisableModule(), the developer assumed that the disableModule() would have one param (incorrect!!)
        );

        // Check the transaction
        _checkTransaction(address(this), address(mockTarget), disableModuleCalldata);
    }

    ...

    function test_Reverts_CheckTransaction_Gnosis_DisableModule() public {
        // Build up the `disableModule(address)` calldata
        bytes memory disableModuleCalldata = abi.encodeWithSelector(
            gnosis_safe_disable_module_selector,
@8          address(mockTarget) //@audit -- Also in the test function: test_Reverts_CheckTransaction_Gnosis_DisableModule(), the developer assumed that the disableModule() would have one param (incorrect!!)
        );

        // Expect revert because of an unauthorized extension
        _checkTransactionRevertUnauthorizedExtension(
            address(this),
            address(mockTarget),
            disableModuleCalldata
        );
    }

@7 -- In the test function: test_Success_CheckTransaction_Gnosis_DisableModule(), the developer assumed that the disableModule() would have one param (incorrect!!): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/test/unit/Guard/CheckTransaction.t.sol#L369
@8 -- Also in the test function: test_Reverts_CheckTransaction_Gnosis_DisableModule(), the developer assumed that the disableModule() would have one param (incorrect!!): https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/test/unit/Guard/CheckTransaction.t.sol#L557

Tools Used

Manual Review

Recommended Mitigation Steps

To point the memory offset at the module param (2nd param), the gnosis_safe_disable_module_offset constant must be set to 0x44 (0x44 == 68 == 32 (the calldata's array length) + 4 (the function selector) + 32 (1st param)).

diff
    // FILE: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalConstants.sol

    /////////////////////////////////////////////////////////////////////////////////
    //                  Gnosis Safe Function Selectors And Offsets                 //
    /////////////////////////////////////////////////////////////////////////////////

    ...

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

    ...

-   uint256 constant gnosis_safe_disable_module_offset = 0x24;
+   uint256 constant gnosis_safe_disable_module_offset = 0x44;