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]: Duplicate Account Issue in MetaMask 7.23.0 on iPhone 14 #9823

Closed
fricklik opened this issue Jun 1, 2024 · 6 comments · Fixed by #9943
Closed

[Bug]: Duplicate Account Issue in MetaMask 7.23.0 on iPhone 14 #9823

fricklik opened this issue Jun 1, 2024 · 6 comments · Fixed by #9943
Assignees
Labels
external-contributor regression-prod-7.23.0 regression-RC-7.24.0 release-7.24.3 Issue or pull request that will be included in release 7.24.3 release-7.24.4 Issue or pull request that will be included in release 7.24.4 release-7.26.0 Issue or pull request that will be included in release 7.26.0 release-blocker This bug is blocking the next release Sev1-high An issue that may have caused fund loss or access to wallet in the past & may still be ongoing team-accounts type-bug Something isn't working

Comments

@fricklik
Copy link

fricklik commented Jun 1, 2024

Describe the bug

After updating MetaMask to version 7.23.0 on my iPhone 14, the same account appears duplicated in the app. When attempting to open the duplicated account, it displays the original account with the same address. A friend with an iPhone 14 Pro has experienced the exact same issue.

Expected behavior

I expected to see only one instance of each account and be able to access each account individually without duplication.

Screenshots/Recordings

Attached is a screenshot showing the duplicated accounts. The original account is highlighted with a red
314C098B-0977-4515-926D-BBE277213B74
frame.

Steps to reproduce

  1. Update MetaMask to version 7.23.0 on an iPhone 14 or iPhone 14 Pro.
  2. Open MetaMask and check the list of accounts.
  3. Notice the duplicated accounts.
  4. Try to open the duplicated account, which displays the original account instead.

Error messages or log output

No specific error messages are shown.

Version

7.23.0

Build type

None

Device

iPhone 14, iPhone 14 Pro

Operating system

iOS

Additional context

This issue affects multiple users with similar devices and the same version of MetaMask.

Severity

No response

@fricklik fricklik added the type-bug Something isn't working label Jun 1, 2024
@fricklik fricklik changed the title [Bug]: uplicate Account Issue in MetaMask 7.23.0 on iPhone 14 [Bug]: Duplicate Account Issue in MetaMask 7.23.0 on iPhone 14 Jun 1, 2024
@gauthierpetetin gauthierpetetin added Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking team-accounts labels Jun 3, 2024
@gantunesr
Copy link
Member

gantunesr commented Jun 3, 2024

Hey @fricklik, it would be really helpful if you can post a video reproducing this issue.

Edit: A quick question, from which version did you updated from?

@NicolasMassart
Copy link
Contributor

Users state logs (2 reports kept private) are showing the duplicated accounts have a checksummed address for the original one (the properly named one) and the same address but lowercased for the duplicate.

Redacted example

"accounts": {
            "XXXXXXXX-XXXX-XXXX-bXXX-XXXXXXXXXX": {
              "address": "0x0...AD",
              "id": "XXXXXXXX-XXXX-XXXX-bXXX-XXXXXXXXXX",
              "options": {},
              "metadata": {
                "name": "xxxxxx.eth",
                "keyring": { "type": "HD Key Tree" },
                "lastSelected": 1234567890123
              },
              "methods": [
                "personal_sign",
                "eth_sign",
                "eth_signTransaction",
                "eth_signTypedData_v1",
                "eth_signTypedData_v3",
                "eth_signTypedData_v4"
              ],
              "type": "eip155:eoa"
            },
            "XXXXXXXX-XXXX-XXXX-bXXX-XXXXXXXXXX": {
              "id": "XXXXXXXX-XXXX-XXXX-bXXX-XXXXXXXXXX",
              "address": "0x0...ad",
              "options": {},
              "methods": [
                "personal_sign",
                "eth_sign",
                "eth_signTransaction",
                "eth_signTypedData_v1",
                "eth_signTypedData_v3",
                "eth_signTypedData_v4"
              ],
              "type": "eip155:eoa",
              "metadata": {
                "name": "Account 7",
                "keyring": { "type": "HD Key Tree" },
                "lastSelected": 1234567890123
              }
            },

Notice "address": "0x0...AD" and "address": "0x0...ad"

@montelaidev
Copy link

I think it's a migration issue. Mobile uses checksummed addresses in the PreferencesController, but the extension uses lowercase addresses. When the AccountsController adds or removes accounts from the KeyringController, the addresses are all in lowercase. The checksummed addresses create different account IDs during the migration, and this also causes the AccountsController to create duplicate accounts since it thinks it's a new account.

@chrisleewilcox chrisleewilcox added release-blocker This bug is blocking the next release Sev1-high An issue that may have caused fund loss or access to wallet in the past & may still be ongoing and removed Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking labels Jun 6, 2024
@Razzbird8
Copy link

I have this same issue on Galaxy Note20 Ultra 5G

owencraston added a commit that referenced this issue Jun 12, 2024
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**
This PR addresses the issue of duplicate accounts showing up in the
wallet. This bug was introduced when [the accounts controller was
integrated](#8759) but
was only visible to users in [this
PR](#9733) when we
started rendering account information in the accounts list. The is that
when we ran migration `036.ts`, we simply added the all of the
identities from the preferences controller into the AccountsController.
However the accounts controller also consumes addresses from the
KeyringController and those addresses are in lowercase. The result would
be two accounts with the same address, one in checksum format and one in
lowercase format. This would result in duplicates when rendering the UI.

This PR addresses this issue in two ways...
1. for users who are using <= v7.19.0
- I have patched migration 36 to ensure that the addresses that are
added are lowercase.
2. For users who have are using version >= 7.20.0 and are already
effected by this bug
- I have created a new migration to deduplicate the accounts in the
accounts controller and ensure that we are only storing lowercase
addresses in state.

## **Related issues**

Fixes: #9823

## QA Builds
- [Current
branch](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/c68d80ea-dbaa-4e81-9018-fe9757cab397)
-
[7.18.0](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/011ac0b3-b1d8-4e53-9595-ea0f739d4d10)
-
[7.24.0](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/a5c54c50-8382-45ad-b203-687b66273c4e)

## **Manual testing steps**

### Path 1: User has duplicate accounts already
1. Install
[7.18.0](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/011ac0b3-b1d8-4e53-9595-ea0f739d4d10)
and import a wallet with multiple addresses already
2. Install
[7.24.0](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/a5c54c50-8382-45ad-b203-687b66273c4e)
and upgrade app.
3. EXPECTED: You should see duplicate accounts when you open the
accounts list. If you had 5 accounts previously there will be should now
be 10.
4. Install [the current branch QA
build](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/c68d80ea-dbaa-4e81-9018-fe9757cab397)
and upgrade.
5. EXPECT: The duplicate accounts should be removed (back to 5 accounts
from 10)
6. EXPECT: The previously selected address should still be selected
7. EXPECT: The account names should be in tact and in chronological
order if they are the default account name.

### Path 2: User has not seen duplicate accounts yet
this occurs for users who have not upgraded from 7.19.0 yet
1. Install
[7.18.0](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/011ac0b3-b1d8-4e53-9595-ea0f739d4d10)
and import a wallet with multiple addresses already
2. Install [the current branch QA
build](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/c68d80ea-dbaa-4e81-9018-fe9757cab397)
and upgrade.
3. EXPECT: All of the account should appear in the accounts list without
duplicates
4. EXPECT: The previously selected address should be the same
5. EXPECT: The account names should be the same

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

7.18.0

<img
src="https://github.com/MetaMask/metamask-mobile/assets/22918444/3ee9b1f3-f042-414b-9ce4-82cad7d791cf"
width="200" height="400">

After Upgrading to 7.24.0

<img
src="https://github.com/MetaMask/metamask-mobile/assets/22918444/47b5ff3d-d2d0-4373-b8a0-ae0007b19b7b"
width="200" height="400">


### **After**

Upgrading to this branch

<img
src="https://github.com/MetaMask/metamask-mobile/assets/22918444/9ed8d80a-ef3c-40a6-9152-e661a1ecdd25"
width="200" height="400">

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [x] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
@metamaskbot metamaskbot added the release-7.26.0 Issue or pull request that will be included in release 7.26.0 label Jun 12, 2024
owencraston added a commit that referenced this issue Jun 12, 2024
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

This PR addresses the issue of duplicate accounts showing up in the
wallet. This bug was introduced when [the accounts controller was
integrated](#8759) but
was only visible to users in [this
PR](#9733) when we
started rendering account information in the accounts list. The is that
when we ran migration `036.ts`, we simply added the all of the
identities from the preferences controller into the AccountsController.
However the accounts controller also consumes addresses from the
KeyringController and those addresses are in lowercase. The result would
be two accounts with the same address, one in checksum format and one in
lowercase format. This would result in duplicates when rendering the UI.

This PR addresses this issue in two ways...
1. for users who are using <= v7.19.0
- I have patched migration 36 to ensure that the addresses that are
added are lowercase.
2. For users who have are using version >= 7.20.0 and are already
effected by this bug
- I have created a new migration to deduplicate the accounts in the
accounts controller and ensure that we are only storing lowercase
addresses in state.

Fixes: #9823

- [Current
branch](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/c68d80ea-dbaa-4e81-9018-fe9757cab397)
-
[7.18.0](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/011ac0b3-b1d8-4e53-9595-ea0f739d4d10)
-
[7.24.0](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/a5c54c50-8382-45ad-b203-687b66273c4e)

1. Install
[7.18.0](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/011ac0b3-b1d8-4e53-9595-ea0f739d4d10)
and import a wallet with multiple addresses already
2. Install
[7.24.0](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/a5c54c50-8382-45ad-b203-687b66273c4e)
and upgrade app.
3. EXPECTED: You should see duplicate accounts when you open the
accounts list. If you had 5 accounts previously there will be should now
be 10.
4. Install [the current branch QA
build](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/c68d80ea-dbaa-4e81-9018-fe9757cab397)
and upgrade.
5. EXPECT: The duplicate accounts should be removed (back to 5 accounts
from 10)
6. EXPECT: The previously selected address should still be selected
7. EXPECT: The account names should be in tact and in chronological
order if they are the default account name.

this occurs for users who have not upgraded from 7.19.0 yet
1. Install
[7.18.0](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/011ac0b3-b1d8-4e53-9595-ea0f739d4d10)
and import a wallet with multiple addresses already
2. Install [the current branch QA
build](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/c68d80ea-dbaa-4e81-9018-fe9757cab397)
and upgrade.
3. EXPECT: All of the account should appear in the accounts list without
duplicates
4. EXPECT: The previously selected address should be the same
5. EXPECT: The account names should be the same

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

7.18.0

<img
src="https://github.com/MetaMask/metamask-mobile/assets/22918444/3ee9b1f3-f042-414b-9ce4-82cad7d791cf"
width="200" height="400">

After Upgrading to 7.24.0

<img
src="https://github.com/MetaMask/metamask-mobile/assets/22918444/47b5ff3d-d2d0-4373-b8a0-ae0007b19b7b"
width="200" height="400">

Upgrading to this branch

<img
src="https://github.com/MetaMask/metamask-mobile/assets/22918444/9ed8d80a-ef3c-40a6-9152-e661a1ecdd25"
width="200" height="400">

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

- [x] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
sethkfman added a commit that referenced this issue Jun 13, 2024
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

This PR addresses the issue of duplicate accounts showing up in the
wallet. This bug was introduced when [the accounts controller was
integrated](#8759) but
was only visible to users in [this
PR](#9733) when we
started rendering account information in the accounts list. The is that
when we ran migration `036.ts`, we simply added the all of the
identities from the preferences controller into the AccountsController.
However the accounts controller also consumes addresses from the
KeyringController and those addresses are in lowercase. The result would
be two accounts with the same address, one in checksum format and one in
lowercase format. This would result in duplicates when rendering the UI.

This PR addresses this issue in two ways...
1. for users who are using <= v7.19.0
- I have patched migration 36 to ensure that the addresses that are
added are lowercase.
2. For users who have are using version >= 7.20.0 and are already
effected by this bug
- I have created a new migration to deduplicate the accounts in the
accounts controller and ensure that we are only storing lowercase
addresses in state.

Fixes: #9823

- [Current
branch](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/c68d80ea-dbaa-4e81-9018-fe9757cab397)
-

[7.18.0](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/011ac0b3-b1d8-4e53-9595-ea0f739d4d10)
-

[7.24.0](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/a5c54c50-8382-45ad-b203-687b66273c4e)

1. Install
[7.18.0](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/011ac0b3-b1d8-4e53-9595-ea0f739d4d10)
and import a wallet with multiple addresses already
2. Install
[7.24.0](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/a5c54c50-8382-45ad-b203-687b66273c4e)
and upgrade app.
3. EXPECTED: You should see duplicate accounts when you open the
accounts list. If you had 5 accounts previously there will be should now
be 10.
4. Install [the current branch QA
build](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/c68d80ea-dbaa-4e81-9018-fe9757cab397)
and upgrade.
5. EXPECT: The duplicate accounts should be removed (back to 5 accounts
from 10)
6. EXPECT: The previously selected address should still be selected
7. EXPECT: The account names should be in tact and in chronological
order if they are the default account name.

this occurs for users who have not upgraded from 7.19.0 yet
1. Install
[7.18.0](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/011ac0b3-b1d8-4e53-9595-ea0f739d4d10)
and import a wallet with multiple addresses already
2. Install [the current branch QA
build](https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/c68d80ea-dbaa-4e81-9018-fe9757cab397)
and upgrade.
3. EXPECT: All of the account should appear in the accounts list without
duplicates
4. EXPECT: The previously selected address should be the same
5. EXPECT: The account names should be the same

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

7.18.0

<img

src="https://github.com/MetaMask/metamask-mobile/assets/22918444/3ee9b1f3-f042-414b-9ce4-82cad7d791cf"
width="200" height="400">

After Upgrading to 7.24.0

<img

src="https://github.com/MetaMask/metamask-mobile/assets/22918444/47b5ff3d-d2d0-4373-b8a0-ae0007b19b7b"
width="200" height="400">

Upgrading to this branch

<img

src="https://github.com/MetaMask/metamask-mobile/assets/22918444/9ed8d80a-ef3c-40a6-9152-e661a1ecdd25"
width="200" height="400">

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding

Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

- [x] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: sethkfman <10342624+sethkfman@users.noreply.github.com>
@metamaskbot metamaskbot added the release-7.24.3 Issue or pull request that will be included in release 7.24.3 label Jun 19, 2024
@metamaskbot
Copy link
Collaborator

Missing release label release-7.24.3 on issue. Adding release label release-7.24.3 on issue, as issue is linked to PR #9943 which has this release label.

@metamaskbot metamaskbot added the release-7.24.4 Issue or pull request that will be included in release 7.24.4 label Jun 24, 2024
@metamaskbot
Copy link
Collaborator

Missing release label release-7.24.4 on issue. Adding release label release-7.24.4 on issue, as issue is linked to PR #9943 which has this release label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-contributor regression-prod-7.23.0 regression-RC-7.24.0 release-7.24.3 Issue or pull request that will be included in release 7.24.3 release-7.24.4 Issue or pull request that will be included in release 7.24.4 release-7.26.0 Issue or pull request that will be included in release 7.26.0 release-blocker This bug is blocking the next release Sev1-high An issue that may have caused fund loss or access to wallet in the past & may still be ongoing team-accounts type-bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

9 participants