Multichain deployed addresses can be disrupted to not deploy on the same address, which could lead to DoS

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Brahma intends to deploy it's functionality to multiple blockchains with all the Safe being in the same address, and passes down to gnosisProxyFactory::createProxyWithNonce the initializer and nonce, which results from the _genNonce() function:

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

If ownerSafeCount[_ownersHash], the _owners array and the _salt are copied from the Blockchain in which it is first deployed, into another one, because the create2 is deterministic and works the same way through all the intended blockchains where Brahma intends to deploy (as you can see here, the GnosisSafeProxyFactory is on the same address), any user could disrupt the functionality of the multichain deployments by frontranning any of the blockchain deployments, which would make the _genNonce() to return a different nonce because the ownerSafeCount[_ownersHash] would have been incremented after the first loop on the do while, resulting in the deployment being in a different address.

This has severe consequences because the frontranning actor could have linked a different _policyCommit to the expected address when calling SafeDeployer::deployConsoleAccount() or SafeDeployer::deploySubAccount. Potentially disrupting and/or bricking this chain Console Account if the policyHash is left to an incongruent value or a valid one but not the same as the same address on the other chains.

Even if such a problem was already acknowledged by the team, the protocol has no way to know which is the desired policy commit the user really wants in an onchain way, since the frontran can happen simmultaneously in all or the majority of the blockchains, the only source of truth for deriving such a think should be done in the frontend.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. Such an attack works can be done to mempool Console Account and SubAccount pending deployments:

1. On detecting of a transaction calling deployConsoleAccount() or deploySubAccount(), frontran it with it's same calldata.
2. At the same time, have this transactions flow into the different blockchains the protocol intends to deploy but with different a different value for bytes32 _policyCommit, this could be one that registers either fund-lossing strategies, DOS strategies or simply unintended ones.

This would break the _doDeployedAddressesMatchPrevious test.

Tools Used

Manual review, Foundry and VSCode.

Recommended Mitigation Steps

Acknowledge that all addresses can have different by a Cross-Blockchain Nonce Coordination done offchain or an Atomic Commitment Across Blockchains which works through a two-phase commit protocol or a similar distributed transactions protocol, which ensures that a transaction is either committed on all blockchains or none at all.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #160

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #468

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)

[c4-judge]

alex-ppg marked the issue as grade-b