search

code-423n4 / 2023-10-brahma-findings

8 stars 7 forks source link

Resetting a sub-account's guard manually from the Main Console can potentially lead to a permanent denial of service (DoS) for that sub-account. #368

Closed c4-submissions closed 10 months ago

c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/ExecutorPlugin.sol#L73-L74 https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/TransactionValidator.sol#L133 https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/SafeModerator.sol#L71-L72 https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/TransactionValidator.sol#L106 https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/TransactionValidator.sol#L186-L197

Vulnerability details

Impact

If the Main Console resets the guard, resets the fallback handler, or disables itself as a module of a sub-account, the executors will permanently cease executing any transactions on that sub-account.

And also if the Main Console resets the fallback handler or disables itself as a module of a sub-account, the operators will not be able to execute any transactions on the sub-account.

Proof of Concept

I discovered the following three sentences in the README file.

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)

I believe this indicates that the main console can reset the guard, reset the fallback handler, or disable itself as a module of a sub-account through module transactions.

Let's imagine that the main console reset the guard of the sub-account. And then, a particular executor initiates a valid transaction on that sub-account and calls the executeTransaction function of the ExecutorPlugin. Every transaction conducted through the ExecutorPlugin should require a security configuration check after the transaction is completed.

function validatePostExecutorTransaction(address, /*msgSender */ address subAccount) external view {
   _checkSubAccountSecurityConfig(subAccount);
}

And all transactions will be reverted because this sub-account lacks a guard.

function _checkSubAccountSecurityConfig(address _subAccount) internal view {
    address guard = SafeHelper._getGuard(_subAccount);
    address fallbackHandler = SafeHelper._getFallbackHandler(_subAccount);

    // Ensure guard has not been disabled
    if (guard != AddressProviderService._getAuthorizedAddress(_SAFE_MODERATOR_HASH)) revert InvalidGuard();
}

You can add the following test to the test/branch-trees/SafeModerator/SafeModerator.misc.t.sol to verify that the main console can reset the guard of a sub-account.

function testResetSafeModeratorFromSubAccount() public {
        Types.Executable memory _txn = Types.Executable({
            callType: Types.CallType.CALL,
            target: address(subAccount),
            value: 0,
            data: abi.encodeCall(IGnosisSafe.setGuard, (address(0)))
        });

        bytes memory _data = abi.encodeCall(
            IGnosisSafe.execTransactionFromModuleReturnData,
            (_txn.target, _txn.value, _txn.data, SafeHelper._parseOperationEnum(_txn.callType))
        );

        bytes memory _sig = _signTxEOA(
            _encodeTxData(
                address(subAccount), 0, _data, Enum.Operation.Call, 0, 0, 0, address(0), payable(0), mainSafe.nonce()
            ),
            userPrivKey
        );

        assertEq(policyRegistry.commitments(address(subAccount)), keccak256("commit")); // Make sure policyHash is set

        assertTrue(
            mainSafe.execTransaction(
                address(subAccount), 0, _data, Enum.Operation.Call, 0, 0, 0, address(0), payable(0), _sig
            )
        );

        assertEq(SafeHelper._getGuard(address(subAccount)), address(0));
    }

Tools Used

Recommended Mitigation Steps

The fix will be complex, and there are several potential approaches. The first option is to include checks in the transactions made by executors or operators of the sub-account to ensure they are not transactions for resetting the guard, resetting the fallback handler, or disabling the main console (the owner of that sub-account) as a module.

The second approach entails introducing a new contract responsible for maintaining the status of sub-accounts. Before executing transactions initiated by executors or operators, we would check the guard, fallback handler, and module of that sub-account and update the new contract accordingly. In the security configuration check after executing a transaction, we would compare the previous states with the current states to ensure security.

Assessed type

DoS

0xad1onchain commented 11 months ago

Once main account owners change the fallback handler or guard on subaccount, operators and executors automatically lose access to subaccount to prevent any malicious activities by the operators or executors in absence of Validation. To restore access, main console can set the guard and handler back to console approved.

c4-pre-sort commented 11 months ago

raymondfam marked the issue as primary issue

c4-pre-sort commented 11 months ago

raymondfam marked the issue as low quality report

alex-ppg commented 10 months ago

As the Sponsor has specified, the described behaviour of this and all relevant issues is desirable.

In detail:

As a result of the above, all relevant issues are considered invalid.

c4-judge commented 10 months ago

alex-ppg marked the issue as unsatisfactory: Invalid