Lack of Input Validation on threshold and _owners

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Unvalidated inputs can lead to unexpected contract behaviors, including but not limited to, incorrect configurations, locked funds, or erroneous operations. In extreme cases, it could also lead to security vulnerabilities if malicious actors can exploit these oversights to their advantage.

Proof of Concept

In the deployConsoleAccount and deploySubAccount function, the threshold and _owners parameters are used without validation checks. The threshold is critical as it sets the number of owner approvals required for executing transactions. A zero or very high threshold could lock funds or make the safe unusable. Similarly, an empty _owners array or duplicate owner addresses could also lead to undesired behaviors.

function deployConsoleAccount(address[] calldata _owners, uint256 _threshold, bytes32 _policyCommit, bytes32 _salt)
    external
    nonReentrant
    returns (address _safe)
{
    //...
    _safe = _createSafe(_owners, _setupConsoleAccount(_owners, _threshold, _policyHashValid), _salt);
    //...
} 

Hypothetical steps to cause the issue:

-A user calls the deployConsoleAccount function with a threshold of 0 and a valid _owners array. The contract deploys a new Gnosis Safe with a threshold of 0, meaning no approvals are required to execute transactions, which could lead to a lack of control and potential fund loss.

Or,

-A user calls the deployConsoleAccount function with an empty _owners array. The contract attempts to deploy a new Gnosis Safe with no owners, which could lead to a locked contract with no one able to execute transactions.

Tools Used

Manual

Recommended Mitigation Steps

Implementing input validation checks can mitigate the risks associated with incorrect or malicious inputs. Here are some steps:

Validate Threshold:

Ensure that the threshold is greater than 0 and less than or equal to the number of owners.

require(_threshold > 0 && _threshold <= _owners.length, "Invalid threshold value");
Validate Owners Array:

Ensure that the _owners array is not empty and does not contain duplicate addresses.

require(_owners.length > 0, "Owners array is empty");
for (uint i = 0; i < _owners.length; i++) {
    for (uint j = i + 1; j < _owners.length; j++) {
        require(_owners[i] != _owners[j], "Duplicate owner address");
    }
}

By adding these validation checks at the beginning of the deployConsoleAccount and deploySubAccount function, you can prevent incorrect or malicious inputs from causing undesired behaviors or vulnerabilities within the contract.

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

[c4-judge]

alex-ppg marked the issue as duplicate of #136

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid