ownerSafeCount gets updated while not deploying safe, leading to unexpected behavior

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

In SafeDepoyer, a consoleAccount and a subAccount can be created using either deployConsoleAccount or deploySubAccount. Both these functions will make an internal call to _createSafe. In _createSafe there is a do-while loop that tries to deploy a gnosis proxy using these parameters gnosisSafeSingleton, _initializer, nonce.

If this deployment fails with the _SAFE_CREATION_FAILURE_REASON error message, the deployment will be tried again by generating a new nonce:

function _createSafe(..){
//.. ommitted code
nonce = _genNonce(ownersHash, _salt);
} catch {
//.. ommitted code
}

Here lays the problem.

Proof of Concept

Consider the following:

• Alice tries to create a consoleAccount using deployConsoleAccount
• The deployment fails with the _SAFE_CREATION_FAILURE_REASON
• Because its a do-while loop, the deployment will be tried again by generating a new nonce using _genNonce:

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

• The ownerSafeCount will be updated +1 without having deployed a new safe.
• This can go for multiple times and when the safe deployment actually succeeds, the ownerSafeCount will be displaying the wrong data, leading to unexpected behaviour in the future.

  Tools Used

  Manual Review

  Recommended Mitigation Steps

  Use another variable in the nonce calculation instead of ownerSafeCount. Update ownerSafeCount after a successful deployment.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #129

[c4-judge]

alex-ppg marked the issue as not a duplicate

[alex-ppg]

The Warden appears to not understand what the loop within the SafeDeployer::_createSafe function is meant for.

Essentially, the loop will attempt to deploy a multi-signature wallet and will retry if a wallet has already been deployed at the relevant address. As such, the nonce will increase only when a multi-signature wallet has actually been deployed at the relevant address.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid

[alex-ppg]

A slight misbehaviour that may arise in the current code is that the nonce of an owner-set hash is not tied to the salt being utilized.

As such, it is possible for the nonce to not represent the actual number of wallets deployed for a particular hash. It will, however, represent at least the minimum number of wallets deployed for an owner-set.

The Sponsor is advised to evaluate whether they wish to attach owner-sets with the salt_ being used to avoid potential griefing attacks in cross-chain deployments.

The Warden remains ineligible for a reward given that the above misbehaviour is not actually identified in their submission.