cosmostation / keystation

🔑 Keystation - decentralized keychain-bassed authenticator enables browsing blockchains built with Cosmos SDK
https://keystation.cosmostation.io/
125 stars 57 forks source link

Misleading HD derivation path in UI #7

Open webmaster128 opened 4 years ago

webmaster128 commented 4 years ago

Hello everyone! I just tested keystation together with https://wallet.cosmostation.io/

I was very confused about the HD derivation path that is used. For Cosmos Hub, the path should be m/44'/118'/0'/0/0, so some of the components are hardened, some are not. In the UI it simply shows 44/118/0/0/0, which indicatiates a path with 5 non-hardened components is used.

But the address derivation is correct (e.g. menmonic economy stock theory fatal elder harbor betray wasp final emotion task crumble siren bottom lizard educate guess current outdoor pair theory focus wife stone leads to address cosmos1pkptre7fdkl6gfrzlesjjvhxhlc3r4gmmk8rs6, which is m/44'/118'/0'/0/0 and compatible with Cosmos SDK and CosmJS).

It would be nice if the UI was more precise about the derivation, which is critical for interoperability as well as privacy.

Booyoun-Kim commented 4 years ago

I also thought about whether I should use the path starting with m. However, I decided to use the same as the Ledger Nano javascript library. I wanted to match the development specs of Ledger Nano and Keystation in our web wallet. If you have any better opinions, please let me know.

https://github.com/Zondax/ledger-cosmos-js/blob/master/tests/basic.ispec.js#L29

webmaster128 commented 4 years ago

I don't care much about the m/. I think it is good to display but not critical.

The main problem is 44 vs. 44' and 118 vs. 118'. Those are completely different path components.

The comment // Derivation path. First 3 items are automatically hardened! says that the resulting derivation path is not [44, 118, 0, 0, 0] but [44 + 2 ** 31, 118 + 2 ** 31, 0 + 2 ** 31, 0, 0].

The way the Ledger lib communicates with the Ledger app is really an implementation detail of Ledger and nothing that should be communicated to the user.

Booyoun-Kim commented 4 years ago

I see. I will discuss with our team member how to handle this. Thank you 😃

webmaster128 commented 4 years ago

By the way, CosmJS can do this for you:

import { pathToString, Slip10RawIndex } from "@cosmjs/crypto";

const path = [
  Slip10RawIndex.hardened(44),
  Slip10RawIndex.hardened(118),
  Slip10RawIndex.hardened(0),
  Slip10RawIndex.normal(0),
  Slip10RawIndex.normal(6),
];

console.log(pathToString(path)); // m/44'/118'/0'/0/6 
Booyoun-Kim commented 4 years ago

I will remove auto harden function. Users will be able to use the HD path as is. I'll add the parameter &version=2 to avoid confusion for existing users. Version param: https://github.com/cosmostation/keystation/blob/dev/service/service.go#L68 Auto harden: https://github.com/cosmostation/keystation/blob/dev/www/js/import.js#L134 Auto harden: https://github.com/cosmostation/keystation/blob/dev/www/js/pin.js#L143