The Watch Only Wallet Bug

[tynes]

The Watch Only Wallet Bug

Overview

The hsd wallet has an edge case regarding watch only wallet usage
that results in the incorrect path being returned to the client.
This will result in invalid signature creation because the wrong
key will be derived and used to sign the transaction. Using an
extra API call, it is possible to work around this problem right
now, but it is not ideal and is fixable.

Background

The hsd wallet server uses bip44 key derivation to make key backup
simple. In practice, users often have business constraints that do
not perfectly match 1:1 with the way that bip44 works. The hsd wallet
server uses the Wallet abstraction to represent a mnemonic, or the
root of a bip44 based wallet, and the Account abstraction to represent
the depth 3 index. This is often referred to as the "account index".

A single Wallet can have many Accounts, as creating a new account
is as simple as incrementing the integer at depth 3 in the bip44 path
and rederiving keys. All of the receive addresses and change addresses
derived from the extended public key that represents the account can
be deterministically regenerated and will automatically be indexed
by the hsd wallet database as new blocks come in.

Currently, the hsd wallet increments the account index for each new
account that is created. This makes sense if the mnemonic is on the
server, but that is a less safe practice for large sums of money.
The hsd wallet also supports "watch only wallets", or when the private
keys are not kept on the server at all and instead the depth 3 extended
public key is indexed. This is generally called an xpub and can derive
receive and change addresses, which allows the wallet to watch and index
any transactions but not spend them.

The problem arises when the user adds an xpub from a different index
than the next expected index that the wallet is expecting. Since the
wallet increments the account index each time and does not validate
incoming xpubs, it is possible to index an xpub of any account index
at the incorrect account index in the database. This will result in
an incorrect path being returned by the database for account index,
as it will return the index in the database instead of the actual
index of the xpub. Luckily this bug can be worked around by parsing
the actual account index out of the xpub, since it gets serialized into
it. The current method to use watch only wallets reliably involves
fetching the account information and the key information for the utxo
being signed. The account information will include the xpub, from which
the account index can be parsed from, and the key information includes
the branch and index. All of this information together will result in
the full path, assuming standard bip44:

m/44'/0'/accountIndex'/branch/index

Possible Solution

The current wallet database has a uniqueness constraint on the account index.
The proposed solution involves changing the account index to an "account id",
which autoincrements and has a uniqueness constraint, but has no relationship
to the bip44 account index. This would allow for many xpubs with the same
account index to be indexed in the database without a problem (ie different
mnemonics, different ancestor in the hd tree). It also means that two database
lookups would be necessary instead of one.

Another index could be added to hold the bip44 account index, but that would
be duplicating data as the bip44 account index is serialized into the xpub.
Pending further inspection of the code, it could be possible to make this
change without needing to add extra buckets to the database.

Some questions to take into account:

• Should watch only wallets be strict bip44 compliant?
  • Enforce the xpub's index and parent chain matches
• Should there be a catch all type of wallet that allows more flexiblity

Implementation Notes

In the lib/wallet/account.js, some changes to Account might be necessary.

• Account.id is the wallet name, a human readable string
• Account.accountIndex needs to be altered to not be used at the primary
  key in the database

Line 24 needs to switch from account index to account id, and everywhere
index is metioned, it needs to swtich to id.

Previous Work

bcoin-org/bcoin#696
bcoin-org/bcoin#689