`getMessageHash` lacks input validation, complex hashing, weak `signedMessages` check.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/c217699448ffd7ec0253472bf0d156e52d45ca71/contracts/src/core/ConsoleFallbackHandler.sol#L68-L71 https://github.com/code-423n4/2023-10-brahma/blob/c217699448ffd7ec0253472bf0d156e52d45ca71/contracts/src/core/ConsoleFallbackHandler.sol#L42-L53 https://github.com/code-423n4/2023-10-brahma/blob/c217699448ffd7ec0253472bf0d156e52d45ca71/contracts/src/core/ConsoleFallbackHandler.sol#L68 https://github.com/code-423n4/2023-10-brahma/blob/c217699448ffd7ec0253472bf0d156e52d45ca71/contracts/src/core/ConsoleFallbackHandler.sol#L43-L44

Vulnerability details

Impact

Potential issues with signature validation logic in getMessageHash and usage in isValidSignature. The getMessageHash and isValidSignature methods are critical for securely verifying signatures on transactions. Any flaws could allow an attacker to bypass policies or spoof signatures to steal funds.

Proof of Concept

Here is the key signature validation logic:

In getMessageHash: #Line 68-70

function getMessageHashForSafe(GnosisSafe safe, bytes memory message) public view returns (bytes32) {

  bytes32 safeMessageHash = keccak256(abi.encode(SAFE_MSG_TYPEHASH, keccak256(message)));

  return keccak256(abi.encodePacked(bytes1(0x19), bytes1(0x01), safe.domainSeparator(), safeMessageHash));

}

And in isValidSignature:

https://github.com/code-423n4/2023-10-brahma/blob/c217699448ffd7ec0253472bf0d156e52d45ca71/contracts/src/core/ConsoleFallbackHandler.sol#L42-L53

The issues here: are

• No validation of _data in getMessageHash - could lead to hash collision vulnerabilities if manipulated

• Overly complex hashing logic - room for errors or hash collisions

• signedMessages check doesn't ensure threshold is met

The getMessageHash and isValidSignature methods are critical for securely verifying signatures on transactions. Any flaws could allow an attacker to bypass policies or spoof signatures to steal funds.

Some key risks are:

• No input validation in getMessageHash - an attacker could manipulate _data to intentionally cause hash collisions, resulting in identical hashes and signatures for different transactions. This defeats the uniqueness property required for security.

• Overly complex hashing logic - the multi-step hashing and encoding process has a higher risk of implementation bugs or hash collisions compared to a simple hash function. Extra care needs to be taken in review.

• Ineffective signedMessages check - this only checks if >0 signatures exist, not if the threshold is met. Could allow partial signatures.

• No checking return values from checkSignatures. Failed or errored responses could be missed.

These flaws could allow signatures to be reused across different transactions, bypass thresholds, or spoof signatures entirely.

Attackers could bypass policies to steal funds, destroy contracts, etc. Auditing would be hindered since different transactions appear signed by owners.

Let me substantiate the analysis around the signature validation logic risks by pointing to the specific lines of code:

Lack of input validation in getMessageHash: 68

function getMessageHashForSafe(GnosisSafe safe, bytes memory message) public view returns (bytes32) {

  // message passed directly to hash without validation

}

Overly complex hashing

// Multiple steps including keccak256, abi.encode, abi.encodePacked

Weak signedMessages check: 43-44

if (_signature.length == 0) {
  // Checks for > 0, not threshold
  require(safe.signedMessages(messageHash) != 0, "Hash not approved");

}

Tools Used

Manual Review

Recommended Mitigation Steps

Assessed type

Invalid Validation

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as high quality report

[c4-pre-sort]

raymondfam marked the issue as low quality report

[alex-ppg]

The Warden has made invalid statements and does not understand how the Gnosis Safe IGnosisSafe::signedMessages function works. Specifically:

• A non-zero result of IGnosisSafe::signedMessages would indicate that the actual message has already been pre-approved by all necessary members of the Gnosis Safe. As such, it is secure to consider a non-zero entry as a signed message.
• A hash collision is practically impossible to occur and the hashing structure in use by Brahma is relatively simple. A single dynamic argument is utilized in the hashed payload and it is also independently hashed, meaning any variation of it in length or structure would produce a unique hash.

As a result of the above, I consider this exhibit invalid.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid