c4-submissions commented 10 months ago

Lines of code

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

Vulnerability details

Impact

The same order of _owners addresses lets generate the same console address on all chains. But any owner from the list can deploy console accounts on other chains with _threshold parameter equals 1 and then change owners in these accounts, i.e. capture these addresses. Another problem is that any user can deploy console accounts on other chains with _threshold == _owners.length. So to reconfigure these accounts it is necessary to have signatures from all owners from the list, but this is not always possible (compromising, dismissals, etc.)

Proof of Concept

It is enough to use the same _salt and the same _owners (in the same order) to to generate same console address on all chains with the deployConsoleAccount function of the SafeDeployer contract:

    /**
     * @notice Deploys a new console account with or without policy commit and registers it
     * @dev _owners list should contain addresses in the same order to generate same console address on all chains
     * @param _owners list of safe owners
     * @param _threshold safe threshold
     * @param _policyCommit commitment
     * @param _salt salt to be used during creation of safe
     * @return _safe deployed console account address
     */
    function deployConsoleAccount(address[] calldata _owners, uint256 _threshold, bytes32 _policyCommit, bytes32 _salt)
        external
        nonReentrant
        returns (address _safe)
    {
        bool _policyHashValid = _policyCommit != bytes32(0);

        _safe = _createSafe(_owners, _setupConsoleAccount(_owners, _threshold, _policyHashValid), _salt);

        if (_policyHashValid) {
            PolicyRegistry(AddressProviderService._getRegistry(_POLICY_REGISTRY_HASH)).updatePolicy(
                _safe, _policyCommit
            );
        }
        emit ConsoleAccountDeployed(_safe);
    }

The _salt and _owners parameters are used for nonce calculation:

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

Then the nonce is used for a new Gnosis Safe creation

230            try IGnosisProxyFactory(gnosisProxyFactory).createProxyWithNonce(gnosisSafeSingleton, _initializer, nonce)
231            returns (address _deployedSafe) {
232                _safe = _deployedSafe;

Also _owners parameter with _threshold parameter are used for the console account setup in the _setupConsoleAccount. The _threshold parameter is very important for a multisig but it is not included in the nonce. So new console accounts on other chains can be deployed with different _thresholds. This lets any owner from the initial list capture the addresses on other chains by deploying console accounts with _threshold parameter equals 1 and removing other owners. This issue can be also exploited by any user with different aims and unexpected consequences.

Tools Used

Manual review

Recommended Mitigation Steps

Consider using the _threshold parameter at the nonce calculation in addition to _owners for a new Gnosis Safe creation.

Assessed type

Other

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 #71

c4-judge commented 10 months ago

alex-ppg marked the issue as not a duplicate

alex-ppg commented 10 months ago

If the threshold parameter of the safe is altered, its initializer will also be affected meaning that the Gnosis Safe will be deployed at a different address than expected. As such, it is not possible to deploy a configured safe of n-out-of-m security in chain A to chain B at the same address using a 1-out-of-m configuration.