Brahma - 0xmystery's results

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/libraries/TypeHashHelper.sol#L47 https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/libraries/TypeHashHelper.sol#L70

Vulnerability details

Impact

The discovered vulnerability poses a medium risk due to the inconsistency between the expected data type in the TRANSACTION_PARAMS_TYPEHASH and the actual type used in the _buildTransactionStructHash function. Specifically, the hashing of txn.data as bytes32 when the comment suggests it should be bytes could lead to unexpected behavior or security issues, particularly if this function's output is used in signature generation or verification per EIP-712 standards.

This discrepancy might not only lead to failed transaction authentication or replay attacks but also create potential disputes during transaction verification processes, especially in a multisig contract setting. The issue can cause significant logical inconsistencies in any system that interacts with this contract expecting EIP-712 compliance, potentially leading to a loss of trust if transactions are incorrectly deemed invalid, or worse, if invalid transactions are considered valid.

Proof of Concept

Function _buildTransactionStructHash hashes txn.data as bytes32:

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/libraries/TypeHashHelper.sol#L64-L77

    function _buildTransactionStructHash(Transaction memory txn) internal pure returns (bytes32) {
        return keccak256(
            abi.encode(
                TRANSACTION_PARAMS_TYPEHASH,
                txn.to,
                txn.value,
@ audit         keccak256(txn.data),
                txn.operation,
                txn.account,
                txn.executor,
                txn.nonce
            )
        );
    }

The comment indicating the data should be bytes:

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

    /**
     * @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;

This involves the discrepancy between the code's implementation and the comments, suggesting a misunderstanding or miscommunication among the developers, which could lead to potential bugs or vulnerabilities.

Tools Used

Manual

Recommended Mitigation Steps

Clarify the Intended Functionality: The team needs to discuss and agree upon the correct behavior of the _buildTransactionStructHash function. Determine whether txn.data should be hashed before being encoded or if it should be included as bytes.

Update the Code or Comment: Depending on the decision above, either the function implementation needs to be updated to handle txn.data as bytes, or the comment (and any corresponding documentation) needs to be corrected to indicate that data is hashed to bytes32.

Assessed type

Other