Arbitrary transactions possible due to insufficient signature validation

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/ConsoleFallbackHandler.sol?plain=1#L39-L54

Vulnerability details

Impact

An attacker can create arbitrary transaction by evading signature validation.

Proof of Concept

The protocol supports contract signed transactions (EIP-1271). The support is implemented as we can see in the isValidSignature call
The isValidSignature function does a thorough check in order to be certain a signature is valid but in this case there is a bug in the checkSignatures which is called by the isValidSignature function.

https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/ConsoleFallbackHandler.sol#L52C1-L53C10

            safe.checkSignatures(messageHash, _data, _signature);

The checkSignatures function inside the Safe contracts, does not check whether the signer is actually the contract owner in a acase where v=0. One might argue that the checkSignatures function is presently out of scope but this is still a valid bug as the isValidSignature in the ConsoleFallbackHandler calls the checkSignatures function in order to verify if a signature is truly valid.

https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/lib/safe-contracts/contracts/GnosisSafe.sol#L240C5-L240C31

  function checkSignatures(
        bytes32 dataHash,
        bytes memory data,
        bytes memory signatures
    ) public view {
        // Load threshold to avoid multiple storage loads
        uint256 _threshold = threshold;
        // Check that a threshold is set
        require(_threshold > 0, "GS001");
        checkNSignatures(dataHash, data, signatures, _threshold);
    }

    function checkNSignatures(
....
 if (v == 0) {
                // If v is 0 then it is a contract signature
                // When handling contract signatures the address of the contract is encoded into r
                currentOwner = address(uint160(uint256(r)));

A hacker can craft a signature that bypasses the signature requirements and sets a hacker controlled signer that will always return EIP1271_MAGIC_VALUE from the isValidSignature. If exploited, the isValidSignature will end up allowing a forged signature from a fake signer. Any Contract that make calls to the isValidSignature function will be affected as well because checkSignatures does not validate that the signer is the caller or owner of the contract.
This impacts the execTransaction function in the GnosisSafe contracts most especially.

Tools Used

Manual Review

Recommended Mitigation Steps

Properly validate that the signer is the owner in the checkSignatures function in order to keep the isValidSignature function in the ConsoleFallbackHandler safe.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #23

[c4-pre-sort]

raymondfam marked the issue as not a duplicate

[c4-pre-sort]

raymondfam marked the issue as duplicate of #46

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as primary issue

[alex-ppg]

This exhibit is incorrect as the Warden specifies that a v value of 0 in a signature payload would not validate that the smart contract the EIP-1271 signature is validated on is an owner.

This statement is incorrect as the second-to-last statement of the for loop within the GnosisSafe::checkNSignatures function will ensure that the owners[currentOwner] is a non-zero address (i.e. an owner of the multi-signature wallet).

As such, this exhibit is invalid.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid

[shealtielanz]

Hello @alex-ppg
i strongly believe the warden is correct here, according to your statement

This statement is incorrect as the second-to-last statement of the for loop within the GnosisSafe::checkNSignatures function will ensure that the owners[currentOwner] is a non-zero address (i.e. an owner of the multi-signature wallet).

the line

 require(currentOwner > lastOwner && owners[currentOwner] != address(0) && currentOwner != SENTINEL_OWNERS, "GS026");

just checks that the address is not a zero address, but when v = 0 it doesn't check that the caller(msg.sender ) is an owner or is approved just like the check made when v = 1,

            } else if (v == 1) {
                // If v is 1 then it is an approved hash
                // When handling approved hashes the address of the approver is encoded into r
                currentOwner = address(uint160(uint256(r)));
                // Hashes are automatically approved by the sender of the message or when they have been pre-approved via a separate transaction
                require(msg.sender == currentOwner || approvedHashes[currentOwner][dataHash] != 0, "GS025");

meaning an attacker can simply create a multi-sig contract of his own and bypass the signature validation. since when v = 0 it doesn't check that the caller is actually the currentOwner or is approved. it just checks that it's not a zero address and the attackers contract is a normal address.

A similar issue was found in here in biconomy contest
please read that report and its mitigation to get a good context of what this warden is saying here.

[alex-ppg]

Hey @shealtielanz, thanks for bringing this to my attention!

I believe you are misinterpreting the referenced check as well as what the original Biconomy contest issue was about.

I will preface this by saying that the code is out-of-scope as it is Gnosis Safe code. In the Biconomy contest, the relevant checkSignaturesfunctions were missing validation of the currentOwner. As the non-duplicate submission Biconomy#175 says:

checkSignature DOES NOT Validate that the _signer or caller is the owner of the contract.

This assumption does not stand in the Brahma contest. The line you have referenced:

require(currentOwner > lastOwner && owners[currentOwner] != address(0) && currentOwner != SENTINEL_OWNERS, "GS026");

Does not simply check that the currentOwner is non-zero. It validates that the entry within owners is non-zero for the key currentOwner.

As such, the currentOwner who signs the message in the smart contract signature verification flow is validated as an actual owner of the Gnosis Safe.

Please let me know if the above address your concerns!