Deploying a Console to the Same Address Across Different Supported Chains Could Become Impossible

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/SafeDeployer.sol#L56-L72 https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/SafeDeployer.sol#L219-L245 https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/SafeDeployer.sol#L253-L255

Vulnerability details

Impact

In Brahma, Users can interact with SafeDeployer::deployConsoleAccount to deploy console accounts/wallets. To deploy the wallet to the same address across all supported chains, the user needs to interact with the deployConsoleAccount function on all chains providing a list of owners in the same order and using the same unique salt value.

The function _genNonce plays an important role in deploying these deterministic safes, as given an owner's hash and salt, this function returns the saltNonce which is used in generating the salt used for deploying the safe.

    function _genNonce(bytes32 _ownersHash, bytes32 _salt) private returns (uint256) {
        return uint256(keccak256(abi.encodePacked(_ownersHash, ownerSafeCount[_ownersHash]++, _salt, VERSION)));
    }

I have observed two possible issues with the current method of generating nonces in the _genNonce function. These issues can obstruct the user's ability to deploy wallets to deterministic addresses on different chains, even while using the same set of owners and salt value.

Weakness 1: Reliance on ownerSafeCount for Nonce Generation

The problem is that the value of ownerSafeCount is incremented each time the _genNonce function is called within the _createSafe function, this can create a disparity between the owner safe count used in generating a nonce in chain A from the value used in chain B. How _createSafe function works is, assuming the first call with nonce1 to GnosisSafe::createProxyWithNonce function fails, genNonce function is called to generate a new nonce, and then createProxyWithNonce function call is retried again. On retry(i.e. genNonce call), the ownerSafeCount value is incremented. Now, consider a scenario where the owner's safe count is initially zero (0). If the user's first and second attempts at creating a wallet fail but the third attempt succeeds, the nonce is generated based on an owner safe count of two (2). Now, assuming the same user tries to deploy a wallet with the same identical parameters on a different chain. On this new chain, the wallet creation occurs when ownerSafeCount equals zero (0). Consequently, a different nonce is employed in this deployment. As a result, the wallet is deployed to a distinct address.

Weakness 2: Reliance on VERSION for Nonce Generation

The second weakness arises from the reliance on the version of the SafeDeployer contract (VERSION) for nonce generation. This reliance complicates the ability to deploy a safe to a deterministic address on different chains. To illustrate, let's consider a scenario:

Suppose a user deploys a safe or wallet on the Ethereum blockchain at x time when the SafeDeployer contract's version is, for example, 'One (1)'. Later on, at a different point in time, the same group of safe owners intends to deploy their safe on other chains. However, at this latter time, there is a new SafeDeployer contract version, let's say two(2), and the previous version is discarded: Here is a message from a sponsor:

###### — 
Will there be different versions of SafeDeployer in the future?
If a new version of SafeDeployer is deployed what happens to the old version?
0xAd1 — 
The old safe deployer might be discarded if we decide to change the Safe deployer later
In that case, the New safe deployer address will be updated in the authorized addresses

Under these circumstances, despite all other conditions remaining the same, a different nonce will be generated. Since the GnosisSafe::createProxyWithNonce function relies on both the generated nonce and the initializer to calculate a salt value for deployment. The safe will be deployed to a distinct deterministic address on the other chain, leading to non-deterministic deployments.

    function deployProxyWithNonce(address _mastercopy, bytes memory initializer, uint256 saltNonce)
        internal
        returns (Proxy 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(Proxy).creationCode, uint256(_mastercopy));
        // solium-disable-next-line security/no-inline-assembly
        assembly {
            proxy := create2(0x0, add(0x20, deploymentData), mload(deploymentData), salt)
        }
        require(address(proxy) != address(0), "Create2 call failed");
    }

    /// @dev Allows to create new proxy contact and execute a message call to the new proxy within one transaction.
    /// @param _mastercopy Address of master copy.
    /// @param initializer Payload for message call sent to new proxy contract.
    /// @param saltNonce Nonce that will be used to generate the salt to calculate the address of the new proxy contract.
    function createProxyWithNonce(address _mastercopy, bytes memory initializer, uint256 saltNonce)
        public
        returns (Proxy proxy)
    {
        proxy = deployProxyWithNonce(_mastercopy, initializer, saltNonce);
        if (initializer.length > 0)
            // solium-disable-next-line security/no-inline-assembly
            assembly {
                if eq(call(gas, proxy, 0, add(initializer, 0x20), mload(initializer), 0, 0), 0) { revert(0,0) }
            }
        emit ProxyCreation(proxy);
    }

Proof of Concept

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

https://vscode.blockscan.com/gnosis/0x8b4404de0caece4b966a9959f134f0efda636156

Tools Used

Manual Review

Recommended Mitigation Steps

I will recommend you allow the user to retry with a different salt if call to IGnosisProxyFactory::createProxyWithNonce fails. I will also recommend changing _genNonce to:

    function _genNonce(bytes32 _ownersHash, bytes32 _salt) private returns (uint256) {
        return uint256(keccak256(abi.encodePacked(_ownersHash, _salt)));
    }

Assessed type

en/de-code

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #143

[c4-pre-sort]

raymondfam marked the issue as duplicate of #398

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #21

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid

[Tendency001]

Hi ser, thanks for the great work judging this contest. I would like to add here that this issue isn't a duplicate of #21 This issue describes how based on the protocol implementation the goal of allowing users to create deterministic wallets across different chains is oppressed. I have described how the issue spans from 1. the increment of the owner's safe count. 2. The use of contract version. While #21 and its likes described a possible DOS, that isn't what this report describes. Thank you.

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

 To generate deterministic addresses for a given set of owners, the order of owner addresses and threshold should be same
@param _owners list of owners addresses

[alex-ppg]

Hey @Tendency001, thanks for following up! While the submission might not by a direct duplicate of the other issues, it relies on the same underlying mechanism.

Weakness 1

This particular scenario relies on an incorrect assumption. In the described case, the “failures” would occur because a wallet is already present at the addresses the SafeDeployer attempted to deploy it.

As such, chain A would actually have 3 Console Accounts while chain B would have 1 Console Account. Regardless of this, I list a reason why these are non issues in Weakness 2.

Weakness 2

If a new deployer is created for a different version of Brahma Console Accounts, it is wise to use a different address generation system as different versions of Brahma would have different features, comply to different interfaces, etc.

Even if the original SafeDeployer is not available for some reason, users can directly interact with the Gnosis Safe deployer to deploy the addresses they want as the address generation ultimately is done by Gnosis and not by Brahma.

Conclusion

Under all cases, no loss of funds will be caused. It is advisable for Brahma to include a VERSION in their system to ensure address generations are distinct between Brahma versions.

Please let me know if the above address your concerns and if you require any further clarifications.

[Tendency001]

Thanks for the response ser, this will do