Common characters being overridden in chinese simplified and chinese Traditional word lists

eth-educators / ethstaker-deposit-cli

Secure key generation for deposits

https://eth-educators.github.io/ethstaker-deposit-cli/

Creative Commons Zero v1.0 Universal

3 stars 6 forks source link

Common characters being overridden in chinese simplified and chinese Traditional word lists #119

Closed yorickdowne closed 1 month ago

yorickdowne commented 2 months ago

From https://github.com/ethereum/staking-deposit-cli/issues/410

“While going through the code, this code here is causing a problem but hiding it. There are common characters between the 2 word lists and when the map is created in the referenced code, if there is a collision in keys, it is silently overridden to the new file being read. This issue can be seen if the code checks for collision of keys.”

We need to write a test to catch this, and fix the issue.

chong-he commented 2 months ago

Not sure what is meant by "hiding it", but happy to get involved in this and do some testing if I have a sample mnemonic.

I tested creating a mnemonic with the Chinese Simplified language and restore it, the public key is the same. So maybe only in certain cases that this issue will manifest

remyroy commented 1 month ago

I think this comment here gives a better picture of the overall issue: https://github.com/eth-educators/ethstaker-deposit-cli/pull/142#issuecomment-2340985192

remyroy commented 1 month ago

There was a lot of good comments in #142 about this and how to potentially resolve it. Is there still something confusing about how we should go forward with this @valefar-on-discord ? If not, are you good implementing a solution for this issue?

valefar-on-discord commented 1 month ago

Nothing confusing.

The original bug report is 100% valid and specificly English/French mnemonic languages have some concerning overlap. It seems to be possible for an English mnemonic to be completely overwritten by French and produce a completely different cryptogrpahic result.

The Fix:

We first need to update the logic and unit tests of how a mnemonic is determined to allow for multiple discovered languages per word instead of overwritting. #142 does the first part. The tests need to be upgraded
The second step is in the situation where the mnemonic fails because multiple languages are detected, we need to notify the user and ask which language the mnemonic is associated with. I haven't investigated the level of effort here, but it may be non-trivial.

I don't believe I have any blockers on handling this besides finding the time but this is my next priority item to look at.