Skip to content

KeyringController does not prevent duplicate accounts properly #5411

Closed
@mikesposito

Description

@mikesposito

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

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions