status-im / status-mobile

a free (libre) open source, mobile OS for Ethereum
https://status.app
Mozilla Public License 2.0
3.9k stars 987 forks source link

Can recover account with seedphrase created with keycard #9062

Closed Serhy closed 4 years ago

Serhy commented 5 years ago

Type: Bug Account created with keycard can be recovered in Status as normal account.

Summary: Account created with keycard can be recovered in Status as normal account.

Expected behavior

Image for expected case!

Screenshot 2019-09-27 at 17 06 17

Actual behavior

Account recovered as normal account

Reproduction

Precondition

Seedphrase of an account created with keycard exists (e.g. pepper save spread chaos unable narrow couple little clog monkey voice weapon)

Reproduction:

Additional Information

Serhy commented 5 years ago

@guylouis the expected result of this behavior based on relatively old requirements. Would there might be another expected result?

guylouis commented 5 years ago

@Serhy do you mean that

and in this case

If yes, that's an issue since there should never be two multiaccounts with the same seed as described here https://github.com/status-im/status-react/issues/8750

churik commented 5 years ago

@guylouis the question here is that case: 1) User create keycard multiaccount on device1 (seed1) 2) User tries to restore ordinary account with seed1 on device2

On our test cases (written by Nastya) - it is not possible, error is

Screenshot 2019-09-27 at 17 06 17

I want to find out what is expected result in this case and in case if we plan to restore account from seed phrase in MetaMask for example.

dmitryn commented 5 years ago

It's not the case when 2 identical account are created. For same seed phrase status-go multiAccountImportMnemonic and keycard generateAndLoadKey derive different keys. For instance, curtain mosquito taste relief topple valid initial plug fall there talk core derive path m/43'/60'/1581'/0'/0:

dmitryn commented 5 years ago

I'd like to ask someone from @status-im/go to check if multiAccountImportMnemonic works as expected.

guylouis commented 5 years ago

cc @gravityblast

guylouis commented 5 years ago

tested with today's nightly StatusIm-191016-025900-8e9010-nightly-universal.apk and I confirm that on the same phone if I create

Then: I get two multiaccounts on the phone with different

Whereas the intended behaviour is that

@gravityblast see the discussion above, can the issue be in go ?

gravityblast commented 5 years ago

I'm trying with yesterday's and today's nightly but even when I select keycard it asks for a password and the keycard is never used. Does it work for you?

guylouis commented 5 years ago

no it doesn't opened an issue https://github.com/status-im/status-react/issues/9231

gravityblast commented 5 years ago

User create keycard multiaccount on device1 (seed1) User tries to restore ordinary account with seed1 on device2 On our test cases (written by Nastya) - it is not possible, error is

@churik are you only trying to recover a normal mnemonic phrase in a different device? Maybe I'm missing something but device 2 shouldn't know that the mnemonic has been used for a keycard. or does device 2 already have the same account with keycard as well?

gravityblast commented 5 years ago

@dmitryn We tried to import your mnemonic to both keycard and status-go manually (outside status) and deriving the chat key. They both derive the same public key 0x04a0a4d09cdafdca75b131d82df8a1a593efe0293573694a74ff1d99be65e39899d331b0f21201a044893e34c718276c9563803a66cb600e07559d5a5d15f2361b.

So I guess maybe we are deriving something else from within status? I can't really test it because the nightly build doesn't work with the keycard. Is there anyone else that can try importing that specific mnemonic to a keycard with a nightly build?

churik commented 5 years ago

@gravityblast nightly build is working with keycard, I suppose you download it from https://status.im/nightly/ which is not updated ~for 1 month. Please use https://ci.status.im/job/status-react/job/nightly/ to get latest develop builds.

@churik are you only trying to recover a normal mnemonic phrase in a different device? Maybe I'm missing something but device 2 shouldn't know that the mnemonic has been used for a keycard. or does device 2 already have the same account with keycard as well?

I don't know for sure. My only source of truth was testcase, created on 01/2019, where is clearly stated that should be error at attempt to recover keycard account with password.

keycard : can't recover keycard account with password - TestRail 2019-10-17 12-53-06
guylouis commented 5 years ago

Full reproduction of the issue. I have to use a different mnemonic than the one above because it's not possible to recover a seed on a keycard now (issue #9231 )

FIRST STEP: SET UP A KEYCARD MULTIACCOUNT

SECOND STEP: SET UP A NON-KEYCARD MULTIACCOUNT WITH SAME SEED ON THE SAME PHONE

CONCLUSION: same seed but everything is different: chat key, wallet address and three names

gravityblast commented 5 years ago

@guylouis I think we display the wrong public key. Try again setting up a Keycard, then re-install status and pair the card with your pairing password. The public key is different, and in this case we aren't even using the mnemonic phrase.

gravityblast commented 5 years ago

@churik I used the build you say but trying importing a mnemonic to a keycard instead of setting up a new account from scratch and it wasn't working

guylouis commented 5 years ago

@gravityblast yes this is #9231

with regards to the public key issue, who can check where is the bug, is it @dmitryn ? Can you share here elements that leads to think issue is on react and not go ? Thanks

gravityblast commented 5 years ago

@guylouis first of all there's something wrong with the mnemonic. if you try to import it in any other wallet like mycrypto, you will see it's invalid. Then if I use the mnemonic with our new and old libraries in go, I see that the right address is the second one. so I guess there's something wrong when we call the keycard API maybe. yes I htink @dmitryn knows more about it

rasom commented 5 years ago

just to clarify, issues title states:

Can recover account with seedphrase created with keycard

As far as I understand, there is no problem in ability to recover keycard's account as a regular account on device. Almost likely we want that account/phrase to be compatible with any other wallet/storage, so that shouldn't be a problem at all.

But we still have two problems with keycard's mnemonic:

  1. The phrase which we show during keycard setup can not be used for restoring account later. It is actually a mnemonic generated for pairing, at least how I understood. Probably it can be used for restoring keycards setup somehow, but you definitely can't use this passphrase at MEW or other wallet.
  2. We can have two accounts with the same root key on a single device, one stored on keycard and another on device. This issue is covered here https://github.com/status-im/status-react/issues/8750

So I would just close this issue as it is a bit misleading already and create a new one for 1. if it doesn't exist already.

flexsurfer commented 4 years ago

@rachelhamlin @guylouis wdyt ?

guylouis commented 4 years ago

There were several problems yes !

9231 should solve the issue of creating a keycard account based on a seed entered by the user. This was a typo issue in the code that you fixed. It seems there are device specific issues that prevent losing it now.

8750 tackles the issue of preventing to have "two accounts with the same root key on a single device, one stored on keycard and another on device", because from a security perspective it makes no sense, and gives a false sense of security to the keycard account, since the same private key are actually on the phone in the other account. I added some comments in #8750 on the roadmap to make transitioning from on keycard account (KCA) to on-device account (ODA) KCA->ODA, ODA->KCA, KCA-> KCA, ODA->ODA.

As for the original problem from this issue, it now has several subparts caused by the same root cause or not. I'll close this one and open a new one (#9393) that shows the symptoms

guylouis commented 4 years ago

closing this one, since tackled in the three issues listed above