Nonce generation must include threshold to avoid collisions during safe deployments.

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

The nonce generation uses the owner addresses and a salt, but not the threshold. So the same owners with different thresholds will collide. Should also include threshold.

Proof of Concept

The nonce generation that does not include the threshold parameter is in the _genNonce function

function _genNonce(bytes32 _ownersHash, bytes32 _salt) private returns (uint256) {

  return uint256(keccak256(abi.encodePacked(
    _ownersHash, 
    ownerSafeCount[_ownersHash]++, 
    _salt,
    VERSION
  )));

}

Specifically, the _ownersHash is generated from the owner addresses

bytes32 ownersHash = keccak256(abi.encode(_owners));

But the threshold is not included in the _ownersHash or _genNonce.

This means the same owner addresses will generate the same _ownersHash, even if they have different thresholds.

So, this could lead to collisions in the nonces when deploying safes for the same owners but different thresholds.

The impact of not including the threshold in the nonce generation in _genNonce, which is used to generate a unique nonce for each safe deployment.

The nonce relies on two main values:

1. _ownersHash - a hash of the owner addresses

2. _salt - a random salt provided by the caller

The problem is _ownersHash only includes the owner addresses, generated by:

bytes32 ownersHash = keccak256(abi.encode(_owners)); 

It does NOT include the threshold value.

This means the SAME _ownersHash will be produced for the same owners, regardless of the threshold.

For example:

Owners A, B, and C with threshold 1 will produce the same _ownersHash as A, B, and C with threshold 2.

Then _genNonce uses this along with the salt to generate the nonce:

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

Since the _ownersHash is the same for different thresholds, the nonce will be the same.

This causes a collision in the nonces when deploying safes for the same owners but different thresholds.

The impact of this is:

• Possible failed deployments due to nonce collisions, causing griefing.

• Security risks from an attacker pre-computing addresses by guessing owner sets and salts.

• Inability to reliably generate unique, deterministic safe addresses for an owner set.

By also including the threshold in _ownersHash and _genNonce, it ensures each owner set + threshold combination gets a unique nonce. This prevents collisions and unpredictability issues.

Code that demonstrates the proof. Here is further proof that _genNonce does not include the threshold:

// _genNonce 
function _genNonce(bytes32 _ownersHash, bytes32 _salt) private returns (uint256) {

  return uint256(keccak256(abi.encodePacked(
    _ownersHash, // _ownersHash is only from owner addresses
    ownerSafeCount[_ownersHash]++, 
    _salt,
    VERSION
  )));

}

// _ownersHash
bytes32 ownersHash = keccak256(abi.encode(_owners)); // only encodes owner addresses

That the key issue is that _ownersHash only encodes the owner addresses:

bytes32 ownersHash = keccak256(abi.encode(_owners)); 

It does NOT include the threshold value.

This means the SAME _ownersHash will be generated for the same owners, regardless of different thresholds.

Tools Used

Manual

Recommended Mitigation Steps

The fix would be to also include the threshold value in the inputs to _genNonce and _ownersHash, for example:

bytes32 ownersHash = keccak256(abi.encode(_owners, _threshold)); 

// and

function _genNonce(bytes32 _ownersHash, uint256 _threshold, bytes32 _salt) {
  // include threshold 
}

This ensures unique nonces are generated per owner set and threshold, avoiding collisions.

Assessed type

Invalid Validation

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #71

[c4-judge]

alex-ppg marked the issue as not a duplicate

[alex-ppg]

The threshold as well as owners specified for a Gnosis Safe directly influence its deployed address based on the implementation of the Gnosis Safe deployer.

As such, the described behaviour by the Warden is incorrect given that an altered threshold will lead to an entirely new address deployment.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid