Skip to content

KeyringController does not prevent duplicate accounts properly #5411

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

Closed
mikesposito opened this issue Feb 28, 2025 · 0 comments · Fixed by #5710
Closed

KeyringController does not prevent duplicate accounts properly #5411

mikesposito opened this issue Feb 28, 2025 · 0 comments · Fixed by #5710
Assignees
Labels
bug Something isn't working team-wallet-framework

Comments

@mikesposito
Copy link
Member

mikesposito commented Feb 28, 2025

Current checks for duplicate accounts do not work properly since support for multiple HD keyrings has been added. Moreover, there are several loopholes in the current method in charge of checking duplicates:

  • No keyring is checked besides the simple one
  • No check is made when adding an account (only when creating the keyring)

The issue can be dangerous because of several selectors that allow picking a keyring based on an account included in it: this can lead to targeting the wrong keyring because of the address belonging to multiple ones (e.g. primary keyring removed instead of the second one)

Currently working repro steps:

  • Install a fresh v12.16.0 build (though this repro steps should work on other versions as well)
  • Import or create an SRP (I used child guilt hollow arrive average popular nasty soon summer like scheme diary pill country rapid)
  • Import an account that is part of the mnemonic (e.g. I used 0x80842b7e3cfb1118e86a427cdec418e3b4179ef5bbbfd71c02a76349831c8a8b which is the account at index 2 of the above SRP)
  • Add a new account on the main HD - note that you'll be able to add the account, but (1) the account is the same as the imported one (expectable) and (2) the newly added account is wrongly labeled as "imported"
  • Lock the wallet
  • Unlock it with the right password - you will see the reported error:
Error: KeyringController - The account you are trying to import is a duplicate
await in withLock		
submitPassword	@	metamask-controller.js:4343
await in submitPassword		
(anonymous)	@	createMetaRPCHandler.js:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working team-wallet-framework
Projects
None yet
2 participants