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

8 stars 7 forks source link

The user won't be able to create Safe console deterministic addresses across target chains #321

Closed c4-submissions closed 12 months ago

c4-submissions commented 1 year ago

Lines of code

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

Vulnerability details

Bug Description

In the Natspec documentation, it is mentioned what needs to be done to obtain the same address for a SafeConsole wallet on different chains, but as indicated in the title, this cannot be achieved.

// Comment 1
@dev _owners list should contain addresses in the same order to generate same console address on all chains

// Comment 2
To generate deterministic addresses for a given set of owners, the order of owner addresses and threshold should be same

Proof-of-Concept

Two scenarios will be outlined to illustrate the unattainability of identical Safe Console addresses across various target chains.

Scenario 1

If we examine the official Gnosis Safe addresses, it becomes apparent that the GnosisSafeProxyFactory is not uniform across all chains. Given that the GnosisSafeProxyFactory serves as the deployer for the ConsoleAccount, and it varies among chains, the ConsoleAccount will exhibit differences as well. Using the official Gnosis addresses (GnosisSafeProxyFactory, GnosisSafe, etc.), creating deterministic ConsoleAccounts on the following chains, Optimism, Base, and Avalanche C, is not possible.

Scenario 2

It's possible to bypass the first scenario by using unofficial Gnosis addresses, but a new problem will arise that prevents the creation of a deterministic ConsoleAccount.

The creation of a console account is carried out using the function GnosisSafeProxyFactory.deployProxyWithNonce(). If any of the arguments accepted by GnosisSafeProxyFactory.deployProxyWithNonce changes, the address of the GnosisSafeProxy(ConsoleAccount) will also be different:

function deployProxyWithNonce(
        address _singleton,
        bytes memory initializer,
        uint256 saltNonce
    ) internal returns (GnosisSafeProxy proxy) {
        // If the initializer changes the proxy address should change too. Hashing the initializer data is cheaper than just concatinating it
        bytes32 salt = keccak256(abi.encodePacked(keccak256(initializer), saltNonce));
        bytes memory deploymentData = abi.encodePacked(type(GnosisSafeProxy).creationCode, uint256(uint160(_singleton)));
        // solhint-disable-next-line no-inline-assembly
        assembly {
            proxy := create2(0x0, add(0x20, deploymentData), mload(deploymentData), salt)
        }
        require(address(proxy) != address(0), "Create2 call failed");
    }

Let's break down each argument that is passed to GnosisSafeProxyFactory.deployProxyWithNonce:

_singleton = AddressProviderService._getAuthorizedAddress(_GNOSIS_SINGLETON_HASH)

initializer = _setupConsoleAccount(_owners, _threshold, _policyHashValid)

saltNonce = uint256(keccak256(abi.encodePacked(_ownersHash, ownerSafeCount[_ownersHash]++, _salt, VERSION)))

Since the creation of a console account is not bound to msg.sender, a malicious user can frontrun the SafeDeployer.deployConsoleAccount transaction and increase the nonce. Because the nonce depends solely on the owners, a malicious user can create a console account with different parameters such as threshold, salt, or with/without policyCommit. This can lead to the creation of an undesired address and increase ownerSafeCount. As a result, there will be two new console accounts, one from the malicious user and the other from a regular user, which will differ from the intended console account address.

Impact

A user aiming to generate identical ConsoleAccount addresses across various chains will not be able to do so.

Tools Used

Manual

Recommended Mitigation Steps

Please add to the documentation that you will be using unofficial Gnosis addresses (GnosisSafeProxyFactory, Singleton, etc.). Make sure that ownerSafeCount depends not only on ownerHash but also on msg.sender. In this way, a user who genuinely wants to generate the same console account address will be able to do so, while other users who do not have the same intention can generate console accounts without any issues.

Assessed type

Context

c4-pre-sort commented 1 year ago

raymondfam marked the issue as low quality report

c4-pre-sort commented 1 year ago

raymondfam marked the issue as duplicate of #143

c4-pre-sort commented 1 year ago

raymondfam marked the issue as duplicate of #398

c4-judge commented 12 months ago

alex-ppg marked the issue as not a duplicate

alex-ppg commented 12 months ago

The Warden describes an out-of-scope issue that relates directly to the Gnosis Safe implementation rather than the Brahma implementation.

The referenced comment by Brahma implies that cross-chain multi-sig wallet deployments will have the same address as long as it is permitted by the Gnosis Safe implementation in the first place.

As such, I consider this exhibit invalid / out-of-scope.

c4-judge commented 12 months ago

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