_data.length is not being checked against _signature.length in 'isValidSignature', which can lead to unexpected behavior

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/ConsoleFallbackHandler.sol#L39-L55

Vulnerability details

Impact

In ConsoleFallbackHandler.isValidSignature, bytes memory _data and bytes memory _signature are being used to validate the signature of the callers. This function makes a call to GnosisSafe.checkSignatures.

source: contracts/lib/safe-contracts/contracts/handler

function isValidSignature(bytes calldata _data, bytes calldata _signature) public view override returns (bytes4) {
//.. ommitted code
@>            safe.checkSignatures(messageHash, _data, _signature);
//.. ommitted code
    }

However, nowhere in the isValidSignature is _data.length checked against _signature.length. This can lead to unexpected behaviour.

The Safe team has fixed this bug in V1.4.0. This is the PR. Unfortunately, Brahma uses Safe V1.3.0, which does not include this fix.

Tools Used

Manual Review

Recommended Mitigation Steps

Use an updated version of Safe.

Assessed type

Invalid Validation

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as duplicate of #16

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-sponsor]

0xad1onchain (sponsor) disputed

[0xad1onchain]

Invalid Please note that 1271 signature method is function isValidSignature(bytes32 _dataHash, bytes calldata _signature) external view returns (bytes4) and not function isValidSignature(bytes memory _data, bytes memory _signature) public view override returns (bytes4)

The method stated in the issue will be called internally by the correct the other isValidSignature method. Also please articulate more on unexpected behavior, while validation is nice to have, signature can be totally empty for a 1271 signature and still be fine as long as data hash is presigned.

[alex-ppg]

The Warden's submission appears to be somewhat incorrect, however, the code they are highlighting is of merit as it does actually showcase a potential v1.4.0+ incompatibility.

The ConsoleFallbackHandler::isValidSignature function that accepts a bytes memory argument is actually a "legacy" version of EIP-1271. The current implementation in Brahma is actually slightly incompatible with Gnosis Safe versions v1.4.0 and v1.4.1 at the time of writing.

The problem lies in that the PR and addition the Warden has referenced is in relation to the data and dataHash arguments of the GnosisSafe::checkSignatures function. In detail, the signature validation portion of GnosisSafe will mandate that the dataHash is an actual hash of the data when dealing with contract signatures.

Given that the ConsoleFallbackHandler::isValidSignature function will craft a special messageHash payload that is not an actual hash of the _data passed in to the GnosisSafe::checkSignatures function, the ConsoleFallbackHandler::isValidSignature may fail for a "correct" signature fatally.

This will solely occur when one of the owners of the multi-signature wallet is a smart contract and the signatures payload specifies that owner as a signer. Nevertheless, it is an actual issue as the legacy EIP-1271 version of the GnosisSafe will not behave as expected.

I will classify this as a "Low" issue given that it manifests under very specific circumstances, and has limited ramifications.

In relation to whether this exhibit will be rewarded, I will revisit at a later date as my desire is to slash the reward (given that the actual vulnerability was not identified) but still reward partial credit as the code segments as well as resources were correct and can lead to an identification of the vulnerability relatively easily.

[c4-judge]

alex-ppg marked the issue as primary issue

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)

[alex-ppg]

My initial inclination was to provide some form of award for being on the right track, however, it would not be justifiable given that the submission is generally of low quality, over-inflated, and completely missed the mark.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid

[c4-judge]

This previously downgraded issue has been upgraded by alex-ppg

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid

[alex-ppg]

This issue had to be upgraded from QA to Medium in order for the unsatisfactory flag to be properly consumed.