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

Enable users to revoke allowances in their activity history #17464

Open
bschorchit opened this issue Jan 27, 2023 · 2 comments
Open

Enable users to revoke allowances in their activity history #17464

bschorchit opened this issue Jan 27, 2023 · 2 comments
Labels

Comments

@bschorchit
Copy link

bschorchit commented Jan 27, 2023

Background

For a very long time we have been wanting to empower users to review and revoke the permissions they have given (see #8174 and #10331 as examples). We haven't got to this yet, but there's a small scope, low hanging fruit work that we can do in the mean time.

Recently, a phishing victim realized while they were being drained, and to save their money the time to revoke was critical. If that user had an easy access revoke option in their recent approve transactions that would increase their chance of revoking the allowance in time. This is the win we're trying to achieve with this issue. It won't be something that will work for everyone and in all situations, but for some users it might be just enough.

Design file

Figma link (WIP)

Requirements and things to consider

  1. This should be implemented for ERC20 approve, ERC721 approve, ERC721 setApprovalForAll, ERC1155 approve and ERC1155 setApprovalForAll.
  2. For each of those allowances in the transaction history, we should check wether it was indeed giving allowance or if it was a revoking allowance (see section below).
    2.a. If it was giving allowance, we'll display a revoke button (per design above) in that activity history line.
    2.b. If it was revoking allowance, we won't display any button.
  3. If a user clicks on the revoke button and submits the revoke transaction, we'll hide (? TBD) that button. It's also TBD wether we also want to hide it If a user revokes that allowance elsewhere. How much additional effort this would add to scope? Let's discuss this.

What giving allowance and revoking allowance look like for each method and contract

Note: for all below, spenderAddress is the address that is receiving or has received the allowance.

  • ERC20 approve
    giving allowance: approve(spenderAddress, amount)
    revoking allowance: approve(spenderAddress, 0) - you revoke by setting the amount to 0 for that specific spenderAddress.

  • ERC721 & ERC1155 approve
    giving allowance: approve(spenderAddress, tokenId)
    revoking allowance: approve(zeroAddress, tokenId) - you revoke by setting a new spender, like the burner address. There can always only be a single spender using this method.

  • ERC721 & ERC1155 setApprovalForAll
    giving allowance: setApprovalForAll(spenderAddress, true)
    revoking allowance: setApprovalForAll((spenderAddress, false) - you revoke by setting the approved(bool) parameter to false.

Reference

Screenshot of new design

image (6)

@bschorchit bschorchit added type-enhancement type-security team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead area-activity labels Jan 27, 2023
@bschorchit
Copy link
Author

De-prioritizing this as we'll be able to leverage a pdapp feature for this in the near future.

@hilvmason hilvmason added team-confirmations-planning (only for internal use within Confirmations team) and removed team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead labels Jun 6, 2023
@Keboletsetse
Copy link

Background

For a very long time we have been wanting to empower users to review and revoke the permissions they have given (see #8174 and #10331 as examples). We haven't got to this yet, but there's a small scope, low hanging fruit work that we can do in the mean time.

Recently, a phishing victim realized while they were being drained, and to save their money the time to revoke was critical. If that user had an easy access revoke option in their recent approve transactions that would increase their chance of revoking the allowance in time. This is the win we're trying to achieve with this issue. It won't be something that will work for everyone and in all situations, but for some users it might be just enough.

Design file

Figma link (WIP)

Requirements and things to consider

  1. This should be implemented for ERC20 approve, ERC721 approve, ERC721 setApprovalForAll, ERC1155 approve and ERC1155 setApprovalForAll.
  2. For each of those allowances in the transaction history, we should check wether it was indeed giving allowance or if it was a revoking allowance (see section below).
    2.a. If it was giving allowance, we'll display a revoke button (per design above) in that activity history line.
    2.b. If it was revoking allowance, we won't display any button.
  3. If a user clicks on the revoke button and submits the revoke transaction, we'll hide (? TBD) that button. It's also TBD wether we also want to hide it If a user revokes that allowance elsewhere. How much additional effort this would add to scope? Let's discuss this.

What giving allowance and revoking allowance look like for each method and contract

Note: for all below, spenderAddress is the address that is receiving or has received the allowance.

  • ERC20 approve
    giving allowance: approve(spenderAddress, amount)
    revoking allowance: approve(spenderAddress, 0) - you revoke by setting the amount to 0 for that specific spenderAddress.

  • ERC721 & ERC1155 approve
    giving allowance: approve(spenderAddress, tokenId)
    revoking allowance: approve(zeroAddress, tokenId) - you revoke by setting a new spender, like the burner address. There can always only be a single spender using this method.

  • ERC721 & ERC1155 setApprovalForAll
    giving allowance: setApprovalForAll(spenderAddress, true)
    revoking allowance: setApprovalForAll((spenderAddress, false) - you revoke by setting the approved(bool) parameter to false.

Reference

Screenshot of new design

image (6)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants