trezor / python-trezor

:snake: Don't use this repo, use the new monorepo instead:
https://github.com/trezor/trezor-firmware
GNU Lesser General Public License v3.0
201 stars 194 forks source link

Ethereum transaction signature problem #236

Closed CryptoIR closed 6 years ago

CryptoIR commented 6 years ago

Hi, It seems like when moving to python3 the ETH signature signed by the code is wrong (at least to me). Maybe the differences in decode/encoding methods between python2 and python3

I'm referring to this part of the code:

sig = client.ethereum_sign_tx(
        n=address_n,
        nonce=nonce,
        gas_price=gas_price,
        gas_limit=gas_limit,
        to=to_address,
        value=value,
        data=data,
        chain_id=chain_id)

    transaction = rlp.encode(
        (nonce, gas_price, gas_limit, to_address, value, data) + sig)

Something like this resolved me the issue

screen shot 2018-03-13 at 17 16 43

Did anyone else encounter it or is it just my environment? Can you please comment?

matejcik commented 6 years ago

Far as I can tell, your diff is a no-op. The result should be the same. The signature values are byte buffers, which you're converting to int. However, rlp doesn't care about the distinction and encodes the values as bytes again.

You can check this pretty easily: duplicate the piece of code and print out tx_hex for unmodified code and after your modifications. Then see if the results differ.

For a minimal case, you can examine the result of rlp.encode(sig_r) before and after your conversion to int.

CryptoIR commented 6 years ago

Before the conversion sig_r is a bytearray and just an integer after. Could this be the issue when using rlp?

After rpl.encode I get completely different signature (with and without the change) The bottom line is that the eth transaction fails without the fix and works after.

Can you also try?

prusnak commented 6 years ago

AFAIK rlp is not Py3 compatible. If that's true, the issue needs to be addressed there and python-trezor requirement set to the fixed version.

matejcik commented 6 years ago

rlp is python3 compatible all right, but it wrongly differentiates between bytes and bytearray.

I'm not sure what behavior the upstream intended, but it's different between py2 and py3, hence https://github.com/ethereum/pyrlp/pull/49

We could also fix this on our side by explicitly converting the bytearrays to bytes when returning the signature. As bytearrays are mutable, that might even be a good idea.

prusnak commented 6 years ago

@matejcik Let's wait if they merge the fix and push it to PyPI (in, let's say, a few days). If not, I propose to fix it in our code like you suggest.

matejcik commented 6 years ago

@prusnak I'll do some refactoring of our code in the meantime, and if it turns out I like returning bytes instead of bytearrays, I'll do that too. Better to be doubly sure :)

prusnak commented 6 years ago

:+1:

CryptoIR commented 6 years ago

Thanks guys!

jhoenicke commented 6 years ago

A difference is that the integer variant strips leading zeros. I was always under the impression that r and s should be encoded as 32-byte buffers with leading zeros, but checking the yellow paper again it seems I was wrong. Is this a bug in the Trezor code?

It should not be too difficult to brute-force a transaction where the signature has a leading zero byte to test his.

matejcik commented 6 years ago

this should now be fixed on our side as well.

purpleeggplant commented 6 years ago

I'm still getting the following error when trying to post signed transactions on Etherscan (http://ropsten.etherscan.io/pushtx) (Ethereum Ropsten).

Error! Unable to broadcast Tx : {"jsonrpc":"2.0","error":{"code":-32602,"message":"Invalid RLP.","data":"RlpExpectedToBeData"},"id":1}

Posting on MyEtherWallet (#offline-transaction) provides a similar error:

rlp: expected input string or byte for *big.Int, decoding into (types.Transaction)(types.txdata).R

As far as I can tell, all my dependencies are good. I'm using a Raspberry Pi3. I am able to complete this operation successfully on MacOs computer without issues. The main difference i see is the MacBook is running Eth-Hash 0.1.2 instead of 0.1.4 (on the Rasp Pi). Although at second glance, it looks like ethereum reverted back from 2.3.1 to 1.0.8.

purpleeggplant commented 6 years ago

manually installing eth-hash to 0.1.2 and ethereum to 2.3.1 doesn't help. All the packages are the same yet running the exact same command outputs two different Signed Raw Transactions.