[Medium-1] The AccountTx type hash is wrong and will not enable clients to encode their signatures properly

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L45-L48 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L424-L446 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L435

Vulnerability details

Impact

The type hash used for encoding and decoding AccountTx signatures in the SmartAccount wallet is correct but is not encoded the same way than how the type hash was generated. It is likely to confuse anybody interacting with their smart wallet and is also non-EIP712 compliant.

Proof of Concept

See the following Rationale about the typehash for EIP712 signatures: https://eips.ethereum.org/EIPS/eip-712#rationale-for-typehash

An example of a typehash implemented in Solidity:

bytes32 constant MAIL_TYPEHASH = keccak256(
  "Mail(address from,address to,string contents)");

Which is then encoded like so:

function hashStruct(Mail memory mail) pure returns (bytes32 hash) {
    return keccak256(abi.encode(
        MAIL_TYPEHASH,
        mail.from,
        mail.to,
        keccak256(mail.contents)
    ));
}

In the SmartAccount contract, the MAIL_TYPEHASH translates as the ACCOUNT_TX_TYPEHASH and the hashStruct() function can be seen as the encodeTransactionData() function.

The issue is that the ACCOUNT_TX_TYPEHASH is not implemented correctly. In the encodeTransactionData() function.

The transaction data, which have the byte type is hashed using the keccak256 method. On the other hand, the typehash don't use the same type returned from the keccak256 which is bytes32.

AccountTx(address to,uint256 value,bytes data,uint8 operation,uint256 targetTxGas,uint256 baseGas,uint256 gasPrice,address gasToken,address refundReceiver,uint256 nonce)

Tools Used

Manual inspection

Recommended Mitigation Steps

Don't hash the transaction data in the encodeTransactionData() so that it respects the ACCOUNT_TX_TYPEHASH.

function encodeTransactionData(
Transaction memory _tx,
  FeeRefund memory refundInfo,
  uint256 _nonce
) public view returns (bytes memory) {
  bytes32 safeTxHash =
    keccak256(
      abi.encode(
        ACCOUNT_TX_TYPEHASH,
        _tx.to,
        _tx.value,
        _tx.data,
        _tx.operation,
        _tx.targetTxGas,
        refundInfo.baseGas,
        refundInfo.gasPrice,
        refundInfo.gasToken,
        refundInfo.refundReceiver,
        _nonce
      )
    );
  return abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator(), safeTxHash);
}

[c4-judge]

gzeon-c4 marked the issue as primary issue

[livingrockrises]

lack of proof

[livingrockrises]

as transactions do go through properly.

[c4-sponsor]

livingrockrises marked the issue as sponsor acknowledged

[livingrockrises]

@tomarsachin2271

[c4-sponsor]

livingrockrises requested judge review

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)

[c4-judge]

gzeon-c4 marked the issue as grade-b

[iFrostizz]

Actually @gzeon-c4 , it's indeed implemented as it should. Please invalidate my issue.

[c4-judge]

gzeon-c4 marked the issue as grade-c

[gzeon-c4]

Actually @gzeon-c4 , it's indeed implemented as it should. Please invalidate my issue.

Thanks for the confirmation, just re-read https://eips.ethereum.org/EIPS/eip-712#rationale-for-typehash and confirm it is implemented correctly.