Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong getSenderAddress() when getting v, r, s from Ledger HW #706

Closed
miohtama opened this issue Sep 25, 2019 · 8 comments
Closed

Wrong getSenderAddress() when getting v, r, s from Ledger HW #706

miohtama opened this issue Sep 25, 2019 · 8 comments

Comments

@miohtama
Copy link

Hi,

I need help to understand why tx.getSenderAddress() returns different value than what Ledger hw device uses for signing.

I am managing to sign a transaction using Ledger. All good, but when the signed transaction comes back from the ledger, tx.getSenderAddress() is incorrect. Also, broadcasting the transaction is not possible. I am trying to figure out what goes wrong and I am out of ideas.

Ledger device shows a correct address on the screen when signing the transaction, so derivate paths must be correct and it cannot be those. tx.validate() goes through, so it is a valid transaction. However, somehow the sender address has mutated along the way.

It might be something chainId related I need to set manually?

//
// Based on https://gist.github.com/miguelmota/62559d02a1b99cb291635de4b224349c
//

const Transport = require('@ledgerhq/hw-transport-node-hid').default

// https://github.com/LedgerHQ/ledgerjs/
// https://www.npmjs.com/package/@ledgerhq/hw-app-eth
const AppEth = require('@ledgerhq/hw-app-eth').default

// https://github.com/ethereumjs/ethereumjs-tx
const Tx = require('ethereumjs-tx').Transaction
const Web3 = require('web3')

// Use your personal Infura.io URL here
const web3 = new Web3('xxx')

// Get this from etherscan IO
const STUCK_NOCE = 12

// 50 Gwei for gas should be good as writing of this
const GOOD_GAS_PRICE = 50e9

//const DERIVATE_PATH = "44'/60'/0'/0/0"
const DERIVATE_PATH = "m/44'/60'/0'/0"

async function main() {
  const devices = await Transport.list()
  if (devices.length === 0) throw 'No Ledger connected? Please connect Ledger to USB, unlock, open Ethereum app'

  console.log("Check your Ledger screen if the script gets stuck here")
  const transport = await Transport.create()
  const eth = new AppEth(transport)

  const addressData = await eth.getAddress(DERIVATE_PATH)
  console.log("Using address", addressData.address, "from derivate path", DERIVATE_PATH)

  const txData = {
    nonce: web3.utils.toHex(STUCK_NOCE),
    gasLimit: web3.utils.toHex(200000),
    gasPrice: web3.utils.toHex(GOOD_GAS_PRICE),
    to: addressData.address,
    from: addressData.address,
    value: web3.utils.toHex(3), // 3 wei
  }

  const tx = new Tx(txData, { chain: 'mainnet'})

  const unsignedTx = tx.serialize().toString('hex')

  console.log("Sign the transaction on your ledger screen")
  console.log("Raw unsigned transaction", unsignedTx)
  // https://www.npmjs.com/package/@ledgerhq/hw-app-eth#signtransaction
  const sig = await eth.signTransaction(DERIVATE_PATH, unsignedTx)

  console.log("Sig", sig)
  txData.v = Buffer.from(sig.v, "hex")
  txData.r = Buffer.from(sig.r, "hex")
  txData.s = Buffer.from(sig.s, "hex")

  console.log(txData.v, txData.r, txData.s)

  console.log("Signing transaction");
  const signedTx = new Tx(txData)

  const signedSerializedTx = signedTx.serialize().toString('hex')

  if(!signedTx.validate()) {
    throw "Signature is not valid?"
  }

  // Here the signedBAddress, as recovered from v, r, s, is completely different from
  // what Ledger device gave us earlier.
  // I suspect it is something to do how tx sets v, r, s and then recovers
  // the key.
  const signedByAddress = "0x" + signedTx.getSenderAddress().toString('hex');
  if(signedByAddress.toLowerCase() != addressData.address.toLowerCase()) {
      throw "Ledger signed transaction using wrong address:" + signedByAddress + " vs. " + addressData.address;
  }

}

main()
@holgerd77
Copy link
Member

Hi @miohtama, sorry, we are currently under-staffed, seems that no one had time to dig deeper here. Did you find a solution/fix/explanation for the problem in the mean time? Would be helpful for others if you post here.

@picatextra
Copy link

I'm having the same issue,
LedgerHQ/ledgerjs#436

@evertonfraga evertonfraga transferred this issue from ethereumjs/ethereumjs-tx Apr 6, 2020
@webees
Copy link

webees commented Apr 15, 2020

image

> '‭0‬'.length // Copy and paste this line to the browser console
> 3
> '‭0'.length // Copy and paste this line to the browser console
> 2
> '0'.length // Copy and paste this line to the browser console
> 1

Check Unicode?

@holgerd77
Copy link
Member

Is this still an issue with Tx v3? //cc @ryanio

@ryanio
Copy link
Contributor

ryanio commented Dec 3, 2020

i'm not sure what the root problem was here, hopefully fixed along many other things with tx v3. i don't have a ledger though, could someone with a ledger try and report back?

@alcuadrado
Copy link
Member

The linked issue was closed. The problem was that the wasn't setting the right v value, and was using EIP115.

TBH I'd close this issue. TX used to be very confusing because it used RLP-encoded buffers everywhere, which lead to people reporting signature-related "bugs" all the time.

@holgerd77
Copy link
Member

@alcuadrado ok, thanks, will close for now, feel free to reopen if issue still persists.

@taylorjdawson
Copy link

@alcuadrado This issue persists for me copying the example on the home page nearly verbatim doesn't work:

// Signing a 1559 tx
txData = { value: 1 }
tx = FeeMarketEIP1559Transaction.fromTxData(txData, { common })
unsignedTx = tx.getMessageToSign(false)
;({ v, r, s } = await eth.signTransaction(bip32Path, unsignedTx)) // this syntax is: object destructuring - assignment without declaration
txData = { ...txData, v, r, s }
signedTx = FeeMarketEIP1559Transaction.fromTxData(txData, { common })
from = signedTx.getSenderAddress().toString()
console.log(`signedTx: ${signedTx.serialize().toString('hex')}\nfrom: ${from}`)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants