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

"--memo" attribute is misleading #9122

Closed
4 tasks
serge1peshcoff opened this issue Apr 15, 2021 · 11 comments · Fixed by #9265
Closed
4 tasks

"--memo" attribute is misleading #9122

serge1peshcoff opened this issue Apr 15, 2021 · 11 comments · Fixed by #9265
Assignees
Labels
T: Client UX T:Docs Changes and features related to documentation.
Milestone

Comments

@serge1peshcoff
Copy link

Summary

cosmos-sdk has the--memo attribute and its description doesn't describe it's public and anyone can see it. There are number of people who put their mnemonics there, exposing their wallets, probably because they though you should put a mnemonic here. See https://wasmywalletleaked.com/ and https://medium.com/frogvpn-ecosystem/how-we-found-exposed-wallets-in-cosmos-based-blockchains-a91f0ad5bb62

Problem Definition

See above.

Proposal

Two things can be done:

  1. updating the --memo description, explicitly stating memo is public
  2. renaming the --memo CLI option to something else (like --note)

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@aaronc
Copy link
Member

aaronc commented Apr 16, 2021

Wow, sounds terrible. We should definitely address this. At a minimum, we can patch the CLI to say --note or --comment.

In the next version, we should probably make a rename at the protocol layer. Any thoughts @alexanderbez @alessio ?

@alessio
Copy link
Contributor

alessio commented Apr 16, 2021

This smells to me more of a UX issue than a security vulnerability. Thus I agree on changing the CLI options names and yet I don't think this issue warrants a change in the protocol.

@serge1peshcoff
Copy link
Author

This smells to me more of a UX issue than a security vulnerability.

I agree, it's not like the core problem of the Cosmos blockchain, it's more like of a parameter that can be unknowingly used in a wrong way.

@anilcse
Copy link
Collaborator

anilcse commented Apr 17, 2021

Yes a UX issue. Renaming the client facing flags should fix the issue. May be wallets (and exchanges?) should also rename this. cc @dogemos

@dogemos
Copy link
Member

dogemos commented Apr 19, 2021

I agree that it is an issue. Something like 'destination tag' (a la XRP) could also work. 'note' and 'comment' are definitely non-typical as most other networks still call it 'memo'

That being said, we would prefer still have to show something like Note (prev. memo) as most exchanges will ask for a 'memo' and not showing that the 'note' just the 'memo' feature renamed could confuse users.

@amaury1093
Copy link
Contributor

Fixed by #9134

@aaronc
Copy link
Member

aaronc commented May 4, 2021

I don't actually think just changing this in the CLI is sufficient. It's likely a bigger issue in user interfaces and I think we need to make this more clear to UI designers. Adding the Docs label. What I would propose is:

  • comments in TxBody that the memo field should not be called memo in UIs
  • maybe some comments in other parts of docs.cosmos.network warning about this

@aaronc aaronc reopened this May 4, 2021
@aaronc aaronc added T:Docs Changes and features related to documentation. and removed T: UX labels May 4, 2021
@robert-zaremba robert-zaremba reopened this May 4, 2021
@anilcse anilcse mentioned this issue May 5, 2021
9 tasks
@mergify mergify bot closed this as completed in #9265 May 6, 2021
@xlab
Copy link

xlab commented May 14, 2021

There are number of people who put their mnemonics there

Ah, they are wrong. They should put good memes there! 🙈

On a serious side, this fix should've about documentation and

description doesn't describe it's public and anyone can see it.

description improvement.

Now there is a lot of scripts and manuals will be outdated for no reason. Also some protocols like Secret Network uses memo as part of their business logic. Note or Comment looks like it is not interpreted by app logic, but a comment that sender leaves for fun.

@xlab
Copy link

xlab commented May 14, 2021

I just wanted to be compatible with 0.43's Rosetta API and keys, not renaming memes all over the docs.

Screenshot_20210514-114857_Chrome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: Client UX T:Docs Changes and features related to documentation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants