SafeDeployer : calling the function `_genNonce` would overflow.

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Genosis safe account can not be created due to overflow of _genNonce

Proof of Concept

The contract SafeDeployer deploy the Genosys safe account. To this, there are set of function which accomplish this task.

First the function deployConsoleAccount will be called with relevant input parameters. This calls the _createSafe function to create the safe account.

when we look at the _createSafe function, it has the logic to calculate and deploy the Genosys safe account.

nonce value is generate based on owners and user provided salt.

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

    function _createSafe(address[] calldata _owners, bytes memory _initializer, bytes32 _salt)
        private
        returns (address _safe)
    {
        address gnosisProxyFactory = AddressProviderService._getAuthorizedAddress(_GNOSIS_PROXY_FACTORY_HASH);
        address gnosisSafeSingleton = AddressProviderService._getAuthorizedAddress(_GNOSIS_SINGLETON_HASH);
        bytes32 ownersHash = keccak256(abi.encode(_owners));

        // Generate nonce based on owners and user provided salt
        uint256 nonce = _genNonce(ownersHash, _salt); -------------------->> find here
        do {
            try IGnosisProxyFactory(gnosisProxyFactory).createProxyWithNonce(gnosisSafeSingleton, _initializer, nonce)
            returns (address _deployedSafe) {
                _safe = _deployedSafe;
            } catch Error(string memory reason) {
                // KEK
                if (keccak256(bytes(reason)) != _SAFE_CREATION_FAILURE_REASON) {
                    // A safe is already deployed with the same salt, retry with bumped nonce
                    revert SafeProxyCreationFailed();
                }
                emit SafeProxyCreationFailure(gnosisSafeSingleton, nonce, _initializer);
                nonce = _genNonce(ownersHash, _salt); -------------------------------------->> find here
            } catch {
                revert SafeProxyCreationFailed();
            }
        } while (_safe == address(0));
    }

as mentioned in above function, the nonce value is calculated by calling the function _genNonce. when we look at the function

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

    function _genNonce(bytes32 _ownersHash, bytes32 _salt) private returns (uint256) {
        return uint256(keccak256(abi.encodePacked(_ownersHash, ownerSafeCount[_ownersHash]++, _salt, VERSION))); ----->> for large keccak256 value, over flow can hapen
    }

Tools Used

Manual review

Recommended Mitigation Steps

Use unchecked { based function implementation to avoid the revert due to overflow.

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

Assessed type

Under/Overflow

[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 relevant nonce increment statement will never realistically overflow; the number of deployments that would need to occur for such an operation are incomprehensible. In detail, the total number of all addresses on Ethereum would have actually run out roughly 2**96 times.

Additionally, callers are free to utilize other _salt values. As such, this exhibit is invalid.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid

[aktech297]

Hi @alex-ppg Thanks for judging.

I would like to add below points on why this issue is cause of concern.

Since the hash value is computed by using the keccak256(abi.encodePacked(_ownersHash, ownerSafeCount[_ownersHash]++, _salt, VERSION)), is really difficult to predict which value would cause the overflow.

We have seen in other contracts which we audited, where the hashing and type conversion is done using the unchecked in order to avoid unnecessary revert which would cause glitches in functioning the system steadily.

I agree that user can change the input salt value, but it is practically tough to go trial and error method to pick a salt value when revert happen randomly / continuously when the other values are dynamic.

This issue would be glitch which cause unpleasant to users. We would see this as temporary DOS.

This issue will happen randomly and will impact most of the users.

I fully respect the judges decision on this issue.

Thanks!

[alex-ppg]

Hey @aktech297, thank you for chiming in!

The ownerSafeCount value is mutated in a single instance of the codebase; the referenced post-fix increment statement (ownerSafeCount[_ownersHash]++).

This means that the value of ownerSafeCount is incremented by 1 each time the SafeDeployer::_genNonce function is invoked. This function will successfully increment the ownerSafeCount solely in the following scenarios:

• A: A new Gnosis Safe was deployed at the address generated by the nonce
• B: A Gnosis Safe already exists at the address generated by the nonce

As such, each 1 value increment of the ownerSafeCount means that an address in the blockchain is occupied by a smart contract.

Given that an address is a uint160 value under the hood, this means that we would have to deploy a smart contract on every possible address on Ethereum before we are even close to an overflow occurring.

To add one last statement to this response, permitting overflows to occur in this case would actually cause the code to always fail as a deployment would have already existed on the overflown 0 value.

In summary, for all intents and purposes that the EVM permits an overflow will not occur in our lifetimes and even if it did, an unchecked tag would not solve it.