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

Display 8/8 address chars during signing instead of 4/4 #914

Merged
merged 4 commits into from
Jun 29, 2022

Conversation

stoooops
Copy link
Contributor

@stoooops stoooops commented Jun 28, 2022

Overview

Patch UI to display 8 leading and 8 trailing characters for transaction request instead of 4 and 4.

Motivation

This fixes security issues around address collisions which match first/last 4 characters. 8 digits is not enough and can be bruteforced locally.

For the forseeable future, 16 characters of collision protection should be reasonable. Extrapolating from 2019 data provided by (@)banteg:
image

New Look

image

Old Look

image

Security Incident

image

https://twitter.com/Alexintosh/status/1540047636467748870

Author

Patched written by cory.eth (Cory Gabrielsen) (@cory_eth).

@banteg
Copy link

banteg commented Jun 28, 2022

6 leading and 6 trailing maybe?

profanity can go through 179 mh/s according to a 2019 benchmark, it would take it

  • 24 seconds to find matching 8 chars
  • 1.7 hours for 10 chars
  • 18.2 days for 12 chars

a generative image alongside would also make it harder to spoof, since it depends on all the bits of the address

@uneventfulhorizon
Copy link

It's a rushed/lazy attacker that can't set aside 18 days to generate the appropriate vanity address?

@aviggiano
Copy link

I think the root cause problem is not knowing if you're interacting with a verified (by who) contract or not.

Maybe integrating with the etherscan API, getting that information and displaying it in the UI?

@outdoteth
Copy link

It's easy for an attacker to verify the contract on etherscan too, and gives a false sense of security if there is a "verified" or green check mark in the UI imo. Although having the information that a contract verified is still useful.

@aviggiano
Copy link

I agree.
Maybe rephrasing my point with your remarks:

The root cause is not knowing if you're interacting with a verified smart contract recognized by the team.
When alexintosh.eth says "the correct one is..." he means "the contract Convex deployed and publicly acknowledged is"

So maybe a robust solution would involve having a repository of "approved" smart contracts + a verification checkmark.

@mholtzman
Copy link
Collaborator

@stoooops thanks for the submission. the code you changed displays the recipient address after the transaction has been broadcast so not as sensitive, but I updated the PR with the 6 character standard in both places Frame prompts for user confirmation of a transaction before signing (normal transaction signing and approve ERC-20 token spend). we'll get this merged and released ASAP

@stoooops
Copy link
Contributor Author

Great, I actually was just looking at this again. Was just working on getting it to build w/ my changes so I could take some screenshots.

I was thinking about whether 8/8 is better than 6/6 but I wanted to see it visually.

@stoooops
Copy link
Contributor Author

Here is the visualization with 6...6 vs 8...8. I think there is still plenty of whitespace and it looks fine with 8/8, and clearly the security is 4 chars (2 bytes) stronger.

image
image

I've added a further change to increase it to 8/8 which I think would be my vote.

You can change it back to 6/6 if you prefer as well.

@stoooops stoooops changed the title Display 6 leading addr chars instead of 4 Display 8/8 address chars during signing instead of 4/4 Jun 29, 2022
@stoooops
Copy link
Contributor Author

stoooops commented Jun 29, 2022

Extrapolating @banteg's data (based on 178,205,000 keys/sec collision attack) to 16 characters suggests a collision time of 3,261 years.
image

If we assume the attacker can do ~6x faster at 1,000,000,000 keys/sec, then it would still take 584 years.
image

(Google Sheet: link)

For the forseeable future (3-5 years), it would take an attacker a considerable amount of compute ($$$) to generate a full 16-character collision. Every reduction in collision is a better chance a user spots the delta and we save someone from losing their funds.

I think for this UX use case, 8/8 is subjectively nice looking while still being strong security for the forseeable future.

@mholtzman
Copy link
Collaborator

keep in mind that in all these cases, users can always view the full addresses both in Frame and on their devices if they're using one. I think 8/8 is good to settle on.

@mholtzman mholtzman merged commit 32d9bc7 into floating:0.5 Jun 29, 2022
mholtzman added a commit that referenced this pull request Jul 1, 2022
* Display 6 leading addr chars instead of 4

* show 6 leading and trailing digits on transaction confirms

* Increase display to 8/8

* create constant

Co-authored-by: Matt Holtzman <matt.holtzman@gmail.com>
mholtzman added a commit that referenced this pull request Jul 1, 2022
* Display 6 leading addr chars instead of 4

* show 6 leading and trailing digits on transaction confirms

* Increase display to 8/8

* create constant

Co-authored-by: Matt Holtzman <matt.holtzman@gmail.com>
goosewobbler pushed a commit that referenced this pull request Jul 2, 2022
* Display 6 leading addr chars instead of 4

* show 6 leading and trailing digits on transaction confirms

* Increase display to 8/8

* create constant

Co-authored-by: Matt Holtzman <matt.holtzman@gmail.com>
mholtzman added a commit that referenced this pull request Jul 8, 2022
* Display 6 leading addr chars instead of 4

* show 6 leading and trailing digits on transaction confirms

* Increase display to 8/8

* create constant

Co-authored-by: Matt Holtzman <matt.holtzman@gmail.com>
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.

6 participants