Skip to content

Commit

Permalink
Always sign transactions and messages with the correct account (#14)
Browse files Browse the repository at this point in the history
The account used to sign transactions and messages should be the one
the transaction or message is from. Instead, the last connected account
was being used to sign any messages or transactions.

This was especially problematic considering the last connected account
was not persisted, meaning that signatures were being performed with
the wrong account after a reset (unless the last connected account
happened to be account 0, which was the default).

A mapping of addresses to indexes as been added to the keyring state,
and this mapping has been persisted. This should ensure the correct
account index is used, and thus the correct hd path, each time this
keyring is used for signing.
  • Loading branch information
Gudahtt authored Mar 2, 2020
1 parent 25d9628 commit f32e529
Showing 1 changed file with 15 additions and 2 deletions.
17 changes: 15 additions & 2 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const NETWORK_API_URLS = {
class LedgerBridgeKeyring extends EventEmitter {
constructor (opts = {}) {
super()
this.accountIndexes = {}
this.bridgeUrl = null
this.type = type
this.page = 0
Expand All @@ -36,6 +37,7 @@ class LedgerBridgeKeyring extends EventEmitter {
return Promise.resolve({
hdPath: this.hdPath,
accounts: this.accounts,
accountIndexes: this.accountIndexes,
bridgeUrl: this.bridgeUrl,
implementFullBIP44: false,
})
Expand All @@ -45,6 +47,7 @@ class LedgerBridgeKeyring extends EventEmitter {
this.hdPath = opts.hdPath || hdPathString
this.bridgeUrl = opts.bridgeUrl || BRIDGE_URL
this.accounts = opts.accounts || []
this.accountIndexes = opts.accountIndexes || {}
this.implementFullBIP44 = opts.implementFullBIP44 || false
return Promise.resolve()
}
Expand Down Expand Up @@ -100,6 +103,7 @@ class LedgerBridgeKeyring extends EventEmitter {
if (this._isBIP44()) {
const path = this._getPathForIndex(i)
address = await this.unlock(path)
this.accountIndexes[ethUtil.toChecksumAddress(address)] = i
} else {
address = this._addressFromIndex(pathBase, i)
}
Expand Down Expand Up @@ -136,6 +140,7 @@ class LedgerBridgeKeyring extends EventEmitter {
throw new Error(`Address ${address} not found in this keyring`)
}
this.accounts = this.accounts.filter(a => a.toLowerCase() !== address.toLowerCase())
delete this.accountIndexes[ethUtil.toChecksumAddress(address)]
}

// tx is an instance of the ethereumjs-transaction class.
Expand All @@ -150,7 +155,11 @@ class LedgerBridgeKeyring extends EventEmitter {

let hdPath
if (this._isBIP44()) {
hdPath = this._getPathForIndex(this.unlockedAccount)
const checksummedAddress = ethUtil.toChecksumAddress(address)
if (!this.accountIndexes[checksummedAddress]) {
reject(new Error(`Ledger: Index for address '${checksummedAddress}' not found`))
}
hdPath = this._getPathForIndex(this.accountIndexes[checksummedAddress])
} else {
hdPath = this._toLedgerPath(this._pathFromAddress(address))
}
Expand Down Expand Up @@ -195,7 +204,11 @@ class LedgerBridgeKeyring extends EventEmitter {
.then(_ => {
let hdPath
if (this._isBIP44()) {
hdPath = this._getPathForIndex(this.unlockedAccount)
const checksummedAddress = ethUtil.toChecksumAddress(withAccount)
if (!this.accountIndexes[checksummedAddress]) {
reject(new Error(`Ledger: Index for address '${checksummedAddress}' not found`))
}
hdPath = this._getPathForIndex(this.accountIndexes[checksummedAddress])
} else {
hdPath = this._toLedgerPath(this._pathFromAddress(withAccount))
}
Expand Down

0 comments on commit f32e529

Please sign in to comment.