Module transactions will always fail because incompatible with Safe 1.5.0

mediumCode4rena

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/SafeModerator.sol#L80-L86 https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/SafeModeratorOverridable.sol#L86-L92

Vulnerability details

Impact

GnosisSafe has concept of module. Trusted modules of GnosisSafe contract can execute arbitrary transactions without signatures once enabled. That's the main purpose of protocol: to allow execute some defined action (like swap on DEX) without signatures via registering such an action in Policy commitment. And then execute this action via Executor or Main Account without need of delegatee's signatures. And also allows to register SubAccounts and manage them via Main Account.

Therefore ExecutorPlugin and Main Account are enabled as modules. Main problem is that all transactions executed via module will fail - it means that SubAccount is just another GnosisSafe without additional utility. When assessing the severity I take into consideration protocol specific: protocol is about adding new functionality

Users also have access to SafeSub-accounts that reduce their risk from the protocol by isolating their interactions.

But core functionality of protocol regarding ExecutorPlugin and SubAccounts doesn't work. Due to this reasoning I submit it as High, despite there is no financial loss

Proof of Concept

Let's take a look on how tx from module is executed. Firstly guard.checkModuleTransaction() is called which returns bytes32 guardHash, and then transaction is executed: https://github.com/safe-global/safe-contracts/blob/810fad9a074837e1247ca24ac9e7f77a5dffce19/contracts/base/ModuleManager.sol#L82-L104

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

In context of Brahma, Guard contract is SafeModerator or SafeModeratorOverridable. They implement incorrect interface IGuard, so that function checkModuleTransaction() returns nothing https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/interfaces/external/IGnosisSafe.sol#L110-L116

solidity📋
    function checkModuleTransaction(
        address to,
        uint256 value,
        bytes memory data,
        Enum.Operation operation,
        address module
    ) external;

Here is PoC in Remix to ensure that function reverts when tries to decode output while function doesn't return anything

solidity📋
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;

interface WithoutReturn {
      function checkModuleTransaction() external;
}

interface WithReturn {
      function checkModuleTransaction() external returns(bytes32);
}

contract Test {

  function test() external returns(bytes32) {

    address calledContract = address(new ContractWithoutReturn());

    // Reverts on decoding
    bytes32 result = WithReturn(calledContract).checkModuleTransaction();
    return result;
  }
}

contract ContractWithoutReturn is WithoutReturn {

  function checkModuleTransaction() external {}

}

As a result, function GnosisSafe.execTransactionFromModule() will always revert.

Tools Used

Manual Review, Remix

Recommended Mitigation Steps

Update interface IGuard:

diff📋
    function checkModuleTransaction(
        address to,
        uint256 value,
        bytes memory data,
        Enum.Operation operation,
        address module
-   ) external;
+   ) external returns(bytes32);

Note

Protocol is meant to use 1.5.0; that feature with callback to guard in module transaction was introduced in 1.5.0 recently. I also advice to add tests for different safe Versions; now only 1.3.0 is used in tests

Sponsor said they took interface from this PR because GnosisSafe 1.5.0. is still in dev And further in discussion they noted mistake in interface

Assessed type

Error