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

Add revoke permissions #26

Merged
merged 20 commits into from
Nov 22, 2023
Merged

Add revoke permissions #26

merged 20 commits into from
Nov 22, 2023

Conversation

shanejonas
Copy link
Contributor

@shanejonas shanejonas commented Nov 16, 2023

Overview

The proposed MIP introduces a new JSON-RPC method, wallet_revokePermissions, to MetaMask. This method aims to empower users with more control over permission management by offering the ability to revoke permissions associated with connected dApps.

MIPs/mip-x.md Outdated Show resolved Hide resolved
MIPs/mip-x.md Outdated Show resolved Hide resolved
MIPs/mip-x.md Outdated Show resolved Hide resolved
MIPs/mip-x.md Outdated Show resolved Hide resolved
MIPs/mip-x.md Outdated Show resolved Hide resolved
MIPs/mip-x.md Outdated Show resolved Hide resolved
MIPs/mip-x.md Outdated Show resolved Hide resolved
MIPs/mip-x.md Outdated Show resolved Hide resolved
MIPs/mip-x.md Outdated Show resolved Hide resolved
MIPs/mip-x.md Outdated Show resolved Hide resolved
MIPs/mip-x.md Outdated Show resolved Hide resolved
MIPs/mip-x.md Outdated Show resolved Hide resolved
MIPs/mip-x.md Outdated Show resolved Hide resolved
MIPs/mip-x.md Outdated Show resolved Hide resolved
shanejonas and others added 2 commits November 17, 2023 06:48
Co-authored-by: Alex Donesky <adonesky@gmail.com>
MIPs/mip-x.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

## Security Considerations
The introduction of the `wallet_revokePermissions` method bolsters security by providing users with more control over permission revocation, reducing the potential attack surface. By allowing users to revoke permissions either partially or entirely, it minimizes the risk of unauthorized or malicious activity.

However, the security model depends on users actively managing these permissions, and there's a minor risk that poorly implemented dApps could confuse users about what they're revoking, potentially leading to unwanted outcomes. Ex. If an attacker had XSS access to a site, it could revoke permissions without the users consent.
Copy link
Collaborator

@jiexi jiexi Nov 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems more like an inconvenience than an actual threat. If an attacker could run arbitrary code on the page for a Dapp, it seems more likely that they would make requests as is or even request more permissions rather than revoke them. I suppose it would be possible that permissions are revoked on the real Dapp in an effort to confuse the user and get them to approve them on a phishing Dapp? Or perhaps constantly revoking permissions that the real Dapp requests and then attempting to slip in a malicious request for permissions among the many reapproval prompts that may prompt the user? Idk

This is already labeled as a minor risk though


To mitigate this risk, MetaMask could implement the following countermeasures:

**User Education**: MetaMask could inform users about the importance of managing permissions and the risks associated with not revoking outdated or unused permissions. Offer guidelines for making informed decisions about when and how to revoke permissions for different dApps.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something we should consider in the future is keeping track of the last time a permission was used by a Dapp and periodically suggesting a list of stale permissions to the user that can be revoked (or even just automatically revoking).

MIPs/mip-x.md Outdated Show resolved Hide resolved
MIPs/mip-x.md Outdated Show resolved Hide resolved
@shanejonas shanejonas merged commit baf2795 into main Nov 22, 2023
2 checks passed
@shanejonas shanejonas deleted the add-revoke-permissions branch November 22, 2023 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants