Skip to content
This repository was archived by the owner on Feb 21, 2024. It is now read-only.

Fix Ledger/Trezor transaction signing#271

Open
JamesLefrere wants to merge 1 commit intomasterfrom
fix/supply-initial-recovery-param-for-ledger-and-trezor
Open

Fix Ledger/Trezor transaction signing#271
JamesLefrere wants to merge 1 commit intomasterfrom
fix/supply-initial-recovery-param-for-ledger-and-trezor

Conversation

@JamesLefrere
Copy link
Contributor

This PR makes some changes to the purser-ledger and purser-trezor packages (necessitating some small changes to purser-core) such that the seeded signature values for the transaction to be signed (i.e. r, s and v) are not included. This is because ethereumjs-tx makes validations for EIP-155 that read these values (if present) and compare them with the chain ID, which was leading to validation errors for mainnet transactions.

Changes

  • Remove initial r, s and v values from the unsigned transaction for Ledger and Trezor
  • Add bigNumberToHexString util
  • Update Ledger packages (minor version, no breaking changes)

* Remove initial `r`, `s` and `v` values from the unsigned transaction for Ledger and Trezor
* Add `bigNumberToHexString` util
* Update Ledger packages (minor version)
@JamesLefrere JamesLefrere requested a review from rdig August 7, 2019 13:30
@JamesLefrere JamesLefrere self-assigned this Aug 7, 2019
Copy link
Member

@rdig rdig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-wise I'm all good with the changes.

But on the functional side, I don't think it works as it should.

The error I'm getting in the dApp is: insufficient funds for gas * price + value

From what I gather either the gas price or the gas value isn't passed correctly to the ledger wallet transaction:

Screenshot from 2019-08-12 15-45-15

Here's the relevant redux state:

{
  list: {
    '0sxpkzs1cdu': {
      context: 'network',
      createdAt: '2019-08-12T12:36:32.435Z',
      error: {
        type: 'SEND',
        message: 'insufficient funds for gas * price + value'
      },
      from: '0x0E07a4D6164E37e4d04d306c2e5322e268EbFEC8',
      gasLimit: '259155',
      gasPrice: '3b9aca00',
      group: {
        key: 'transaction.batch.createUser',
        id: '0sxpkzs1cdu',
        index: 0
      },
      id: '0sxpkzs1cdu',
      methodName: 'registerUserLabel',
      options: {},
      params: {
        username: 'r',
        orbitDBPath: '/orbitdb/zdpuApJ1wvTuQHFR1mpHNpavVuFnGh7XB3xcjMPg8A9aA7dmH/network.1.userProfile.0x0E07a4D6164E37e4d04d306c2e5322e268EbFEC8'
      },
      status: 'FAILED',
      loadingRelated: false
    },
    kv4ac8cy1l: {
      context: 'network',
      createdAt: '2019-08-12T12:38:19.717Z',
      error: {
        type: 'SEND',
        message: 'insufficient funds for gas * price + value'
      },
      from: '0x0E07a4D6164E37e4d04d306c2e5322e268EbFEC8',
      gasLimit: '259155',
      gasPrice: '77359400',
      group: {
        key: 'transaction.batch.createUser',
        id: 'kv4ac8cy1l',
        index: 0
      },
      id: 'kv4ac8cy1l',
      methodName: 'registerUserLabel',
      options: {},
      params: {
        username: 'r',
        orbitDBPath: '/orbitdb/zdpuApJ1wvTuQHFR1mpHNpavVuFnGh7XB3xcjMPg8A9aA7dmH/network.1.userProfile.0x0E07a4D6164E37e4d04d306c2e5322e268EbFEC8'
      },
      status: 'FAILED',
      loadingRelated: false
    },
    glau9e29esu: {
      context: 'network',
      createdAt: '2019-08-12T12:43:55.709Z',
      error: {
        type: 'SEND',
        message: 'insufficient funds for gas * price + value'
      },
      from: '0x0E07a4D6164E37e4d04d306c2e5322e268EbFEC8',
      gasLimit: '259155',
      gasPrice: '77359400',
      group: {
        key: 'transaction.batch.createUser',
        id: 'glau9e29esu',
        index: 0
      },
      id: 'glau9e29esu',
      methodName: 'registerUserLabel',
      options: {},
      params: {
        username: 'r',
        orbitDBPath: '/orbitdb/zdpuApJ1wvTuQHFR1mpHNpavVuFnGh7XB3xcjMPg8A9aA7dmH/network.1.userProfile.0x0E07a4D6164E37e4d04d306c2e5322e268EbFEC8'
      },
      status: 'FAILED',
      loadingRelated: false
    }
  }
}

Exactly the same thing happening on the Trezor side:

Screenshot from 2019-08-12 15-54-19

Note: this is not about the wallet's balance, since both my trezor and ledger have about ~0.003 ETH, which is more the enough to cover this transaction

const lastArgIndex: number = args.length - 1;
const options: * = args[lastArgIndex];
const [message]: [string] = args;
const [message] = args;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got tripped up by this too, at some point 👍

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants