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

Added GreenBits wallet #725

Closed
wants to merge 1 commit into from

Conversation

@greenaddress
Copy link
Contributor

greenaddress commented Jan 30, 2015

This change adds GreenBits to the mobile wallet list for Android.

GreenBits is a native Android Bitcoin Wallet app based on BitcoinJ's latest stable release.

The wallet is BIP32, supports BIP70 and other payment protocol related BIPS, supports a variety of hardware wallets (Ledger, Trezor and anything that uses the same interface), uses P2SH multisignature in 2of2 and 2of3 forms with a multisig oracle (GreenAddress) while validating all data via SPV and generally aiming for zero/minimal trust including minimal permissions involved with the app on the stores.

The app has been in a internal Beta for a couple of months and in a public Beta on Play for a month, the [open] source code is available on GitHub and the app is also available on F-Droid.

While the app is new, the oracle service it uses has been in operation since 2013-04-16 and on bitcoin.org with HTML5 hybrid apps since April 2014.

Let us know if you have any questions or suggestions.

@saivann saivann added the Wallets label Feb 1, 2015
@harding harding self-assigned this Feb 6, 2015
@harding

This comment has been minimized.

Copy link
Contributor

harding commented Feb 6, 2015

@greenaddress thanks for creating and submitting GreenBits!

I just began reviewing GrennBits in an emulator on my laptop when I noticed that my home broadband Internet connection was saturated. Opening wireshark, I discovered GreenBits was downloading full blocks using max-sized getdata requests:

f9beb4d9 ... magic
676574646174610000000000 ... command: getdata
53460000 ... size: 18,003 bytes
0e1fe173 ... checksum

fdf401 ... number of inventories: 500
02000000 ... inventory type: 02/block 
f85b64534597dfea252eeecb93b13eb2fe04c207a18c51f70000000000000000 ...  header hash

Is this intentional? Does the app do all this downloading when it's on typically-expensive mobile wireless? Is it going to store the entire block chain on my (virtualized) SD card?

Version: GreenBits 1.38 downloaded from fDroid webpage
Android 4.2.2 API 17 on (Virtualized) Nexus 4

@greenaddress

This comment has been minimized.

Copy link
Contributor Author

greenaddress commented Feb 6, 2015

@harding thank you! good catch!

This wasn't intentional. See work around for now, going forward we'll change the app from constructing bloom filters from addresses as opposed to txhash (which on new wallets caused the issue since there would be none).

Looks like bitcoinj ignores empty bloom filters.

Since it wasn't intentional we were not going to store the block chain on SD cards, we intend to store recent headers only.

We do worry about big downloads on wireless, which is why the user is asked first if he wants to continue or wait for WiFi to download the headers (which should be around 27 MB if I'm not mistaken). Such prompt is shown when user is on cellular connection.

@harding

This comment has been minimized.

Copy link
Contributor

harding commented Feb 6, 2015

@greenaddress my pleasure! Thanks for getting back to me so quick and for answering my other questions. I'll let you know if I find anything else during my review.

@harding

This comment has been minimized.

Copy link
Contributor

harding commented Feb 7, 2015

@greenaddress thanks again for submitting your wallet! I really like the combination of features you offer: SPV on the P2P network, using a hardware wallet in 2-of-2, using a HW wallet and your signing oracle in a 2-of-3, with support for both Trezor and Ledger hardware and clones---these are exactly the kind of next-generation features I think we're looking for on the Bitcon.org wallet page.

However, it is my recommendation that we wait at least a month and re-review GreenBits before adding it to the site. Per our wallet requirements, "wallets [should be] publicly announced and released for at least 3 months". GreenBits was just announced earlier this week.

The purpose of this requirement is to ensure there's enough time for early adopters and other reviewers to identify potential problems with a new wallet. For most new wallets, we need at least three months to see even a modest number of public reviews---but because of of the popularity of your service, you already have a good amount of feedback for GreenBits. In addition, you've built GreenBits on established technology such as BitcoinJ and your signing oracle.

So, for those reasons, I suggest that we halve the full 3-month waiting period to just 1.5 months---roughly March 20th. If that's acceptable to you, I'll put re-reviewing GreenBits on my calendar for shortly before then.

I think a short delay is also important because myself and other reviewers have found a bunch of minor (non-security) issues with the current version that seem to indicate the app isn't quite mature yet. The patches you put into version 1.40 and 1.41 should address the issues I've encountered so far, but waiting a month will hopefully provide sufficient time to shake out most other minor-but-troubling bugs.

I also reviewed the various scores set in your commit, and everything looks good to me---but I do have one suggestion: I think to be fair to other wallet providers, we should only list one of your apps in the Android section---so would you consider dropping "os: android" from GreenAddress Cordova in the commit that adds GreenBits? (I know right now GA-C has more features than GB, but hopefully GB will be closer to feature parity next month.)

Thanks again for your submission!


Not essential to being added to Bitcoin.org, but these two comments from @luke-jr in his Reddit comments stuck out to me:

  • "Transactions do not tell you what address they were received with, or let you set labels, so there appears to be no way to tell who paid you or for what." (Emphasis his)

  • "On the receive tab, there is a button to get a new address, but it is very unclear that is its purpose. It would be nice if it automatically made a new address every time you open the tab."

    This is related to one of our possible future requirements to "Avoid address reuse by displaying a new receiving address for each transaction in the wallet UI"

I know you responded to Luke already, but I wanted to add that I also think both of those things are important.

(Thanks also for all of your replies on that Reddit thread. I know you've always been very communicative, and I appreciate that.)

@saivann

This comment has been minimized.

Copy link
Contributor

saivann commented Feb 7, 2015

On the 3 months delay and duplicate listing, the suggested solutions sound good to me!

@harding harding removed their assignment Feb 11, 2015
@harding harding added the Help Needed label Feb 27, 2015
@harding harding self-assigned this Apr 10, 2015
@crwatkins

This comment has been minimized.

Copy link
Contributor

crwatkins commented Apr 12, 2015

I have reviewed GreenBits based on the current wallet requirements criteria and my evaluation is below. The summary is that I found nothing wrong with the security or the overall design. However, because of the bugs, rough edges and instability that I noted in the review, I cannot at this time recommend it for listing as it seems unsafe for general use. I would be happy to re-review GreenBits after the original three month period has expired and issues have been corrected.

I concur with the scoring in the pull request.

GreenBits

Version 1.46

Review version 2015041201

The wallet list is based on the personal evaluation of the maintainer(s) and regular contributors of this site, according to the criterias detailed below.

These requirements are meant to be updated and strengthened over time. Innovative wallets are exciting and encouraged, so if your wallet has a good reason for not following some of the rules below, please submit it anyway and we'll consider updating the rules.

NOTE GreenBits is the new Android native version of the existing GreenAddress wallet. However it is not yet fully featured. For example, many settings are not yet available, such as enabling time locked transaction emails for 2of2 recovery. Also, “redepositing” time locked transactions which have expired requires using the older GreenAddress wallet to access the feature.

NOTE No BIP70 review/testing was done beyond the simplest testing of a request link

NOTE No social media (Reddit, etc) addressing was reviewed/tested

NOTE HW.1 was used with relative success for HD seed storage and login. There were some issues when HW.1 launched the app with a different account while the app was already running.

NOTE Trezor could not be tested. See below.

NOTE Odd fees were noticed. On a 200 bit, single input transaction, the fee was 339.86 bits. Seems high, and unusual. On a 100 bit transaction the fee was 360.14, and suspiciously claimed the remainder of the input exactly (i.e. the transaction only had one output) perhaps suggesting a (serious?) bug in the fee code?

NOTE When switching tabs between “Send” and “Receive” and “All” the account displayed may change. If the account is set to “Account 1” on Receive, it should be on “Account 1” when you switch to Send. This is fairly confusing.

Basic requirements:

  • Sufficient users and/or developers feedback can be found without concerning issues, or independent security audit(s) is available

POOR There seems to be concerning issues. I have not been able to test Trezor. I reported my issues on a Sunday to info@greenaddress.it and I received a reply on Tuesday asking me what platform I was using, which I had clearly stated in my email along with what version and the exact steps I performed. I immediately replied, repeating the requested information, and have received no response a week later. Two others have reported this same Trezor problem on Reddit and it has not been addressed.

http://www.reddit.com/r/Bitcoin/comments/2uv8iv/greenbits_the_all_new_snappy_android_bitcoin/cocviy2

https://www.reddit.com/r/greenaddress/comments/30jeql/i_get_this_every_time_when_i_try_to_log_back_in/

  • No indication that users have been harmed considerably by any issue in relation to the wallet

POOR I have sent multiple emails to the address on support page without any answer for a week. Others complain about timely responses also
http://www.reddit.com/r/Bitcoin/comments/30apm1/stay_away_from_greenaddressit/

  • No indication that security issues have been concealed, ignored, or not addressed correctly in order to prevent new or similar issues from happening in the future

PASS Responsive and willing to engage on Reddit and Bitcointalk.

  • No indication that the wallet uses unstable or unsecure libraries

PASS Uses bitcoinj

  • No indication that changes to the code are not properly tested

POOR No known indication of lack of testing other than numerous bugs noticed and reported.

  • Wallet was publicly announced and released since at least 3 months

NOTE 5/Feb/2015

http://blog.greenaddress.it/2015/02/05/greenbits-the-all-new-snappy-android-bitcoin-wallet-with-multisig-and-hardware-wallets-support/

While it hasn’t been three months yet, an exemption for consideration was offered, allowing consideration on or after March 20th. Source:

I suggest that we halve the full 3-month waiting period to just 1.5
months---roughly March 20th. -- @harding
#725 (comment)

  • No concerning bug is found when testing the wallet

FAIL Concerning bugs and instability have been noted

Example 1:

Signed up for new wallet, programmed NFC. Send screen shows “bits 500”. Attempting to perform a Send returns “internal error”. “All” screen shows “bits 0” while “Send” screen shows “bits 500.” I logged out, logged in with NFC, set a PIN and it crashed. Now I can’t launch it without a crash. Incorrect balances are very concerning.

Example 2:

Set up new account. Skipped PIN. Set up Email 2FA. Entered address, got confirmation email, entered code into GreenBits. Hit [Continue] and the app hung (Continue was grayed out; nothing happened). Used Android back button, tried again: entered address, got “Authentication Required”. Quit. Logged back in, tried again. It worked.

Example 3:

I launched the app with an NFC token, but it took me to different wallet (probably already running) with no indication that I was in a different wallet.

Example 4:

Logging in to Chrome App (using same seed as GreenBits), caused GreenBits on Android to crash. Android reports “Unfortunately, GreenBits has stopped” with options to “Report” it.

Example 5:

Android back button goes from All page to All page (over and over, tested over 20 times). If I go to Send or Receive, Back takes me back to All, but Back will never get out of the app.

  • Website supports HTTPS and 301 redirects HTTP requests

PASS For greenaddress.it

PASS Rating: A (CloudFlare based)

  • Website serving executable code or requiring authentication uses HSTS with a max-age of at least 180 days

PASS greenaddress.it HSTS max-age is 356 days

NOTE Executables are downloaded via Google Play and F-Droid

  • The identity of CEOs and/or developers is public

PASS https://greenaddress.it/en/about/

  • If private keys or encryption keys are stored online:

NOTE HD seed is stored online in this multi-sig wallet

  • Refuses weak passwords (short passwords and/or common passwords) used to secure access to any funds, or provides an aggressive account lock-out feature in response to failed login attempts along with a strict account recovery process.

PASS PIN system provides aggressive lockout (3 failures, keys are erased) enforced by server. Original HD phrase is required for recovery.

  • If user has no access over its private keys:

N/A

  • Provides 2FA authentication feature
  • Reminds the user to enable 2FA by email or in the main UI of the wallet
  • User session is not persistent, or requires authentication for spending
  • Provides account recovery feature
    • If user has exclusive access over its private keys:

NOTE User has some access to private keys (multi-sig wallet)

  • Allows backup of the wallet

PASS Wallet can be backed up using BIP39 phrase

NOTE To enable the 2of2 account timelock recovery feature which would allow the user to recover from a server failure, the user has to enable this feature from the desktop app. It cannot yet be done from GreenBits.

  • Restoring wallet from backup is working

PASS Wallet can be logged in (restored fully) with wallet BIP39 phrase

NOTE If GreenAddress server is not available, 2 of 2 multi-sig accounts can be recovered by using an offline utility using the wallet’s BIP39 phrase and time locked transactions email previously from server (after time lock expires). The main account BIP32 path is m/1/i.

NOTE Using the 2 of 2 recovery utility Gentle from online source https://greenaddress.github.io/gentle/ fails because Gentle can’t access mixed (http vs https) content at http://btc.blockr.io/api/v1/block/info/last . Firefox blocks mixed content and blockr is going to redirect to https anyway, so these references should be changed to https.

NOTE Using Gentle on Firefox from local file system caused

Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at http://btc.blockr.io/api/v1/block/info/last. This can be fixed by moving the resource to the same domain or enabling CORS.

This means that Gentle failed to restore funds because it did not know that the locktime had expired. It would not attempt to submit the transactions. I extracted the raw transaction and submitted them by hand using the core client. That worked.

PASS Timelock transaction emails can be used to recover funds

FAIL Gentle would not be able to be used as a tool to recover those funds for most users

NOTE If GreenAddress server is not available, 2 of 3 multi-sig accounts can be recovered using the wallet’s BIP39 phrase plus the “subaccount recovery mnemonic passphrase” (BIP39) and the “subaccount GreenAddress xpub” (xpub key).

NOTE There is a pull request for Electrum to be able to restore 2of3

https://github.com/greenaddress/electrum/tree/greenaddress-2of3-recovery

I could not find evidence of success by others:

http://www.reddit.com/r/greenaddress/comments/2pp2p3/mnemonic_passphrase_for_greenaddress/

however I was able to generate all three addresses for a transaction by hand (BIP32 path m/k’/1’/i where k is the sub account, and P2SH address order 1) server, 2) wallet (hot), 3) recovery(cold) (not sorted), so this should be possible to recover.

  • Source code is public and kept up to date under version control system

PASS https://github.com/greenaddress/GreenBits

  • If user has no access to some of the private keys in a multi-signature wallet:

NOTE This is a multi-signature wallet with 2of2 and 2of3 account options

  • Provides 2FA authentication feature

PASS Rich set of 2FA options: E-mail, Google Authenticator, SMS, and Phone call

  • Reminds the user to enable 2FA by email or in the main UI of the wallet

PASS Very obvious constant reminder in orange bar at bottom of screen (can be disabled in settings)

  • User session is not persistent, or requires authentication for spending

PASS 2FA is required for spending by default

  • Gives control to the user over moving their funds out of the multi-signature wallet

PASS Can send funds to any address or use recovery tools.

  • For hardware wallets:

N/A

  • Uses the push model (computer malware cannot sign a transaction without user input)
  • Protects the seed against unsigned firmware upgrades
  • Supports importing custom seeds
  • Provides source code and/or detailed specification for blackbox testing if using a closed-source Secure Element

Optional criterias (some could become requirements):

  • Received independent security audit(s)

NOTE No known independent audits performed

  • Avoid address reuse by using a new change address for each transaction

PASS Uses a different change address for each transaction. It gets change addresses from the same path as received addresses (m/1/i)

  • Avoid address reuse by displaying a new receiving address for each transaction in the wallet UI

PASS

  • Does not show "received from" Bitcoin addresses in the UI

PASS

  • Uses deterministic ECDSA nonces (RFC 6979)

PASS Software and hardware signature are deterministic. This was tested by recreating a 2of2 script by hand and signing it using custom code based on pybtctools. The result was the same signature as created by the wallet, which indicates that it is RFC 6979 compliant.

  • Provides a bug reporting policy on the website

PASS Provides contact email address and phone number on support page https://greenaddress.it/en/support/

POOR Response to email address is poor. Multiple emails went unanswered and unacknowledged. Others have had the same issue http://www.reddit.com/r/Bitcoin/comments/30apm1/stay_away_from_greenaddressit/

  • If user has no access over its private keys:

N/A

  • Full reserve audit(s)
  • Insurrance(s) against failures on their side
  • Reminds the user to enable 2FA in the main UI of the wallet
  • If user has exclusive access over its private keys:

NOTE This is a multi-sig wallet. User has some control over private keys.

  • Supports HD wallets (BIP32)

PASS BIP32 with BIP39 seed, uses m/1/i path for 2of2 sig

  • Provides users with step to print or write their wallet seed on setup

PASS Users are provided with BIP39 mnemonics in addition to QR code or saving to NFC token

  • Uses a strong KDF and key stretching for wallet storage and backups

PASS Wallet storage uses strong keying material stored on server retrieved with PIN
https://ghgreenaddress.files.wordpress.com/2014/04/greenaddressp2sh2of2hd-6.pdf

  • On desktop platform:

N/A

  • Encrypt the wallet by default
  • For hardware wallets:

N/A

  • Prevents downgrading the firmware
@harding

This comment has been minimized.

Copy link
Contributor

harding commented Apr 12, 2015

I reviewed @crwatkins's notes and agree with his conclusion that GreenBits is too unstable to add to the wallets list at this time. I will leave this issue open so @greenaddress can post here when they've fixed the issues described and we can schedule a re-review.

@crwatkins thank you very much for all of the time you put into this review!

@harding harding removed the Help Needed label Apr 12, 2015
@harding harding removed their assignment Apr 12, 2015
@saivann

This comment has been minimized.

Copy link
Contributor

saivann commented Apr 13, 2015

@crwatkins Thanks for this fantastic in-depth review! @greenaddress, there's quite an interesting number of bug reports you might be interested to read and investigate in that post.

Agreed on waiting for the full 3 months. Looking forward to seeing this app moving forward and gain more stability.

Re slow support: That tends to be common in startups to not catch up with communications. Although it's good to know and report, I suggest we don't worry too much unless it's found that security issues seem to be neglected for features. Regarding the reddit post from rfmalta, although they allegedly didn't provide responsive support, they adopted the prudent behavior of not resetting 2FA on demand at the cost of potential bad PR and responded on reddit (username BitFast), which in itself is a good indication to me.

@harding

This comment has been minimized.

Copy link
Contributor

harding commented May 29, 2015

@greenaddress

This comment has been minimized.

Copy link
Contributor Author

greenaddress commented May 29, 2015

I apologize for the problems with support, as for my reply on reddit we were trying to solve all concerns before replying here on GitHub and the private support email came at a time which was heavily under resourced and a few emails haven't been actioned/replied to. We have replied to most/all other request of support since then and I believe we have fixed some of the issues reported but I want to finish some work before asking for another review of concerns and fixes.

@harding

This comment has been minimized.

Copy link
Contributor

harding commented Sep 21, 2015

Closing in favor of #1065

@harding harding closed this Sep 21, 2015
@greenaddress greenaddress referenced this pull request Nov 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.