trezor / trezor-suite

Trezor Suite Monorepo
https://trezor.io/trezor-suite
Other
743 stars 257 forks source link

Trezor ethereumSignTransaction function signs the transaction with a different address when given the same path #5802

Open ghost opened 2 years ago

ghost commented 2 years ago

Overview

Am currently building a Trezor wallet integration into an App. I am using the "trezor-connect" package to do this and on the latest version (8.2.10). I am able to signMessages and recover the address from the signed the message to validate that the correct address is signing this. But when I sign transactions through the ethereumSignTransaction method if I modify any of the fields on the transaction it results in a different address having signed the transaction. I can validate this actually happens because if I fund that account I can now successfully send this signed transaction to the network and it is processed. And I am definitely using the same path parameter for all of these.

How to reproduce For instance, if I provide the transaction (ignore the zeroed fields): { "value": "100000000000000", "gasLimit": "21000", "nonce": "0", "data": "0x", "to": "0xe545599a536D4F929258DADa946dA41AfC54d036", "chainId": 5, "maxFeePerGas": "0", "maxPriorityFeePerGas": "0" } Then if I decode the response, using parseTransaction from ethers, it returns a "from" address of: "0x2A83aF4bE9C4B65EfB5bFCa4E09B4Ab832D46bDf"

Then if I modify the above transaction and change the value field to be: "200000000000000". It returns a "from" address of: "0x789eb08EBE5B12c50af43ca4e12f65449a373B4A"

I could be doing something wrong, but have tested this many times, I've tested changing different fields; maxFeePerGas, maxPriorityFeePerGas, data, etc. and they all result in a different "from" address. This doesn't feel like intended behavior.

mroz22 commented 2 years ago

Hi, is your app public or can you provide a runnable example?

ghost commented 2 years ago

The app is private, but I've written the following example and it does produce the same bug:

import { ethers } from "ethers";
import { serialize } from "@ethersproject/transactions";

const sharedBasicTxFields: Omit<EthereumTransactionEIP1559, "value"> = {
  "gasLimit": "21000",
  "nonce": "0",
  "data": "0x",
  "to": "0xe545599a536D4F929258DADa946dA41AfC54d036",
  "chainId": 5,
  "maxFeePerGas": "0",
  "maxPriorityFeePerGas": "0",
};

const firstBasicTx: EthereumTransactionEIP1559 = {
  ...sharedBasicTxFields,
  "value": "100000000000000",
};

const secondBasicTx: EthereumTransactionEIP1559 = {
  ...sharedBasicTxFields,
  "value": "200000000000000",
};

const path = "m/44'/60'/0'/0/0";

const serializeAndSignTx = async (tx: EthereumTransactionEIP1559) => {
  const serializedTx = ethers.utils.keccak256(serialize({ ...tx, nonce: parseInt(tx.nonce), type: 2 }));

  const res = await TrezorConnect.ethereumSignTransaction({
    path,
    transaction: tx,
  });

  if (!res.success) throw new Error(res.payload.error);

  const fixed = {
    ...res.payload,
    v: parseInt(res.payload.v),
  };

  const signature = ethers.utils.joinSignature(fixed);

  return ethers.utils.recoverAddress(serializedTx, signature);
};

export const basicTest = async () => {
  TrezorConnect.manifest({
    email: "test@gmail.com",
    appUrl: "localhost:3000",
  });

  const addressForPath = await TrezorConnect.ethereumGetAddress({ path });

  if (!addressForPath.success) throw new Error(addressForPath.payload.error);

  console.log("Address for path: ", addressForPath.payload.address);

  const firstTxRecoveredAddress = await serializeAndSignTx(firstBasicTx);

  console.log("First recovered address: ", firstTxRecoveredAddress);

  const secondTxRecoveredAddress = await serializeAndSignTx(secondBasicTx);

  console.log("Second recovered address: ", secondTxRecoveredAddress);
};

Which produces the following logs:

Address for path: 0xDd5ce0e30237702af935e9631795f04a716772fa First recovered address: 0xE9A8e5A308E1F2e63e5ED70026500833f6eA9F82 Second recovered address: 0x8EA3995fc094c153F68fBc69CA49F77723680CFa

Thanks!

ghost commented 2 years ago

I've recently updated my version to use V9 of the trezor connect library and still having this issue.

@mroz22 Any ideas? This is starting to feel a bit absurd.

mroz22 commented 2 years ago

Hi, I have just updated your example, so it can be easily copy pasted and run, possibly served from local webserver. Pasting it here to have it available for future use.

Are you suggesting that TrezorConnect.ethereumSignTransaction returns wrong results? If yes, we should definitely investigate. If not, it is most likely some problem with ethers lib which would be out of the scope in this repo.

<!DOCTYPE html>
<html lang="en">
    <head>
        <title>@trezor/connect example</title>
    </head>
    <body>
        <script type="text/javascript" src="https://connect.trezor.io/9/trezor-connect.js"></script>
        <script
            src="https://cdn.ethers.io/lib/ethers-5.6.umd.min.js"
            type="application/javascript"
        ></script>

        <script type="text/javascript">
            window.TrezorConnect.init({
                lazyLoad: true,
                manifest: {
                    email: 'developer@xyz.com',
                    appUrl: 'http://your.application.com',
                },
            });

            const sharedBasicTxFields = {
                gasLimit: '21000',
                nonce: '0',
                data: '0x',
                to: '0xe545599a536D4F929258DADa946dA41AfC54d036',
                chainId: 5,
                maxFeePerGas: '0',
                maxPriorityFeePerGas: '0',
            };

            const firstBasicTx = {
                ...sharedBasicTxFields,
                value: '100000000000000',
            };

            const secondBasicTx = {
                ...sharedBasicTxFields,
                value: '200000000000000',
            };

            const path = "m/44'/60'/0'/0/0";

            const serializeAndSignTx = async tx => {
                const serializedTx = ethers.utils.serializeTransaction({
                    ...tx,
                    nonce: parseInt(tx.nonce),
                    type: 2,
                });

                const res = await TrezorConnect.ethereumSignTransaction({
                    path,
                    transaction: tx,
                });

                if (!res.success) throw new Error(res.payload.error);

                const fixed = {
                    ...res.payload,
                    v: parseInt(res.payload.v),
                };

                const signature = ethers.utils.joinSignature(fixed);

                return ethers.utils.recoverAddress(serializedTx, signature);
            };

            const basicTest = async () => {
                TrezorConnect.manifest({
                    email: 'test@gmail.com',
                    appUrl: 'localhost:3000',
                });

                const addressForPath = await TrezorConnect.ethereumGetAddress({ path });

                if (!addressForPath.success) throw new Error(addressForPath.payload.error);

                console.log('Address for path: ', addressForPath.payload.address);

                const firstTxRecoveredAddress = await serializeAndSignTx(firstBasicTx);

                console.log('First recovered address: ', firstTxRecoveredAddress);

                const secondTxRecoveredAddress = await serializeAndSignTx(secondBasicTx);

                console.log('Second recovered address: ', secondTxRecoveredAddress);
            };
        </script>

        <button onclick="basicTest()">basic test</button>
        <div id="result"></div>
    </body>
</html>
ghost commented 2 years ago

Awesome thanks for doing that.

Yeah my suspicion is that the trezor sign method:TrezorConnect.ethereumSignTransaction, is returning the wrong results.

If I fund the account in question and try to send a valid transaction it fails saying; I don't have enough funds to execute the transaction, then if I fund the recovered address it works, so ethers seems to be working fine.

mroz22 commented 2 years ago

could you maybe try to sign the same transaction with a different wallet a post the result here? also maybe @trezor/qa could help if they find some free time, not sure but lets try tagging them

ghost commented 2 years ago

Sure no problem. Just a note that when I was testing I realised I was serialising the transactions incorrectly and have updated that part of the demo code. I re-tested everything and still having the same issue.

To demonstrate the problem further the Trezor account I control for the path has this address 0xDd5ce0e30237702af935e9631795f04a716772fa, but when I recover the address through the ethers function it was returning 0x6cFc7608C14Aff797E98e4698cB092Bd89EE0494 So I sent it some ETH to this address and tried to send the transaction to ensure the ethers address recovery is working correctly, and the following transaction was sent

Also I tested this code with a private key wallet and had the following recovery:

Address for path: 0xEEc702bd5bf31d01BdbfD977332644164150392B First recovered address: 0xEEc702bd5bf31d01BdbfD977332644164150392B Second recovered address: 0xEEc702bd5bf31d01BdbfD977332644164150392B

So I'm quite confident ethers is working correctly here

agrudsky commented 1 year ago

Bump. I'm having this exact same issue with ethereumSignTransaction.

mroz22 commented 1 year ago

@trezor/connect doesn't do much here. It simply passes your data to trezor device and returns response (more or less). So there are basically 3 options what could go wrong:

  1. data returned from Trezor device is wrong.
  2. data returned from TrezorConnect is wrong.
  3. data returned from TrezorConnect is technically correct, but maybe specifically formatted and fed into your ethers lib in a wrong shape

Can we somehow rule out number 1? Possibly using https://docs.trezor.io/trezor-firmware/python/trezorlib.html

bosomt commented 1 year ago

Checked this issue and i see that user that created this issue is already deleted. Not sure what to do next , close ? @hynek-jina @mroz22

Hannsek commented 1 year ago

If this is still a thing, we should prioritize this accordingly.

reVrost commented 3 months ago

I can confirm, this is still happening. Can we please get this looked into? Am testing with: Trezor model one. and Firmware: Universal 1.12.1

ethereumSignedTransaction seems to yield different address everytime the payload changes. ethereumGetAddress however returns the correct address.