Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Phrases using common words between wordlists will fail unless passing lower index in Mnemonic.Words #3211

Closed
dabura667 opened this issue Sep 20, 2015 · 7 comments · May be fixed by bitpay/bitcore-mnemonic#36

Comments

@dabura667
Copy link
Contributor

English check OK:
abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon angle
French check OK:
abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon brave
both check OK:
abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon amateur double

Here are 3 phrases, all using only the common words of English and French.

One only checks OK on the English wordlist, one only checks OK on the French wordlist, and the last one checks OK on both lists.

Entering all three into Copay using the following locale settings will yield the following results:

'en':

  1. Pass
  2. Fail
  3. Pass

'fr':

  1. Pass
  2. Fail
  3. Pass

The expected behavior would be:

'en':

  1. Pass
  2. Fail
  3. Pass

'fr':

  1. Fail
  2. Pass
  3. Pass

Suggestion:
Depending on the locale of Copay, pass a wordlist to bitcore-mnemonic in this line:
https://github.com/bitpay/bitcore-wallet-client/blob/b342e6d8f9be23a904dbff3219028bc47a47e322/lib/credentials.js#L86
like this: var m = new Mnemonic(words, localewordlist);

There is a possibility (though very small) that a French phrase generated by Copay can not be recovered in Copay due to this. (Unless you are checking the mnemonic output again using Mnemonic.isValid() after generating.

@dabura667 dabura667 changed the title [Bug] Phrases using common words will fail unless passing lower index in Mnemonic.Words [Bug] Phrases using common words between wordlists will fail unless passing lower index in Mnemonic.Words Sep 20, 2015
@dabura667
Copy link
Contributor Author

Chance of randomly generating a phrase that is valid for French and contains only words that are both in English and French:
0.000000000000001%

1 in 100 quadrillion phrases.

@dabura667
Copy link
Contributor Author

btw: Chinese Traditional / Simplified wordlists have a MUCH higher convergence rate...

However, luckily (?) we don't need to worry, as we can't support zh-CN and zh-TW right now, so we only have zh-CN (simplified) on Crowdin.

@matiu
Copy link
Collaborator

matiu commented Sep 21, 2015

Thanks a lot for this detailed report! We will check it.

@dabura667
Copy link
Contributor Author

@matiu Found this:
https://github.com/bitpay/bitcore-wallet-client/blob/ed984c79766f1ba15b4c92735b9924beb9682d50/lib/credentials.js#L65

Between L65 and L66 we could add the lines:

while (!Mnemonic.isValid(m.toString())) {
  var m = new Mnemonic(wordsForLang[language])
};

This would basically check to see if the generated phrase will be Validated correctly when entered, and regenerate if not.

This way, at least, we know that Copay can not generate phrases that it will not be able to validate.

@matiu
Copy link
Collaborator

matiu commented Sep 21, 2015

Yes, that is good idea. But, since we manage the bitcore-payment-protocol libs, we will try to fix it there directly. Given that the error is so unlikely, and if it happens the funds will safe anyways, we will update bitcore-payment-protocol for the next release. Thanks a lot for the report, great catch!

@matiu
Copy link
Collaborator

matiu commented Sep 21, 2015

  • bitcore-mnemonic not bitcore-payment-protocol.

@dabura667
Copy link
Contributor Author

bitcore-mnemonic not bitcore-payment-protocol.

I was about to say... haha.

but to be honest, I am not sure how you would fix it in bitcore-mnemonic. The method of "search every word and find the first wordlist that contains every word." seems like a fine method.

Maybe "search all wordlists even if a match is found, and if it matches multiple wordlists, check the checksum against all matched wordlists and return true if any one of them matches." would be better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants
@matiu @dabura667 and others