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

[Feature]: disable authz grant message grant #19737

Closed
tac0turtle opened this issue Mar 12, 2024 · 7 comments · Fixed by #20687
Closed

[Feature]: disable authz grant message grant #19737

tac0turtle opened this issue Mar 12, 2024 · 7 comments · Fixed by #20687

Comments

@tac0turtle
Copy link
Member

Summary

disable authz grant of authz grant to avoid individuals accidentally authzing their entire account to a different account. We should provide this via accounts and sub accounts

Problem Definition

No response

Proposed Feature

disable message grant to be authzed

@alagiz
Copy link

alagiz commented Mar 13, 2024

i certainly support it, we help scammed users to try and get back whatever possible from their staked/unstaking coins - this grant to issue grants makes it pretty difficult if scammers are a little tech-savvy.

@arlai-mk
Copy link

I hadn't seen this feature request before, and I posted the below to Keplr repo, asking them to issue a warning before letting users sign GenericAuthorization with MsgGrant (even MsgSend, especially if combined with MsgUndelegate and MsgTokenizeShares):
chainapsis/keplr-wallet#1084

If there is no valid case where we would want to allow the grantee to be able to run MsgGrant transactions, then yes, disabling it would surely go a long way in protecting users. I am not sure that there is no legit case, but can't think of any myself.

Also, I was going to ask for a MsgRevokeAll message, as I saw a case where an attacker, right after receiving the MsgGrant authorization, created grants for 2000 other addresses under its control (one example tx: https://www.mintscan.io/cosmos/tx/5C21BC64CF0FF8502DCF1DD623973FDB939B3519259881135E5861CC5BA16157?height=19607416).
Currently, revoking grants need to be done one by one (so you need a UI to list them and then revoke them all), which makes it impossible to protect yourself after the authorization has been granted to 2000 addresses.

So, in case you keep the MsgGrant option in GenericAuthorization, maybe a MsgRevokeAll message can be considered? Please let me know if you think I should create a separate issue for this feature request, or it can be kept within the discussion of this current issue.

Thanks.

@Cordtus
Copy link

Cordtus commented Apr 9, 2024

@arlai-mk Keplr extension for quite some time now gives a warning as well as an additional mandatory tickbox to the user when they are asked to sign any "potentially harmful" authz tx.
On the mobile app these transactions just seem to crash the app, not sure if this is intentional or just a convenient glitch though.

Anyway, adding to this issue/suggestion:

There is no reason to place hundreds or literally thousands of authz grants on a single account. I'm not sure what a reasonable number may be, but this should be limited. 100 seems adequate, but I will ask someone with a better idea to comment further.

for context: https://www.mintscan.io/cosmos/tx/0D8F9EFCF3475FE159268E612717A909B38001D547AB6A53A3BC51AB039DE871?height=19115577

@kakucodes
Copy link

@arlai-mk Keplr extension for quite some time now gives a warning as well as an additional mandatory tickbox to the user when they are asked to sign any "potentially harmful" authz tx. On the mobile app these transactions just seem to crash the app, not sure if this is intentional or just a convenient glitch though.

Anyway, adding to this issue/suggestion:

There is no reason to place hundreds or literally thousands of authz grants on a single account. I'm not sure what a reasonable number may be, but this should be limited. 100 seems adequate, but I will ask someone with a better idea to comment further.

for context: https://www.mintscan.io/cosmos/tx/0D8F9EFCF3475FE159268E612717A909B38001D547AB6A53A3BC51AB039DE871?height=19115577

Talking through this with @Cordtus it might make sense to introduce a new MsgRevoke variant that revokes all grants in one call so that a wallet can always restore it's sovereignty.

@julienrbrt
Copy link
Member

@arlai-mk Keplr extension for quite some time now gives a warning as well as an additional mandatory tickbox to the user when they are asked to sign any "potentially harmful" authz tx. On the mobile app these transactions just seem to crash the app, not sure if this is intentional or just a convenient glitch though.
Anyway, adding to this issue/suggestion:
There is no reason to place hundreds or literally thousands of authz grants on a single account. I'm not sure what a reasonable number may be, but this should be limited. 100 seems adequate, but I will ask someone with a better idea to comment further.
for context: https://www.mintscan.io/cosmos/tx/0D8F9EFCF3475FE159268E612717A909B38001D547AB6A53A3BC51AB039DE871?height=19115577

Talking through this with @Cordtus it might make sense to introduce a new MsgRevoke variant that revokes all grants in one call so that a wallet can always restore it's sovereignty.

Can you create an issue about this? MsgRevokeAll sounds like a good message to add.

@arlai-mk
Copy link

@arlai-mk Keplr extension for quite some time now gives a warning as well as an additional mandatory tickbox to the user when they are asked to sign any "potentially harmful" authz tx. On the mobile app these transactions just seem to crash the app, not sure if this is intentional or just a convenient glitch though.
Anyway, adding to this issue/suggestion:
There is no reason to place hundreds or literally thousands of authz grants on a single account. I'm not sure what a reasonable number may be, but this should be limited. 100 seems adequate, but I will ask someone with a better idea to comment further.
for context: https://www.mintscan.io/cosmos/tx/0D8F9EFCF3475FE159268E612717A909B38001D547AB6A53A3BC51AB039DE871?height=19115577

Talking through this with @Cordtus it might make sense to introduce a new MsgRevoke variant that revokes all grants in one call so that a wallet can always restore it's sovereignty.

Can you create an issue about this? MsgRevokeAll sounds like a good message to add.

It's done, please see issue #20139

@PFC-developer
Copy link

my 2c's

I believe this should be a wallet level concern not a SDK one. (the disabling of authz)

there are several places where grants are part of the tech design of applications.

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

Successfully merging a pull request may close this issue.

8 participants