c4-submissions commented 11 months ago

Lines of code

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

Vulnerability details

Impact

According to the docs, the fallback handler provides compatibility between pre-1.3.0 and 1.3.0+ Safe contracts, and additionally, also ensures policy validation guarantees required for ConsoleAccounts/SubAccounts that have policy validation enabled. If not set, this functionality will not be available.

Proof of Concept

In ConsoleOpBuilder.enablePolicyOnConsole(), it was explicitly stated what needs to be done if policy is enabled: https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/ConsoleOpBuilder.sol#L25C5-L25C124

* @dev Enables policy, by updating policy on `PolicyRegistry`, setting guard on Safe, setting fallback handler on Safe

This was implemented as stated in ConsoleOpBuilder.enablePolicyOnConsole(), a function that returns the multicall bytecode for enabling policy on brahma console account. https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/ConsoleOpBuilder.sol#L53-L56

txns[2] = Types.Executable({
            callType: Types.CallType.CALL,
            target: account,
            value: 0,
            data: abi.encodeCall(
                IGnosisSafe.setFallbackHandler,
                (AddressProviderService._getAuthorizedAddress(_CONSOLE_FALLBACK_HANDLER_HASH))
                )
        });

However, in SafeDeployer._setupConsoleAccount(), if policy is enabled, fallback handler address is obtained but not set in safe., only guard was set there. https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/SafeDeployer.sol#L118-L130

if (_policyHashValid) {
            txns = new Types.Executable[](2);
            fallbackHandler = AddressProviderService._getAuthorizedAddress(_CONSOLE_FALLBACK_HANDLER_HASH);

            // Enable guard on console account
            txns[1] = Types.Executable({
                callType: Types.CallType.DELEGATECALL,
                target: AddressProviderService._getAuthorizedAddress(_SAFE_ENABLER_HASH),
                value: 0,
                data: abi.encodeCall(
                    IGnosisSafe.setGuard, (AddressProviderService._getAuthorizedAddress(_SAFE_MODERATOR_OVERRIDABLE_HASH))
                    )
            });

Tools Used

Manual review.

Recommended Mitigation Steps

Plsease kindly Include setting the fallBack on safe in the multicall transaction when policy is being set, thank you.

  if (_policyHashValid) {
            txns = new Types.Executable[](3);
            fallbackHandler = AddressProviderService._getAuthorizedAddress(_CONSOLE_FALLBACK_HANDLER_HASH);

            // Enable guard on console account
            txns[1] = Types.Executable({
                callType: Types.CallType.DELEGATECALL,
                target: AddressProviderService._getAuthorizedAddress(_SAFE_ENABLER_HASH),
                value: 0,
                data: abi.encodeCall(
                    IGnosisSafe.setGuard, (AddressProviderService._getAuthorizedAddress(_SAFE_MODERATOR_OVERRIDABLE_HASH))
                    )
            });

         // Enable fallBack on console account
         txns[2] = Types.Executable({
            callType: Types.CallType.DELEGATECALL
            target: AddressProviderService._getAuthorizedAddress(_SAFE_ENABLER_HASH),
            value: 0,
            data: abi.encodeCall(IGnosisSafe.setFallbackHandler, (fallbackHandler))
        });

Assessed type

Other

c4-pre-sort commented 11 months ago

raymondfam marked the issue as low quality report

c4-pre-sort commented 11 months ago

raymondfam marked the issue as duplicate of #368

c4-judge commented 10 months ago

alex-ppg marked the issue as not a duplicate

alex-ppg commented 10 months ago

The fallback handler of a Gnosis Safe deployed via the SafeDeployer is automatically set via the IGnosisSafe::setup call that is encoded at the end of the relevant function body.

As such, the fallback handler is properly set and does not need to perform any special delegate call instruction.