Incorrect encoding of bytes for EIP712 `digest ` in createDigestExecTx() functions thus not compatible with EIP712

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x3d8e0be59cc1e1447972ce1ffa03b3545e35280084af68c072989160a8637746 Severity: medium

Description: Description\

    function createDigestExecTx(
        bytes32 domainSeparatorSafe,
        Transaction memory safeTx
    ) public view returns (bytes32) {
        bytes32 digest = _hashTypedDataV4(
            domainSeparatorSafe,
            keccak256(
                abi.encode(
                    keccak256(
                        "execTransaction(address to,uint256 value,bytes data,Enum.Operation operation,uint256 safeTxGas,uint256 baseGas,uint256 gasPrice,address gasToken,address refundReceiver,bytes signatures)"
                    ),
                    safeTx.to,
                    safeTx.value,
@>                    safeTx.data,
                    safeTx.operation,
                    safeTx.safeTxGas,
                    safeTx.baseGas,
                    safeTx.gasPrice,
                    safeTx.gasToken,
                    safeTx.refundReceiver,
@>                    safeTx.signatures
                )
            )
        );

        return digest;
    }

According to EIP-712:

The dynamic values bytes and string are encoded as a keccak256 hash of their contents.

Reference link: https://eips.ethereum.org/EIPS/eip-712#definition-of-encodedata

However, the digesthas encoded the bytes data and signature as not keccak256 hashed as per the requirement of EIP-712.

This violates EIP712 requirement and produce incorrect encoding.

Recommendation to fix

Encoding the bytes arguments as a keccak256 hash of its contents before computing the digest from it:

[alfredolopez80]

Non-Issue for several point here:

1. This is a helpers function, is not part of any contract, we only use in very few Unit-test
2. This contract receive a safe.tx fromencodeTransactionData of safe and they hash de data like as you see here: https://github.com/safe-global/safe-smart-account/blob/186a21a74b327f17fc41217a927dea7064f74604/contracts/GnosisSafe.sol#L365 and the unique value different is the signatures, and this is hashed before to call this funtion.

[0xRizwan]

@alfredolopez80 Please find additional context on points raised by you.

This is a helpers function, is not part of any contract, we only use in very few Unit-test

As an auditor, our job is to find bugs and secure the protocol to best we can by taking all security measures. Per the competition page details, SigningUtils.sol is inscope.

Nowhere, it mentioned on contest page to not report issues found in SigningUtils.sol and there are not present an exceptions for this contract on contest page and in Out of scope section.

This contract receive a safe.tx fromencodeTransactionData of safe and they hash de data like as you see here:

Correct, if you see this then the bytes argument is keccak256 while encoding and this entire issue is about same.

I believe, this issue is correct and its break the EIP712 encoding requirement for dynamic type arguments like string, bytes and Historically such issues are confirmed as Medium severity.

Recomendation to fix as follows:

    function createDigestExecTx(
        bytes32 domainSeparatorSafe,
        Transaction memory safeTx
    ) public view returns (bytes32) {
        bytes32 digest = _hashTypedDataV4(
            domainSeparatorSafe,
            keccak256(
                abi.encode(
                    keccak256(
                        "execTransaction(address to,uint256 value,bytes data,Enum.Operation operation,uint256 safeTxGas,uint256 baseGas,uint256 gasPrice,address gasToken,address refundReceiver,bytes signatures)"
                    ),
                    safeTx.to,
                    safeTx.value,
-                   safeTx.data,
+                   keccak256(safeTx.data),
                    safeTx.operation,
                    safeTx.safeTxGas,
                    safeTx.baseGas,
                    safeTx.gasPrice,
                    safeTx.gasToken,
                    safeTx.refundReceiver,
-                   safeTx.signatures
+                   keccak256(safeTx.signatures)
                )
            )
        );

        return digest;
    }

[alfredolopez80]

@0xRizwan i understand your point and if this method it was an integral part of our protocol i totally agree to put this issue like medium issue, but in this case i consider only low is possible!!

[0xRizwan]

@alfredolopez80 Thanks for the understanding. createDigestExecTx() is an external function which is producing incorrect digest due to incorrect coding. This is a non-compliance of EIP-712 and deviates from EIP712 specification. Historically, such issues are considered as Medium severity and Medium severity is truly justified based on issues description and further recommendation to fix it.