Ethereum transaction signing issues about trezor.net

[MelbourneDeveloper]

Great!

The whole area of code needs to be rewritten. I only wrote it in one weekend. It can also be performance optimized. It was a literal translation from Java and I took shortcuts because I didn't understand Java too well. When I have time, I will clean this up and make it more readable.

[juanfranblanco]

All ok on my side, see also the final tests for Message Signing, Ether Transaction and Token Transaction. I need to do further checks on 155 signatures and big data elements. But I don't think it will be an issue. Next step to integrate in Nethereum like ledger :) :)

[MelbourneDeveloper]

@juanfranblanco @ThatSlyGuy incidentally, transaction signing now works on KeepKey (KeepKey.Net). It's basically all the same code.

[juanfranblanco]

@ThatSlyGuy @MelbourneDeveloper
Hi I have not managed to sign a transaction successfully yet, banging my head on combinations. Well I do sign stuff, but it is incorrect the response in comparison with Nethereum, so some of the parameters are not passed correctly.

Signing messages work, not transactions :) I moved to messages to have a simple test case to start with.

This is message signing using Nethereum

 var message = new EthereumSignMessage
                        {
                            AddressNs = KeyPath.Parse("m/44'/60'/0'/0/0").Indexes,
                            Message = Encoding.UTF8.GetBytes("Hello").ToHex().ToHexBytes()
                        };

                        var messageSignature = await trezorManager.SendMessageAsync<EthereumMessageSignature, EthereumSignMessage>(message);

                        //Same as first address 
                        Console.WriteLine(ToHex(messageSignature.Address));

                        var signer = new Nethereum.Signer.EthereumMessageSigner();
                        var messageSignature2 = signer.EncodeUTF8AndSign("Hello", new EthECKey(
                            "0x2e14c29aaecd1b7c681154d41f50c4bb8b6e4299a431960ed9e860e39cae6d29"));

And this is where I am with transaction signing, GasLimit is recognised and address correctly, from the response, but it does not recognise the value.

  var txMessage = new EthereumSignTx
                        {
                            Nonce = 10.ToBytesForRLPEncoding().ToHex().ToHexBytes(),
                            GasPrice = 10.ToBytesForRLPEncoding().ToHex().ToHexBytes(),
                            GasLimit = 21000.ToBytesForRLPEncoding().ToHex().ToHexBytes(),
                            To = "689c56aef474df92d44a1b70850f808488f9769c".ToHexBytes(),
                            Value = BigInteger.Parse("10000000000000000000").ToBytesForRLPEncoding().ToHex().ToHexBytes(),
                            AddressNs = KeyPath.Parse("m/44'/60'/0'/0/0").Indexes,
                            //ChainId = 1
                        };
                        var transaction = await trezorManager.SendMessageAsync<EthereumTxRequest, EthereumSignTx>(txMessage);

[mtleliever]

@MelbourneDeveloper This is great! The changes you made in the issue10 branch seemed to have completely fixed everything, thanks a lot! Everything seems to be correctly and consistently displayed to the Trezor.

@juanfranblanco The UNKN thing is just a result of not including the ChainId in the EthereumSignTx initialization. If the ChainId is set to 1, then the Trezor displays ETH as the currency. If the ChainId is set to 4 (rinkeby), then the Trezor displays tETH. When no ChainId is included, it is not serialized, which results in the currency type appearing as UNKN.

        [TestMethod]
        public async Task SignEthereumTransaction2()
        {
            await GetAndInitialize();

            var txMessage = new EthereumSignTx
            {
                Nonce = 10.ToBytesForRLPEncoding().ToHex().ToHexBytes(),
                GasPrice = 1000000000.ToBytesForRLPEncoding().ToHex().ToHexBytes(),
                GasLimit = 21000.ToBytesForRLPEncoding().ToHex().ToHexBytes(),
                To = "689c56aef474df92d44a1b70850f808488f9769c".ToHexBytes(),
                Value = BigInteger.Parse("10000000000000000000").ToBytesForRLPEncoding().ToHex().ToHexBytes(),
                AddressNs = KeyPath.Parse("m/44'/60'/0'/0/0").Indexes,
                ChainId = 1
            };
            var transaction = await TrezorManager.SendMessageAsync<EthereumTxRequest, EthereumSignTx>(txMessage);

            Assert.AreEqual(transaction.SignatureR.Length, 32);
            Assert.AreEqual(transaction.SignatureS.Length, 32);
        }

[juanfranblanco]

Cool the ChainId makes different types of Ether.
On a similar fashion there is a registry of token addresses, if the address match the amount of tokens will be transferred.

[MelbourneDeveloper]

@ThatSlyGuy have you looked at #9 ?

I believe that 9# has been fixed.

[MelbourneDeveloper]

This stuff does seem strange. It seems to be doing the same thing with the latest code. It is is sending the EthereumSignTx message which is 58:

[MelbourneDeveloper]

It's possible that there are still bugs in the reading/writing to the device.

[MelbourneDeveloper]

@juanfranblanco have you debugged the helper methods for ToHex and ToHexBytes etc? I'm not 100% sure that they're correct. If so...

Are you up for doing a test with the Trezor python library ? That would determine if the problem is in the library or in the parameters being passed. If the input is the same and the output is different, it probably means there is a bug in my library and I can take it from there.

[juanfranblanco]

@MelbourneDeveloper yes that was the next step.. the ToHex() is Nethereum the ToHexBytes() is yours but only for strings, on a documentation I read the need to remove the 0x and use hex strings.. hence GasLimit is recognised. I have to stop this for a while as it was lots of trying and error. But yeah ToHexBytes for a string seem to work correctly, based on the message, GasLimit and the Address.

[juanfranblanco]

Although it might be something with the Transaction Type, that was my other thought.

[MelbourneDeveloper]

@juanfranblanco @ThatSlyGuy the problem is pretty clear. On a basic level, the To address is completely wrong. I'm getting the same result with KeepKey. It could be that the address is being sent to the device incorrectly, or it could be that the code is reading it incorrectly from the device.

There's enough information here for me to investigate. I'll update you when I can confirm whether or not there is an obvious bug.

[juanfranblanco]

I don't have a problem with the To address (check my code) but it appears as "send message", so it might the transaction type. If you try really hard you will see the address here "689c56aef474df92d44a1b70850f808488f9769c"

also check the Gwei here

[juanfranblanco]

Here is the full program, it is using the "old" TrezorManager https://github.com/juanfranblanco/Trezor.Net/blob/master/src/Trezor.Net.Sample/Program.cs

[juanfranblanco]

Ok further analysis https://github.com/trezor/trezor-mcu/blob/master/firmware/ethereum.c#L267-L268 the Value is send as 0 hence we have "message". The txn type is for WanChain so ignore that.

[MelbourneDeveloper]

@juanfranblanco thanks! Actually, I might have found the problem. I will get back to you soon.

[juanfranblanco]

Cool :)

[MelbourneDeveloper]

@juanfranblanco
Can you try a pull from branch Issue10? I believe this has fixed @ThatSlyGuy 's problem. It may fix yours.

[juanfranblanco]

Sure

[MelbourneDeveloper]

@juanfranblanco I have added the unit test SignEthereumTransaction2 to master/Issues10 which is from your sample app. You can get it by doing a pull. Am I right in understanding that the problem is the Gas limit is coming up as 210000, and the currency is saying UNK?

[MelbourneDeveloper]

What do you expect this test to display on the Trezor?

[juanfranblanco]

Correct that is where we are now, although your fix has allowed to see the Value

[juanfranblanco]

It should be 21000 and 10 Ether, let me recreate in MEW and post some shots.

[MelbourneDeveloper]

Unfortunately, I have to call it a night. But, I can see that there is an issue. I think I have some ideas about what is happening. The Issue10 branch is one step closer. I think I'll be able to fix it tomorrow.

[MelbourneDeveloper]

@ThatSlyGuy can you see if your issues are any closer with the Issue10 branch?

[juanfranblanco]

Have a great sleep, yes that branch fixed reading the Value so that is a great start, I don't get the extra zero of the GasLimit.

[juanfranblanco]

Ok re-reading the description it is the total Wei Price to it is 21k * 10 hence the 210000, so that is correct. I have checked the signatures and everything is fine.

Also the simplified test case is here, just use Nethereum extensions for this.

                        var txMessage = new EthereumSignTx
                        {
                            Nonce = 10.ToBytesForRLPEncoding(),
                            GasPrice = 10.ToBytesForRLPEncoding(),
                            GasLimit = 21000.ToBytesForRLPEncoding(),
                            To = "689c56aef474df92d44a1b70850f808488f9769c".HexToByteArray(),
                            Value = BigInteger.Parse("10000000000000000000").ToBytesForRLPEncoding(),
                            AddressNs = KeyPath.Parse("m/44'/60'/0'/0/0").Indexes,
                        };


                        var trezorTransactionSignature = await trezorManager.SendMessageAsync<EthereumTxRequest, EthereumSignTx>(txMessage);

                        var transactionSigner = new TransactionSigner();
                        var nethereumSignature = transactionSigner.SignTransaction("0x2e14c29aaecd1b7c681154d41f50c4bb8b6e4299a431960ed9e860e39cae6d29",
                         "0x689c56aef474df92d44a1b70850f808488f9769c", BigInteger.Parse("10000000000000000000"), 10, 10, 21000);
                        var transactionRecovered = new Transaction(nethereumSignature.HexToByteArray());

                        Console.WriteLine("Trezor R: " + trezorTransactionSignature.SignatureR.ToHex());
                        Console.WriteLine("Nethereum R: " + transactionRecovered.Signature.R.ToHex());
                        Console.WriteLine("Trezor S:" + trezorTransactionSignature.SignatureS.ToHex());
                        Console.WriteLine("Nethereum S: " + transactionRecovered.Signature.S.ToHex());
                        Console.WriteLine("Trezor V:" + trezorTransactionSignature.SignatureV);
                        Console.WriteLine("Nethereum V: " + new BigInteger(transactionRecovered.Signature.V));

[juanfranblanco]

@MelbourneDeveloper So the only issue is the currency UNK, the rest is correct and signing correctly. Next thing is signing token transactions or big data transactions.

[juanfranblanco]

More tidying on the message signing side:

 var message = new EthereumSignMessage
                        {
                            AddressNs = KeyPath.Parse("m/44'/60'/0'/0/0").Indexes,
                            Message = Encoding.UTF8.GetBytes("Hello")
                        };

Ill do some pull requests later

[juanfranblanco]

@MelbourneDeveloper here is a test for Token signing transfer:

   var transfer = new TransferFunction();
                        transfer.To = "0x12890d2cce102216644c59daE5baed380d848301";
                        transfer.Value = 100000000000000000;
                        var data = transfer.GetCallData();
                        var txMessage2 = new EthereumSignTx
                        {
                            Nonce = 10.ToBytesForRLPEncoding(),
                            GasPrice = 10.ToBytesForRLPEncoding(),
                            GasLimit = 21000.ToBytesForRLPEncoding(),
                            To = "6810e776880c02933d47db1b9fc05908e5386b96".HexToByteArray(),
                            DataInitialChunk = data,
                            DataLength = (uint)data.Length,
                            AddressNs = KeyPath.Parse("m/44'/60'/0'/0/0").Indexes,
                        };

                        var trezorTransactionContractSignature = await trezorManager.SendMessageAsync<EthereumTxRequest, EthereumSignTx>(txMessage2);

                        var account = new Account("0x2e14c29aaecd1b7c681154d41f50c4bb8b6e4299a431960ed9e860e39cae6d29");
                        var rpcClient = new RpcClient(new Uri("http://localhost:8545"));
                        transfer.Nonce = 10;
                        transfer.GasPrice = 10;
                        transfer.FromAddress = account.Address;
                        transfer.Gas = 21000;
                        var transactionInput = transfer.CreateTransactionInput("0x6810e776880c02933d47db1b9fc05908e5386b96");

                        account.TransactionManager.Client = rpcClient;

                        var nettherumTransactionContractSignature = await account.TransactionManager.SignTransactionAsync(transactionInput);
                        var transactionContractRecovered = new Transaction(nettherumTransactionContractSignature.HexToByteArray());

                        Console.WriteLine("Trezor R: " + trezorTransactionContractSignature.SignatureR.ToHex());
                        Console.WriteLine("Nethereum R: " + transactionContractRecovered.Signature.R.ToHex());
                        Console.WriteLine("Trezor S:" + trezorTransactionContractSignature.SignatureS.ToHex());
                        Console.WriteLine("Nethereum S: " + transactionContractRecovered.Signature.S.ToHex());
                        Console.WriteLine("Trezor V:" + trezorTransactionContractSignature.SignatureV);
                        Console.WriteLine("Nethereum V: " + new BigInteger(transactionContractRecovered.Signature.V));

[juanfranblanco]

Hopefully there are no more Java arrays ;)

[MelbourneDeveloper]

I have merged the fix in to master and will do a NuGet release.

@ThatSlyGuy ,The test SignEthereumTransaction still gives me crazy outputs on the display. Can you submit a pull request to fix the values on that test?

@juanfranblanco so transactions are all good now? I can close these issues?

[mtleliever]

@MelbourneDeveloper I'll work on that pull request next!

I'm going to close this issue now that everything seems good. I'll reopen it or create a new one if any other problems arise.