MetaMask / metamask-extension

:globe_with_meridians: :electric_plug: The MetaMask browser extension enables browsing Ethereum blockchain enabled websites
https://metamask.io
Other
11.87k stars 4.84k forks source link

[Bug]: Sign types fire an Approved event when I reject the signature on Hardware Wallets #18361

Open seaona opened 1 year ago

seaona commented 1 year ago

Describe the bug

Problem: when I check the Metametrics events for any Signature using a hardware wallet (Trezor/Ledger), if I cancel the signature I can see how an Approve event is fired

https://user-images.githubusercontent.com/54408225/228551196-702f9135-db5a-49e2-b0ca-6ba39d633c04.mp4

Steps to reproduce

  1. Import a Hardware Wallet
  2. Connect to test-dapp
  3. Click to any signature
  4. Cancel signature
  5. Check Metametrics events - an Approved event is registered

Error messages or log output

No response

Version

10.26.2

Build type

None

Browser

Chrome

Operating system

Linux

Hardware wallet

No response

Additional context

No response

digiwand commented 1 year ago

I think the canceled RPC method with the hardware wallets is returning a different response than those handled in the following code:

app/scripts/lib/createRPCMethodTrackingMiddleware.js

      if (isDisabledRPCMethod) {
        event = eventType.FAILED;
        eventProperties.error = res.error;
      } else if (res.error?.code === errorCodes.provider.userRejectedRequest) {
        event = eventType.REJECTED;
      } else {
        event = eventType.APPROVED;
      }

RPC responses not handled here then default to the approved type. maybe we can explicitly handle the approved type, and record an error for any outliers

seaona commented 11 months ago

Notice this issue will also affect metrics for malicious signatures with hardware wallets. See example below, where I rejected the signature with the hardware wallet, but we see it as approved a malicious signature

Screenshot from 2023-10-18 11-53-13

github-actions[bot] commented 8 months ago

This issue has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 45 days if there is no further activity. The MetaMask team intends on reviewing this issue before close, and removing the stale label if it is still a bug. We welcome new comments on this issue. We do not intend on closing issues if they report bugs that are still reproducible. Thank you for your contributions.

github-actions[bot] commented 1 month ago

This issue has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 45 days if there is no further activity. The MetaMask team intends on reviewing this issue before close, and removing the stale label if it is still a bug. We welcome new comments on this issue. We do not intend on closing issues if they report bugs that are still reproducible. Thank you for your contributions.