urbit / archeology-bridge

bridge
MIT License
7 stars 4 forks source link

Needs to be more informative about security & best practices #12

Open Fang- opened 6 years ago

Fang- commented 6 years ago

MEW added a bunch of blurps to the different login methods telling users about why using a private key directly might be a bad idea, and why hardware wallets are good. See here. In general, their site contains a bunch of similar copy warning the user to stay safe and how to make sure they aren't being scammed.

I feel this is important to do when providing an accessible interface for potentially high-value operations. Especially if we're also going to serve it over our own web domain (which may or may not be a good idea, looking at how many MEW scam attempts there have already been using subtle URL changes as an attack vector).

It's less aesthetic, but I'd be very willing to give that up if it means educating users about proper security practices. Considering not all MUW users will be familiar with Ethereum, we sort of have an obligation to, don't we?

vvisigoth commented 6 years ago

This is a good point, Mark.

We’ve talked a little bit about this, but I think that the amount of warnings, etc. will depend on our “average user.” It might be safe to assume that Urbit is their only crypto asset.

On Wed, Jan 10, 2018 at 2:00 AM Fang notifications@github.com wrote:

MEW added a bunch of blurps to the different login methods telling users about why using a private key directly might be a bad idea, and why hardware wallets are good. See here https://www.myetherwallet.com/#send-transaction. In general, their site contains a bunch of similar copy warning the user to stay safe and how to make sure they aren't being scammed.

I feel this is important to do when providing an accessible interface for potentially high-value operations. Especially if we're also going to serve it over our own web domain (which may or may not be a good idea, looking at how many MEW scam attempts there have already been using subtle URL changes as an attack vector).

It's less aesthetic, but I'd be very willing to give that up if it means educating users about proper security practices. Considering not all MUW users will be familiar with Ethereum, we sort of have an obligation to, don't we?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/urbit/etherwallet/issues/12, or mute the thread https://github.com/notifications/unsubscribe-auth/ABiQ4xHMWqEbs_WtfMF5SxsHi_dBl99yks5tJIpGgaJpZM4RZEnU .

cgyarvin commented 6 years ago

I wonder if we should get our wallet code audited as well?

Sent from my iPhone

On Jan 10, 2018, at 10:27 AM, vvisigoth notifications@github.com wrote:

This is a good point, Mark.

We’ve talked a little bit about this, but I think that the amount of warnings, etc. will depend on our “average user.” It might be safe to assume that Urbit is their only crypto asset.

On Wed, Jan 10, 2018 at 2:00 AM Fang notifications@github.com wrote:

MEW added a bunch of blurps to the different login methods telling users about why using a private key directly might be a bad idea, and why hardware wallets are good. See here https://www.myetherwallet.com/#send-transaction. In general, their site contains a bunch of similar copy warning the user to stay safe and how to make sure they aren't being scammed.

I feel this is important to do when providing an accessible interface for potentially high-value operations. Especially if we're also going to serve it over our own web domain (which may or may not be a good idea, looking at how many MEW scam attempts there have already been using subtle URL changes as an attack vector).

It's less aesthetic, but I'd be very willing to give that up if it means educating users about proper security practices. Considering not all MUW users will be familiar with Ethereum, we sort of have an obligation to, don't we?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/urbit/etherwallet/issues/12, or mute the thread https://github.com/notifications/unsubscribe-auth/ABiQ4xHMWqEbs_WtfMF5SxsHi_dBl99yks5tJIpGgaJpZM4RZEnU .

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

Fang- commented 6 years ago

Audited for what, exactly?

The logic powering transaction creation and signing should still be unchanged from MEW (been around for a while, good reputation etc), so as long as we can verify that the code only ever creates the transactions we tell it to create, and that what's sent out is equal to what's displayed, we're good.

Of course you also want to make sure all the contract-side checks are mirrored locally so that we don't waste the user's gas on a failing transaction. It might be nice to write proper fake-user tests to make sure everything's covered thoroughly. The coverage of these just has to match the coverage of the contracts' tests.

cgyarvin commented 6 years ago

Exactly -- verify that the code only ever creates the transactions we tell it to create, and that what's sent out is equal to what's displayed. If it's trivial, it's easy...

On Wed, Jan 10, 2018 at 11:16 AM, Fang notifications@github.com wrote:

Audited for what, exactly?

The logic powering transaction creation and signing should still be unchanged from MEW (been around for a while, good reputation etc), so as long as we can verify that the code only ever creates the transactions we tell it to create, and that what's sent out is equal to what's displayed, we're good.

Of course you also want to make sure all the contract-side checks are mirrored locally so that we don't waste the user's gas on a failing transaction. It might be nice to write proper fake-user tests to make sure everything's covered thoroughly. The coverage of these just has to match the coverage of the contracts' tests.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/urbit/etherwallet/issues/12#issuecomment-356706940, or mute the thread https://github.com/notifications/unsubscribe-auth/AALyAReI_DJekCeRShFdVi4nMEbDKb77ks5tJQypgaJpZM4RZEnU .

Fang- commented 6 years ago

@vvisigoth, correct me if I'm wrong, but I'm fairly sure the transaction generation and all data that comes out of it never leaves these three functions.
The uiFuncs they call out to are a copy of MEW's (and still up-to-date at that). We could walk through that and match it against the Ethereum spec, I guess. Not that I've ever heard about MEW sending bogus transactions.

vvisigoth commented 6 years ago

Yes, Mark, the transaction are contained to those functions and those functions are untouched.

As for auditing, I do know that MEW’s reputation is good, but I don’t know how formalized their verification and validation is. Perhaps if they’ve done formal v&v we can prove that the applicable code is equivalent?

On Wed, Jan 10, 2018 at 11:36 AM Fang notifications@github.com wrote:

@vvisigoth https://github.com/vvisigoth, correct me if I'm wrong, but I'm fairly sure the transaction generation and all data that comes out of it never leaves these three functions https://github.com/urbit/etherwallet/blob/mercury/app/scripts/controllers/urbitCtrl.js#L167-L241 . The uiFuncs they call out to are a copy of MEW's (and still up-to-date at that). We could walk through that and match it against the Ethereum spec, I guess. Not that I've ever heard about MEW sending bogus transactions.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/urbit/etherwallet/issues/12#issuecomment-356712569, or mute the thread https://github.com/notifications/unsubscribe-auth/ABiQ4_Xw81eMzUJz3Bq4klDFh_5ORF_qks5tJREwgaJpZM4RZEnU .