search

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

8 stars 7 forks source link

No proper validation of Singleton #457

Closed c4-submissions closed 10 months ago

c4-submissions commented 10 months ago

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/TransactionValidator.sol#L197

Vulnerability details

Bug Description

Operators, executors, or the Main Console account can execute transactions on behalf of a SubAccount. SubAccounts must have an enabled SafeModerator guard, which checks whether the guard and handler have not been disabled or updated, and whether the owner console, acting as a module, has not been disabled. But there is no check to verify whether the user has changed the current singleton to another, malicious one.

Proof-of-Concept

Anyone from the list mentioned above can send a transaction to change the Singleton (GnosisSafe implementation). For operators and executors, the TRUSTED VALIDATOR should allow this.

Any of these actors can set the address of the GnosisSafe implementation to a malicious one. For example, the internal function TransactionValidator._checkSubAccountSecurityConfig, which calls the IGnosisSafe.isModuleEnabled function, may return an invalid value. As a result, the owner console as a module can be disabled, but isModuleEnabled will still return 'true:

function _checkSubAccountSecurityConfig(address _subAccount) internal view {
        ///code

        address ownerConsole =
            WalletRegistry(AddressProviderService._getRegistry(_WALLET_REGISTRY_HASH)).subAccountToWallet(_subAccount);

        // Ensure owner console as a module has not been disabled
        if (!IGnosisSafe(_subAccount).isModuleEnabled(ownerConsole)) revert InvalidModule();
    }

Impact

Since there is no check in place to prevent subAccounts from altering the Singleton, it is possible to set a malicious one and manipulate function calls on the subAccount. I understand that this can be done manually and bypass all security measures, but the protocol itself does not verify this, as it does with guard, fallback handler, and module.

Tools Used

Manual

Recommended Mitigation Steps

Consider validating whether the address of the Gnosis Safe implementation has not changed to a malicious one using the SafeModerator guard within the _checkSubAccountSecurityConfig function.

Assessed type

Invalid Validation

c4-pre-sort commented 10 months ago

raymondfam marked the issue as low quality report

c4-pre-sort commented 10 months ago

raymondfam marked the issue as duplicate of #39

c4-judge commented 10 months ago

alex-ppg marked the issue as not a duplicate

alex-ppg commented 10 months ago

The Warden talks about a Singleton which does not apply to the Brahma system. Swapping the "address" of the Gnosis Safe is not possible.

c4-judge commented 10 months ago

alex-ppg marked the issue as unsatisfactory: Invalid