Possibility of an infinite loop until out-of-gas in _createSafe()

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

There is a possibility of going into an infinite loop within the _createSafe function until out-of-gas.

Proof of Concept

The condition while (_safe == address(0)) in _createSafe function contains a do-while loop that continues as long as the _safe address remains zero. If the IGnosisProxyFactory.createProxyWithNonce function repeatedly fails due to reasons other than nonce conflicts, The _safe address will never be assigned a non-zero value, resulting in an infinite loop until out-of-gas. Point form explaination below.

• It enters a do-while loop that continues until a Gnosis Safe contract is successfully created.
• Inside the loop, it tries to create a Gnosis Safe proxy using the IGnosisProxyFactory interface.
• If the creation is successful and a new safe is deployed, it sets the _safe variable to the address of the deployed safe and exits the loop.
• If an unexpected error occurs during the creation process not matching the predefined reason _SAFE_CREATION_FAILURE_REASON or not due to nonce conflict, the _safe address wont be updated to non-zero value.
• So as mentioned in point 4, If the _safe address not updated to non-zero value, The condition while (_safe == address(0)) is true and will lead into a infinite loop resulting to the function temporally.

Tools Used

Manuel Review

Recommended Mitigation Steps

Implement a maximum retry count to limit the number of attempts to create a Gnosis Safe contract. Basically adding an additional condition to break the loop when the _safe address remains zero after the maximum number of retries.

      } while (_safe == address(0) && retryCount <= maxRetries);

Assessed type

Loop

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

No loss of funds similar to an unbounded loop.

[alex-ppg]

The gas cost required to force the transaction to run out of gas is significant and the caller can simply utilize a different salt_ to start a fresh loop that would require the "attacker" to re-deploy all necessary wallets again. This scenario is unrealistic and as such, all relevant exhibits are invalid.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid