Protocol is not EIP712 compliant: incorrect typehash for Validation and Transaction structures

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/libraries/TypeHashHelper.sol#L45-L50
https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/libraries/TypeHashHelper.sol#L52-L57

Vulnerability details

Summary

When implementing EIP712, among other things, for data structures that will be a part of signing message a typehash must be defined.

The structure typehash is defined as:

typeHash = keccak256(encodeType(typeOf(s)))

• where encodeType is the type of a struct that is encoded as:

name ‖ "(" ‖ member₁ ‖ "," ‖ member₂ ‖ "," ‖ … ‖ memberₙ ")"

• and each member is written as:

type ‖ " " ‖ name.

The project uses 2 structures on the signed data Transaction and Validation declared in TypeHashHelper:

    /**
     * @notice Structural representation of transaction details
     * @param operation type of operation
     * @param to address to send tx to
     * @param account address of safe
     * @param executor address of executor if executed via executor plugin, address(0) if executed via execTransaction
     * @param value txn value
     * @param nonce txn nonce
     * @param data txn callData
     */
    struct Transaction {
        uint8 operation;
        address to;
        address account;
        address executor;
        uint256 value;
        uint256 nonce;
        bytes data;
    }


    /**
     * @notice Type of validation struct to hash
     * @param expiryEpoch max time till validity of the signature
     * @param transactionStructHash txn digest generated using TypeHashHelper._buildTransactionStructHash()
     * @param policyHash policy commit hash of the safe account
     */
    struct Validation {
        uint32 expiryEpoch;
        bytes32 transactionStructHash;
        bytes32 policyHash;
    }

However, the precalculated typehash for each of the structure are of different structures:

• for Transaction the hash is actually calculated on a now removed ExecutionParams structure:

    /**
     * @notice EIP712 typehash for transaction data
     * @dev keccak256("ExecutionParams(address to,uint256 value,bytes data,uint8 operation,address account,address executor,uint256 nonce)");
     */
    bytes32 public constant TRANSACTION_PARAMS_TYPEHASH =
        0xfd4628b53a91b366f1977138e2eda53b93c8f5cc74bda8440f108d7da1e99290;

• for Validation the hash is calculated on another old, removed structure ValidationParams:

    /**
     * @notice EIP712 typehash for validation data
     * @dev keccak256("ValidationParams(ExecutionParams executionParams,bytes32 policyHash,uint32 expiryEpoch)ExecutionParams(address to,uint256 value,bytes data,uint8 operation,address account,address executor,uint256 nonce)")
     */
    bytes32 public constant VALIDATION_PARAMS_TYPEHASH =
        0x0c7f653e0f641e41fbb4ed1440c7d0b08b8d2a19e1c35cfc98de2d47519e15b1;

POC

The issue went undetected in the initial development phase, and can be verified, because the tests still use the old hashes:

    function testValidateConstants() public {
        assertEq(
            TypeHashHelper.TRANSACTION_PARAMS_TYPEHASH,
            keccak256(
                "ExecutionParams(address to,uint256 value,bytes data,uint8 operation,address account,address executor,uint256 nonce)"
            )
        );
        assertEq(
            TypeHashHelper.VALIDATION_PARAMS_TYPEHASH,
            keccak256(
                "ValidationParams(ExecutionParams executionParams,bytes32 policyHash,uint32 expiryEpoch)ExecutionParams(address to,uint256 value,bytes data,uint8 operation,address account,address executor,uint256 nonce)"
            )
        );
    }

The specific test can be verified by running:

forge test --fork-url "https://eth-mainnet.g.alchemy.com/v2/<ALCHEMY_API_KEY>" -vvv --ffi --match-test testValidateConstants

Impact

Protocol is not EIP712 compliant which will result in issues with integrators.

Tools Used

Manual review and an online keccak256 checker for validating that the those hashes are not actually for the correct structures.

Recommendations

Modify the structure typehash (and tests) to point to the correct structures.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as high quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-sponsor]

0xad1onchain (sponsor) confirmed

[alex-ppg]

The Warden has demonstrated that the contract is non-compliant with EIP-712 as the type-hashes defined within it are outdated and incorrect.

The Warden has articulated what the potential impact is and, while slightly brief, the recommendation the Warden provided is correct.

As a result, I fully accept this submission as satisfactory as well as the best of its kind. #182 will be awarded partial credit given that it managed to pinpoint a single flaw in a single type-hash rather than identify that multiple type hashes were incorrect.

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg marked the issue as selected for report

[0xad1onchain]

Fixed, amended EIP 712 struct