hw-app-eth does not support EIP155 for chainId >255

[hackmod]

1. In most case of ethereum variants, chainId != networkId (networkId is not used at all in the signing process. ony the chainId is used for signing)
2. in the EIP155 ([6]) discussion. chainId is not only one byte. after Arachnid's suggestion chainID > 1-byte also supported now as a defacto standard.

to support chainId > 255 cases the following FIXME annotated line should be fixed.
https://github.com/LedgerHQ/ledgerjs/blob/master/packages/web3-subprovider/src/index.js#L160

...
      // EIP155: v should be chain_id * 2 + {35, 36}
      const signedChainId = Math.floor((tx.v[0] - 35) / 2);
      const validChainId = networkId & 0xff; // FIXME this is to fixed a current workaround that app don't support > 0xff
      if (signedChainId !== validChainId) {
        throw makeError(
          "Invalid networkId signature returned. Expected: " +
            networkId +
            ", Got: " +
            signedChainId,
          "InvalidNetworkId"
        );
      }

...

NetworkID	CHAIN_ID	Chain(s)
1	1	Ethereum mainnet
2	2	Morden (disused)
3	3	Ropsten
4	4	Rinkeby
?	30	Rootstock mainnet
?	31	Rootstock testnet
?	42	Kovan
1	61	Ethereum Classic mainnet
3	62	Ethereum Classic testnet
1	2	Expanse mainnet ([1], [2], [3])
1	8	Ubiq mainnet ([2] , [3])
1	820	Callisto mainnet ([2] , [3])
?	1337	Geth private chains (default) [4]

[1] https://github.com/expanse-org/go-expanse/blob/master/eth/config.go#L42
[2] https://www.ethernodes.org/network/1/nodes
[3] https://github.com/kvhnuke/etherwallet/blob/mercury/app/scripts/nodes.js
[4] https://github.com/ethereum/EIPs/blob/master/EIPS/eip-155.md
[5] ethereum/EIPs#155
[6] https://github.com/LedgerHQ/ledgerjs/blob/master/packages/web3-subprovider/src/index.js#L160

[ghost]

Trezor doesn't support chainid more than 2000 also. It is a normal thing to use chainid smaller than 20 on ethereum networks

[gre]

just to be clear, the Ledger device does work with chainId>255 (for instance I was playing with 1337 recently on a DApp and with my Nano S). But using this workaround, the issue is that you have potential collisions: you can sign something that can be broadcasted on a network with the same chainId modulus 256.
to be confirmed but I think @btchip is aware of the present problem and there will be a solution to support more than one-byte chainId. (maybe it's even already possible, i need to discuss more in detail with the hardware team)

[hackmod]

Similar issue has been resolved at the firmware level of the Trezor trezor/trezor-mcu#381

I strongly suspect that the following lines have this issue.

• https://github.com/LedgerHQ/blue-app-eth/blob/master/src_common/ethUstream.h#L58-L59
• https://github.com/LedgerHQ/blue-app-eth/blob/master/src_common/ethUstream.c#L23
• https://github.com/LedgerHQ/blue-app-eth/blob/master/src_genericwallet/main.c#L1902-L1918

[hackmod]

the latest ledger correctly support 32bits chainId + EIP155 :)

[gre]

@hackmod neat. but it's still to be implemented on the js side I guess. what's kinda tricky is code won't be backward compatible I guess. unless we check the version 🤔

[btchip]

It was already ok on the device side before the new ETH app which introduced a regression by validating the chainId for the currently run app.

On the JS side the best solution is to only use v coming from the device to get the signature "parity" then recompute it according to EIP 155

[hackmod]

this is a quick hack

• MyEtherwallet - support 32bits chainId with the latest Ledger app MyEtherWallet/etherwallet#1979
• ClassicEtherWallet - support 32bits chainId with the latest Ledger app EthereumCommonwealth/etherwallet#202
• MyCrypto - support 32bits chainid with the latest Ledger app MyCryptoHQ/MyCrypto#2094

diff --git a/app/scripts/uiFuncs.js b/app/scripts/uiFuncs.js
index 6d09394..4af38e5 100644
--- a/app/scripts/uiFuncs.js
+++ b/app/scripts/uiFuncs.js
@@ -56,7 +56,7 @@ uiFuncs.signTxTrezor = function(rawTx, txData, callback) {
     );
 }
 uiFuncs.signTxLedger = function(app, eTx, rawTx, txData, old, callback) {
-    eTx.raw[6] = Buffer.from([rawTx.chainId]);
+    eTx.raw[6] = rawTx.chainId; // ethUtil.rlp.encode() can manage it.
     eTx.raw[7] = eTx.raw[8] = 0;
     var toHash = old ? eTx.raw.slice(0, 6) : eTx.raw;
     var txToSign = ethUtil.rlp.encode(toHash);
@@ -69,7 +69,17 @@ uiFuncs.signTxLedger = function(app, eTx, rawTx, txData, old, callback) {
             });
             return;
         }
-        rawTx.v = "0x" + result['v'];
+        var v = result['v'].toString(16);
+        if (!old) {
+            // EIP155 support. check/recalc signature v value.
+            var rv = parseInt(v, 16);
+            var cv = rawTx.chainId * 2 + 35;
+            if (rv !== cv && (rv & cv) !== rv) {
+                cv += 1; // add signature v bit.
+            }
+            v = cv.toString(16);
+        }
+        rawTx.v = "0x" + v;
         rawTx.r = "0x" + result['r'];
         rawTx.s = "0x" + result['s'];
         eTx = new ethUtil.Tx(rawTx);

[gre]

@hackmod thanks for the references! 🙌