ethereum / eth-account

Account abstraction library for web3.py
http://eth-account.readthedocs.io/
MIT License
260 stars 156 forks source link

SignedMessage.messageHash not matching Solidity hash for array types #201

Closed radup1337 closed 2 years ago

radup1337 commented 2 years ago

What was wrong?

When signing a message using sign_message, the messageHash output does not match the Solidity generated hash, but only when one of the 'types' is an array i.e. 'uint256[]' or 'address[]'. Is this a known issue?

Example without array type:

{
  "domain": {
    "chainId": 1,
    "name": "Test",
    "verifyingContract": "0x0000000000000000000000000000000000000000",
    "version": "1"
  },
  "message": {
    "name": "0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2"
  },
  "primaryType": "Person",
  "types": {
    "EIP712Domain": [
      {
        "name": "name",
        "type": "string"
      },
      {
        "name": "version",
        "type": "string"
      },
      {
        "name": "chainId",
        "type": "uint256"
      },
      {
        "name": "verifyingContract",
        "type": "address"
      }
    ],
    "Person": [
      {
        "name": "name",
        "type": "address"
      }
    ]
  }
}

The hashing function in my .sol file is also quite typical:

struct Person{
        address name;
    }

string constant PERSON ="Person(address name)";
bytes32 constant PERSON_TYPEHASH = keccak256(abi.encodePacked(PERSON));
function hashMsg(Person memory person) public view returns (bytes32) {
        return
            keccak256(
                abi.encodePacked(
                    "\x19\x01",
                    DOMAIN_SEPARATOR,
                    keccak256(
                        abi.encode(
                            PERSON_TYPEHASH,
                            person.name
                        )
                    )
                )
            );
    }

This works fine, I get a hash of 0x09eb75cfd1cd5661523c7a3e681f07d57b108a6aff997658939c4dff0006e74a from both the messageHash in Python and my hashed output from my Solidity function.

Example with array type:

{
  "domain": {
    "chainId": 1,
    "name": "Test",
    "verifyingContract": "0x0000000000000000000000000000000000000000",
    "version": "1"
  },
  "message": {
    "name": ["0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2","0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2"]
  },
  "primaryType": "Person",
  "types": {
    "EIP712Domain": [
      {
        "name": "name",
        "type": "string"
      },
      {
        "name": "version",
        "type": "string"
      },
      {
        "name": "chainId",
        "type": "uint256"
      },
      {
        "name": "verifyingContract",
        "type": "address"
      }
    ],
    "Person": [
      {
        "name": "name",
        "type": "address[]"
      }
    ]
  }
}

Solidity function stays the same, just changing from address -> address[]

struct PersonArray {
        address[] name;
    }
string constant PERSON_ARRAY ="Person(address[] name)";
bytes32 constant PERSON_TYPEHASH_ARRAY = keccak256(abi.encodePacked(PERSON_ARRAY));
function hashPersonArray(PersonArray memory person) public view returns (bytes32) {
        return
            keccak256(
                abi.encodePacked(
                    "\x19\x01",
                    DOMAIN_SEPARATOR,
                    keccak256(
                        abi.encode(
                            PERSON_TYPEHASH_ARRAY,
                            person.name
                        )
                    )
                )
            );
    }

In this case, the signed message messageHash = 0x3251bee8474b58a5df1adab9be78e2b18462d7a5ddf423389dcf98042188bef3 and the Solidity generated hash = 0x656f94667281b3e471e7b418a73d8dd8dd1dc8d6c9198fa29daf0d6291178a40

I cannot see why there is a difference..

kclowes commented 2 years ago

what version of eth-account are you using?

radup1337 commented 2 years ago

what version of eth-account are you using?

Latest one, 0.7.0

radup1337 commented 2 years ago

Update: I now managed to get this to work by NOT using the 'is_array_type' check in the 'encode_field' function in 'hashing.py'

Using this version of encode_field, it works just as I needed it to

def encode_field(types, name, field_type, value):

    if value is None:
        raise ValueError(f"Missing value for field {name} of type {field_type}")

    if field_type in types:
        return ('bytes32', keccak(encode_data(field_type, types, value)))

    if field_type == "bytes":
        if not isinstance(value, bytes):
            raise TypeError(
                f"Value of field `{name}` ({value}) is of the type `{type(value)}`, "
                f"but expected bytes value"
            )

        return ('bytes32', keccak(value))

    if field_type == "string":
        if not isinstance(value, str):
            raise TypeError(
                f"Value of field `{name}` ({value}) is of the type `{type(value)}`, "
                f"but expected string value"
            )

        return ('bytes32', keccak(text=value))

    return (field_type, value)
fselmo commented 2 years ago

@radup1337 I'm confused by you closing the issue. Do you still believe there is a bug in the library or were you able to get the results you needed with the latest version?

radup1337 commented 2 years ago

@radup1337 I'm confused by you closing the issue. Do you still believe there is a bug in the library or were you able to get the results you needed with the latest version?

Not sure if it is a bug, what is happening is that is_array_type encodes the array and adds it as type 'bytes32' and its encoded bytes value to the encoded_types and encoded_values arrays, respectively.

so say two types, an int array and an address array, that need to be encoded using eth_abi.encode: The following action should occur: eth_abi.encode( ['uint256[]', 'address[]'], [[1, 2, 3], ['0x...', '0x...']] ) Instead, the following happens eth_abi.encode( ['bytes32', 'bytes32'], [bytes_value, bytes_value] ) I don't know why the arrays are converted to bytes in the first place, but the outputs you get from those two functions above are different and it results in different message_hash results. Shouldn't they be the same (in theory)?

The only way I could get the results I need was by returning (field_type, value) in encode_field when the type is an array type (instead of letting it go through the 'is_array_type' check).

fselmo commented 2 years ago

@radup1337, did you use encode rather than encodePacked in your Solidity example? These can give very different results. I'm also not seeing addresses encoded as bytes32. If you mean in the encode_data(), what returns from encode_field is not the addresses themselves... it is the keccak hash of the encoded addresses that get encoded here and those are indeed being encoded as addresses.

So when we get to encode(["bytes32", "bytes32"], [bytes_value_1, bytes_value_2]) the bytes values are not the addresses but rather the keccak hash of the encoded address values.

I want to know if you can test abi.encode() instead of abi.encodePacked() within your Solidity example and see what you get. Is that a possibility?

radup1337 commented 2 years ago

@fselmo Yes, in Solidity I use the following to hash my message:

keccak256(
                abi.encodePacked(
                    "\x19\x01",
                    DOMAIN_SEPARATOR,
                    keccak256(
                        abi.encode(
                            TYPEHASH,
                            array
                        )
                    )
                )
            );

But again, the issue is that it is working as expected when not using arrays, and it only breaks when using array type

I tried using only abi.encode() in Solidity but it didn't give the right results

fselmo commented 2 years ago

@radup1337, I'm not entirely sure what's going on in your example or what you mean by "right results" but I ran your example against the latest version of eth-sig-utils (the library that Metamask uses), and it is consistent across the board with what eth-account is returning.

To be clear... are you talking about sign_message()? And are you signing with the same private key? [I'm not sure this is relevant]. I don't see any discrepancies comparing encode_structured_data() in eth-account with TypedDataUtils.encodeData() from the latest version of eth-sig-utils using your exact example. All the same values are being returned.


edit: For reference:

encodeData in eth-sig-utils (encode_structured_data in eth-account) result: (0x)33237f1509d805c9d9351a64b57d013d2c086bee444f72faa9be94d7dcdebdb4000000000000000000000000ab8483f64d9c6d1ecf9b849ae677dd3315835cb2

messageHash in eth-sigh-utils (hash_message in eth-account) result: (0x)34b4f7ced8edacb449acc3061a8a1b44dc996724e416734bf32c26fdb9854690

eip712Hash in eth-sig-utils (encode_structured_data + _hash_eip191_message in eth-account): (0x)09eb75cfd1cd5661523c7a3e681f07d57b108a6aff997658939c4dff0006e74a

radup1337 commented 2 years ago

@fselmo That is correct, I have checked against eth-sig-utils as well and it is consistent with eth-account, but both give me the wrong result when compared against Solidity.

To recap, I have a hashing function in Solidity which is equivalent to a EIP 712 Hash, which is what I'm comparing everything else against.

keccak256(
                abi.encodePacked(
                    "\x19\x01",
                    DOMAIN_SEPARATOR,
                    keccak256(
                        abi.encode(
                            OBJECT_TYPEHASH,
                            object
                        )
                    )
                )
            );

When my object is something that is not an array (string, uint256, address, etc), then the hash I get from that function (which indeed is [or should be] the EIP 712 Hash) matches the encode_structured_data + _hash_eip191_message in eth-account. So all is fine up to this point.

If now my Solidity object changes to an array object (string[], uint256[], address[], etc), assuming I have adjusted my OBJECT_TYPEHASH accordingly and everything else stays the same, then I see a discrepancy in the EIP 712 Hashes.

I hope I could explain this better now, but this is my issue. So I am not sure if this is a Solidity issue when encoding arrays or an issue with both libraries.

And again, I got the eth_account version to work by doing what I explained in my first comments.

radup1337 commented 2 years ago

@fselmo Got an update! So I think in the end it was a solidity issue, all I had to do was change my Solidity hashing function to

                abi.encodePacked(
                    "\x19\x01",
                    DOMAIN_SEPARATOR,
                    keccak256(
                        abi.encode(
                            TYPEHASH,
                            keccak256(abi.encodePacked(array))
                        )
                    )
                )
            );

Basically have to encodePacked my array objects in Solidity and now it works! Closing this as it was never a eth-account issue to begin with! I mistakenly assumed it in the beginning.