Trezor can't make transaction with chainid more than 2000

ghost commented 6 years ago

Tested with our ethereum private chain with MyEtherWallet, it errors with insufficient gas when I sign and deploy transactions with trezor ( When I lower the chainid it came back to normal )

prusnak commented 6 years ago

I really doubt this is a TREZOR issue. Are you setting gas correctly?

ghost commented 6 years ago

@prusnak Yes it only happens when I use trezor, wallet with private key or json file doesn't effect this problem

prusnak commented 6 years ago

Which firmware version do you have?

prusnak commented 6 years ago

Isn't this a duplicate of https://github.com/trezor/connect/issues/139 by any chance?

ghost commented 6 years ago

@prusnak Might be... Investigating this issue, I am using the latest version of trezor one firmare

aerth commented 6 years ago

I think I have ran into this issue. Using aquachain's ChainID 61717561 returns this funky situation where the transaction's from field is switched to a random other address. If you click send, the error appears insufficient gas fee * price and whatnot.

This happens using Geth, MEW and MyCrypto, in combination with trezor

Can we isolate this to MEW/MC or is there a limit on the device? thanks.

Signing through metamask via MEW/MC works as expected.

In this image you can see my transaction from 0xA4 to 0xA4, but the from field gets switched (only after signing with trezor)

signing-from-wrong-account

hackmod commented 6 years ago

chain_id > 256 does not work correctly.

for examples (based on https://github.com/trezor/connect/blob/v4/examples/signtx-ethereum.html )

chainId == 255 case (working case)

(like as @aerth 's example, to address is the same from address)

<!DOCTYPE html>
<html>
  <head>
    <title>TREZOR Sign Transaction Test</title>
    <script>

     function trezorSignTx() {
         // spend one change output
         var address_n = "m/44'/255'/0'/0/0";
         var chainId = 255;

         // var address_n = [44 | 0x80000000,
         //                  60 | 0x80000000,
         //                  0  | 0x80000000 ,
         //                  0 ]; // same, in raw form

         var nonce = '04'; // note - it is hex, not number!!!
         var gas_price = '0861c46800';
         var gas_limit = '5208';
         var to = 'e878e39e3BB2276f535D40aD2DC900d6d1a21B8b'; // 255
         var value = "2386f26fc10000"; // in hexadecimal, in wei - this is about 18 ETC
         var data = null  // for no data
         var chain_id = chainId; // 1 for ETH, 61 for ETC

         TrezorConnect.ethereumSignTx(
            address_n,
            nonce,
            gas_price,
            gas_limit,
            to,
            value,
            data,
            chain_id,
            function (response) {
             if (response.success) {
                 console.log('Signature V (recovery parameter):', response.v); // number
                 console.log('Signature R component:', response.r); // bytes
                 console.log('Signature S component:', response.s); // bytes
             } else {
                 console.error('Error:', response.error); // error message
             }
             response.value = value;
             response.v = response.v.toString(16);
             response.nonce = nonce;
             response.gasLimit = gas_limit;
             response.gasPrice = gas_price;
             response.to = to;
             document.getElementById("response").innerHTML = JSON.stringify(response, undefined, 2);
         }, '1.4.2');
     }
    </script>
  </head>
  <body>
    <button onclick="trezorSignTx()">Sign</button>
    <pre id="response"></pre>
    <script src="../connect.js"></script>
  </body>
</html>

now r, s are obtained and we can check r,s using ethereumjs-tx

const ethTx = require('ethereumjs-tx');

var chainId = 255;

const txParams =
{"nonce":"0x04","gasPrice":"0x0861c46800","gasLimit":"0x5208",
"value":"0x2386f26fc10000","data":"0x",
"to":"0xe878e39e3BB2276f535D40aD2DC900d6d1a21B8b",
"chainId":chainId,
"v":"0x222",
"r":"0x7f723e4cb79e807434e582a8f37a37aa5e5fe4b7acda21ccf40fc7959464be1a",
"s":"0x75d53521e50ba32416872453cf5bc10786d849f626d3687f442363993000186a"};
// Transaction is created
const tx = new ethTx(txParams);

const serializedTx = tx.serialize();
const rawTx = '0x' + serializedTx.toString('hex');
var ret = tx.verifySignature();
console.log('verifySignature() = ', ret);
console.log('SenderAddress = ' + tx.getSenderAddress().toString('hex')); // sender matched!

console.log(rawTx)

sender(from address) is matched. works nicely.

chainId == 256 case (not working)

--- dist/connect/examples/signtx-255.html       2018-07-14 08:03:52.598101677 +0900
+++ dist/connect/examples/signtx-256.html       2018-07-14 08:10:37.655018941 +0900
@@ -6,8 +6,8 @@

      function trezorSignTx() {
          // spend one change output
-         var address_n = "m/44'/255'/0'/0/0";
-         var chainId = 255;
+         var address_n = "m/44'/256'/0'/0/0"; // not work
+         var chainId = 256;

          // var address_n = [44 | 0x80000000,
          //                  60 | 0x80000000,

check r,s

const ethTx = require('ethereumjs-tx');

var chainId = 256;

const txParams =
{"nonce":"0x04","gasPrice":"0x0861c46800","gasLimit":"0x5208",
"value":"0x2386f26fc10000","data":"0x",
"to":"0x522632e07658A60488B49f00E08cd96B5596fE44",
"chainId":chainId,
"v":"0x223",
"r":"0x2d628da50cf5f8d0ed7572ec83988b5fc4615e7378bc74a534ae9e18bd79169e",
"s":"0x70324d888dc214e51997b2bbf6df37411362875405cb3d9d94ef9112f080cad8"};
// Transaction is created
const tx = new ethTx(txParams);

const serializedTx = tx.serialize();
const rawTx = '0x' + serializedTx.toString('hex');
var ret = tx.verifySignature();
console.log('verifySignature() = ', ret);
console.log('SenderAddress = ' + tx.getSenderAddress().toString('hex')); // sender not matched!

console.log(rawTx)

as @aerth already mentioned, sender(from) address is mismatched.

hackmod commented 6 years ago

I just looked over the source and found out the following line https://github.com/trezor/trezor-mcu/blob/master/firmware/ethereum.c#L571 (Im curious about the following line is correct.)

diff --git a/firmware/ethereum.c b/firmware/ethereum.c
index 87b7f52..b71001a 100644
--- a/firmware/ethereum.c
+++ b/firmware/ethereum.c
@@ -568,7 +568,7 @@ void ethereum_signing_init(EthereumSignTx *msg, const HDNode *node)
                rlp_length += rlp_calculate_length(1, tx_type);
        }
        if (chain_id) {
-               rlp_length += rlp_calculate_length(1, chain_id);
+               rlp_length += rlp_calculate_length(msg->has_chain_id && chain_id > 0xff ? 2 : 1, chain_id);
                rlp_length += rlp_calculate_length(0, 0);
                rlp_length += rlp_calculate_length(0, 0);
        }

rlp_length affect continuously at hash_rlp_list_length() => hash_data() => sha3_Update()

update (7/16)

a custom firmware with above fix for chainId > 255 https://github.com/trezor/trezor-mcu/pull/381

prusnak commented 6 years ago

Closed in favor of https://github.com/trezor/trezor-mcu/pull/381

prusnak commented 6 years ago

@eosclassicteam @aerth you are encouraged to help @hackmod with creating the test vectors for https://github.com/trezor/trezor-mcu/pull/381 so we can properly fix the issue

trezor / connect