gaiacli keys add gives different addresses with --recover OR --ledger (same mnemonic)

[gamarin2]

Summary of Bug

I tried to generate an account from the same 12-words mnemonic (obtained with the fundraiser tool) using two different processes and obtained two different addresses.

The first process was to use the --recover flag of gaiacli keys add, with --account 0
The second process was to restore a ledger wallet with the 12-words mnemonic, and use the --ledger flag of gaiacli keys add, with --account 0.

Steps to Reproduce (using 12-words mnemonic)

1. Generate a 12-words mnemonic using https://github.com/cosmos/fundraiser-cli
2. Generate an account from this mnemonic using gaiacli keys add --recover --account 0. This gives address1.
3. Restore a ledger with the 12-words mnemonic. Then, use gaiacli keys add --ledger --account 0. This gives address2.
4. address1 and address2 are not the same.

Edit: I restored the ledger twice to see if it was not a human error when inputting the mnemonic on the device -- @gamarin2

Steps to Reproduce (using 24-words mnemonic)

1. Generate a 24-words mnemonic using a Ledger device. Note: install cosmos ledger app directly from Ledger Live: Update README.md ledger-cosmos#99 (comment)
2. Generate a local ledger key for the device: gaiacli keys add --ledger ledger-key
3. Recover locally a key with the 24-word mnemonic obtained at step 1: gaiacli keys add --recover ledger-key-recover
4. Print both keys:

gaiacli keys show ledger-key
gaiacli keys show ledger-key-recovered

Addresses/public keys don't match.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[cwgoes]

cc @jleni do you have any idea what the cause might be or where we should start to debug this?

Alternatively, have we tested the Ledger app against a separate HD derivation implementation?

[jleni]

The only "simple" thing I can imagine is that the derivation path is not hardened in the same way.

When sending a path to a ledger device, the first three elements in the path are automatically hardened: i.e. 44'/118'/0'/0/5 =
0x8000 0000 | 44 , 0x8000 0000 | 118, 0x8000 0000 | 0, 0, 5

Maybe gaia is not hardening in the same way. Could this be the problem?

[cwgoes]

It's supposed to - https://github.com/cosmos/cosmos-sdk/blob/develop/crypto/keys/hd/hdpath.go#L28 - but perhaps the hardening isn't being processed correctly? cc also @liamsi - do you remember, when you worked on this before, if we tested it against the Ledger derivation?

[jleni]

We can easily add more tests for that. It would definitely be a good thing to do.

[alessio]

EDIT: I can reproduce it, I'm investigating.

[jackzampolin]

You can also initialize a ledger with a user supplied mnemonic. So I think the issue here is saying that a ledger restored with the mnemonic generated from the fundraiser code provides a different address from that same 12 word phrase restored with gaiacli.

[alessio]

CC'ing @jessysaurusrex @zmanian

[alessio]

Looks like a regression:

alessio@bangalter:~/.../cosmos/cosmos-sdk$ git bisect good
04736215707f7172ecd9f25843345106233c7135 is the first bad commit
commit 04736215707f7172ecd9f25843345106233c7135
Author: Juan Leni <jleni@users.noreply.github.com>
Date:   Fri Nov 30 15:23:55 2018 +0100

    Merge PR #2870: Upgrading ledger-cosmos-go dependency
    
    * Upgrading dependencies
    * Update dependency

:100644 100644 cd0efad9e0775f1efca5d4478a84be47eb8d9a1d acb08648263bd67f7b9a626f76c8aaf48d88c88e M	Gopkg.lock
:100644 100644 43fe589ef68565867bf17a1fa2d8567344b7685f 1593e72986c9a51ef0260f73e256c14650e6835d M	Gopkg.toml
:040000 040000 a03f5ebf1ac300a59825ea5d2669e4ffbd494a75 efcb398b24d7858053d5ad45e07a96fe93ef8ed1 M	crypto

[jackzampolin]

@alessio can we fix this by reverting to the older dependancy.

[jleni]

@jackzampolin @alessio No, the upgrade is necessary for the latest app.
Could you assign this to me? I will have a look at what is going on in the dependency today.

[jleni]

I would also like to update gaia's unit tests and extend them to consider all these cases

[jleni]

I have just tested the PR. I will add some gaia unit tests for this.

[cwgoes]

@jleni Can you explain what the upstream issue was (just curious, and wanting to make sure we don't have any similar possible latent bugs)?

[alessio]

@cwgoes see Zondax/ledger-cosmos-go@ed9aa39

[liamsi]

cc also @liamsi - do you remember, when you worked on this before, if we tested it against the Ledger derivation?

(Sorry, somehow missed that mention/notification) No, I do not really remember (but I think we did somehow manually). Looks like @jleni / @alessio found the issue: instead of the first 3, the first 4 were hardened: Zondax/ledger-cosmos-go@ed9aa39#diff-36caaa1394a5803bfa9f964755f1d733L41
and it used to be the first 3: Zondax/ledger-cosmos-go@983ac32#diff-7db993309c63d42a3dfbbd197b9af49cL71

[jleni]

Correct. I will later add a few additional integration tests in gaia/sdk to check public keys/mnemonics.

[jleni]

While the problem in the ledger app has been fixed, there is still a problem in gaiacli.
The following three commands give the same address:

gaiacli keys add gaia0 --recover --account 0
gaiacli keys add gaia1 --recover --account 1
gaiacli keys add gaia152 --recover --account 152

@gamarin2 could you confirm this too?

[gamarin2]

Yes, I can confirm

[jackzampolin]

@jleni can you open another issue to track work there? Also is that recovering with the same seed, or different seeds?

[jleni]

same seed

[cwgoes]

Indeed, we probably do not respect the HD derivation flags in our own crypto HD derivation, and we ought to.