Zecwallet Lite Security Updates and Review

Community Grants Applications
1

Applicant background

Zecwallet Lite is a set of desktop and mobile apps that implement the Zcash Lightclient protocol. They are light wallets that allow users to easily send and receive shielded transactions without needing to download the entire blockchain. Zecwallet Lite was originally released in early 2020, and is widely used in the Zcash ecosystem.

Zecwallet Lite uses the Zecwallet Lightclient SDK, an independent implementation of the Zecwallet Lightclient protocol that is used by the desktop and mobile Zecwallet Lite apps.

Motivation and overview

When Zecwallet launched, the Light client protocol was very new, and some features weren’t implemented yet, so Zecwallet had to fork a couple of projects to add support for the full feature set. Since then, Zecwallet has upstreamed several changes, but we need to catchup and pay down some of the technical and security debt we have accumulated over the last year. This proposal outlines the 3 biggest shortcomings and proposes to address them over the next 7 weeks.

A big reason for doing this now is to prepare for the upcoming Pollard/Halo upgrade. Removing un-needed dependencies and relying on common implementations will make sure that future Pollard work will be doable without complicated customization, which might introduce further risks.

Technical approach

This project proposes doing 3 major tasks:

1. Depend directly on ECC’s librustzcash

When Zecwallet Lite originally launched last year, we decided to support t address transactions as well in the lite client. This didn’t have support in librustzcash, so Zecwallet forked ECC’s librustzcash repository to add t-address support. Since then, we’ve been working to upstream the changes, and we’ve already submitted several PRs. Additionally, ECC has also added t-address support into librustzcash. This task is to finish the final set of changes (which are largely on the Zecwallet SDK side) to completely remove the dependency on the Zecwallet’s librustzcash fork, and depend directly on ECC’s librustzcash API.

2. LightwalletD compatibility

When Zecwallet originally launched, it forked the stock LightwalletD and implemented two sets of changes in the fork:

  • Add t-address support
  • Cache the entire Compact blockchain in memory, trading off higher resources for faster sync speed. Combined with multi-threaded syncing support on the client side, this significantly improved sync speed, which was the biggest complaint lite wallet users had.

Since then, ECC’s LightwalletD has progressed considerably, and now also has support for t-addresses. Unfortunately, this is not API-compatible with Zecwallet’s LightwalletD, and this task is to fix this by switching to the stock LightwalletD’s API.

  • Add support for stock LightwalletD’s t-address API and implementation.
  • Fix Zecwallet Lightclient SDK to use the new t-address API.

Once this is done, we’ll have two way compatibility. i.e.,

  • Zecwallet Lite apps will be able to use stock LightwalletD to sync
  • Other wallets implementing ECC’s light client SDK will be able to sync against Zecwallet’s LightwalletD

This should go a long way in reducing the dependency on Zecwallet’s LightwalletD server, and allow users to easily use any of the community-run LightwalletD servers.

3. Security Review of the Zecwallet Lite SDK

One of the major outstanding items from last year is to complete a full security audit of the Zecwallet Lite SDK. As a reminder, Zecwallet Lite SDK is an independent implementation of the Lightclient Protocol, which is used in Zecwallet Lite apps and a few other community projects. It uses librustzcash to access Zcash’s cryptographic natives. While librustzcash is maintained by the ECC and has regular security review, Zecwallet’s Lightclient SDK has never been security audited.

Zecwallet solicited 3 proposals from external companies, and the most competitive proposal is from Least Authority. You can read the detailed proposal here

  • Security audit Zecwallet Light client SDK.

  • EUR ​48,038​.00

Zecwallet will also set aside three weeks of Developer time to address any issues that are uncovered by the Security Review.

Execution risks

  • For the librustzcash change, there aren’t many unknowns, since Zecwallet has kept up with the upstream changes. The vast majority of changes are on the Zecwallet Lite SDK side, to use the APIs and ZIP implementations of the upstream librustzcash

  • For the LightwalletD compatibility, there aren’t many unknowns, since the fork already implements the t-address support. Once major risk area is performance - Zecwallet will need to maintain the faster performance even for the new API implementation, using the in-memory cache for Compact blocks.

The Security Review has many unknowns. Least Authority has done many reviews, and they have a lot of expertise on both the Rust side and on the Zcash side. There is some uncertainty around timing and availability of engineers if we are delayed beyond end-Jan.

The risks are mainly on the Zecwallet side. If there are major issues uncovered, we’ll need to move fast and implement them. For other fixes, the 3 weeks of allocated developer time to fix issues might not be sufficient.

Downsides

There are no major downsides to this project, but some things to keep in mind:

  • The security review might uncover major issues, which might need major architecture/infra changes
  • Even after the LightwalletD API compatibility fixes, the implementations of LightwalletD will continue to be different, which might cause unexpected future issues.

Evaluation plan

This project will be a success, if:

  • Zecwallet and LightwalletD are two-way compatible, and users can switch between many lightwallet servers
  • The dependency on the Zecwallet fork of librustzcash is completely eliminated
  • Zecwallet addresses and fixes major items from the security review.

Tasks and schedule

Rough schedule, tasks and timelines:

  • Remove librustzcash fork

    • 80 hrs (2 weeks) of work
    • Publish new releases of the Lite apps that don’t have this dependency
    • Expected 31 Jan 2021

  • LightwalletD API compatibility

    • 80 hrs (2 weeks of work)
    • Implement stock LightwalletD API
    • Update Zecwallet apps to use new API and publish releases
    • Expected 15 Feb 2021

  • Security Review

    • Will be done in parallel by Least Authority
    • 120 hrs (3 weeks) of developer time to fix any issues uncovered
    • Security Review: Last week of Jan, 2 weeks
    • Fixes: Expected 6 Mar 2021

Budget and justification

Total: USD 112,500

Zecwallet work

  • 7 weeks total (@ USD 187.5/hr)
  • USD 52,500

Security Review

  • USD 60,000 approx
8 Likes
2

I am expressing my support for this grant proposal.

Zecwallet lite is currently the only GUI desktop wallet with shielded support. Personally, I only use Zecwallet lite for social transactions (zecpages, zbay, dm, etc.) but I hope that with the security review I can be more comfortable in using Zecwallet for commercial transactions too.

Some feature requests (which just come to my mind and I understand not part of this proposal):

  • Hardware wallet support (i.e. Metamask+Ledger style)
  • Better viewing key + payment disclosure UX
  • Complete revamp of contacts (saved addresses) UX

I will be glad contributing to those features in the future when I got the chance to.

Cheers!

6 Likes
3

Would it be helpful to move your performance improvements in zecwallet-light-cli and lightwalletd with the ECC’s counterparts? What would be the advantages and disadvantages of this?

Thanks for all your work for the ecosystem, Aditya. As one of the community projects using the Zecwallet SDK and your lightwalletd infrastructure I’m really grateful for your work!

Also, to be super explicit about a potential conflict as a ZOMG member—which I don’t think is a conflict, but could be—my project Zbay is a user of Zecwallet SDK, so I have an interest in it getting improved, security audited, etc. I think this mostly isn’t a conflict because many people in the ecosystem share this interest, and because I explicitly ran on a platform of focusing on user facing apps and representing an app developer’s perspective (see my announcement :slight_smile: ). But all of this is uncharted territory so I wanted to note it here.

4 Likes
4

Thank you, @holmesworcester!

I am definitely open to upstreaming some of the performance work where possible, and we’ve even discussed it with the ECC’s wallet team. Something on the roadmap for 2021!

Although I should note that the improved performance comes at a cost of increased server resources (and some other things like increased startup time), so it might not be as simple as just applying the increased perf to upstream, but definitely something I’m interesting in achieving in 2021.

2 Likes
5

May we see the other proposals please. (and does the ECC one infer that they will fix the bugs?)

I am interested in
Company Name:
Estimated time per milestone:
Cost per milestone:
Man hours per milestone:
Accreditations of companies.
Insurance level carried by accreditations:

I am curious as to how you shortlisted 3 companies. were more considered? who? PM if this is sensitive info.

6

Hi @adityapk00

The @ZcashGrants discussed this proposal today we have opted to put this proposal on hold, pending the questions below:

  1. The review will not cover the electron applications, the mobile applications or desktop applications, and as such will focus exclusively on the rust SDK code. This is a relatively small code base, and also seems to ignore a large portion of the risk profile of the wallet, so we were curious if you had inquired as to the cost to review those additional portions?

  2. We are not convinced that maintaining (and securing) a second distinct lite SDK is an efficient use of funds, especially given the current (and proposed future) overlap in the underlying dependencies, and the incomplete nature of the lite client protocol. Looking into the future, do you foresee a time when we could merge these 2 SDKs into one? And if so, do you still think it makes sense to invest significant resources now into this SDK instead of e.g. conducting a security review of electron or mobile applications and/or integrating the 2 SDKs?

Thank you for your proposal and we look forward to discussing this with you further.

3 Likes
7

This is, of course, disappointing, but I accept the ZOMG’s decision.

In my defense, when Zecwallet Lite was built, the ECC’s SDK did not exist yet, so Zecwallet had to build it. Replacing it with the ECC’s SDK now would be too much work. Zecwallet Lite’s SDK also has additional features that the ECC’s SDK doesn’t support (Desktop support, fast sync, t-address incoming support)

Of course, without the Zecwallet Lite SDK, Zecwallet also cannot exist, so this decision also makes Zecwallet’s other proposal moot.

Thank you for your time and consideration.

2 Likes
8

3 posts were merged into an existing topic: Principles for deprecating t2t transactions

9

@aditya, I want to make sure you fully understand the direction these questions are pointing in:

We get why it made sense and may continue to make sense in the near term, but we’re asking if you think it makes sense long term.

It’s really important for ZOMG to make sure that the community’s funds are used in the most effective way they can be, so we have to ask whatever questions come up—to understand what we’re funding and why—before we sign off. I’m sure you get this, but I just wanted to give you a sense of where we’re coming from on this.

Like I said in the other post, you should know that we’re eager to fund Zecwallet long term!

It’s just that we have a process in place and are trying to apply it evenly to everyone, even established projects in the ecosystem.

(Though we wanted to offer you fast-turnaround short-term funding, while we go through this process, since that was an easy decision to make quickly.)

1 Like
12

Wait,

The ECC should build on ady’s work. We are not talking about maintaining forever. I am greatly disappointed in this.

I was going post this in the other thread. a memeber of MiSt works at NCC - I will write your testplans, security testplans and unit tests. if they were needed.

game. over. man.

Lets all in on long term strategy for short term redundancy loss. No. This personally does not make sense to me. Please would you explain how having less is more.

So, we have an independent wallet dev the ZOMG would like to be involved with the project long term and we are favouring the ECC over that?

This dev has now withdrawn both proposals. so now what?

I dont understand this. Please explain “fast-turnaround short-term funding,” and where it is defined.

13

Last ZOMG meeting we agreed that we wanted some clarification on some parts of the two proposals that @adityapk00 had submitted. And we also did not want to have Aditya waiting for funds until the next scheduled ZOMG meeting two weeks from now due to the importance of ZecWallet in the ecosystem.

So the idea was to get out some immediate funding (as in 2 months of ZEC available immediately) and that would give more time to get clarification on the other points @sarahjamielewis raised above.

2 months is not intended to be a cap or a blocker, it was just to buy ZOMG more time to build a robust long-term plan to fund ZecWallet and the security audits it needs.

Apologies @adityapk00 if that was not made clear in the post’s above.

2 Likes
14

Thank you for reminding that of this! I hope more developers (with funding from ZOMG and ECC) take advantage of this. Security testplans and audits are great!

1 Like
15

I like that idea!

1 Like
16

Aditya has informed ZOMG of his decision to withdraw this proposal at this time.

17

now everyone is sad. @adityapk00 would you copy & paste your proposal (with whatever modifications you think makes sense) for ZF grant :slight_smile:

Who expected everything to go smoothly in this world, this will make our community stronger & better

18

In my opinion, the first two tasks (“Depend directly on ECC’s librustzcash” and “LightwalletD compatibility”) are very clearly worthy of doing and funding. They are about reducing duplicated effort, resolving incompatibilities between forked codebases, and improving interoperability between two already-deployed, in-use software products. If the choice is between (a) keeping the incompatible forks, (b) killing one of the products, or (c​) harmonizing the codebases, then (c​) is obviously the best, and it’s what @adityapk00 is proposing.

Regarding “Security Review of the Zecwallet Lite SDK”, there’s much to say about the desired scope of full security reviews and improvements, but starting anywhere is a good start. That said, I’d love to see a long-term plan drawn by @adityapk00 about future planned security reviews and improvements. Both for perspective, and to see where the currently proposed work fits in.

I’m a bit fuzzy on where ECC’s code ends and Aditya’s SDK starts (after the first two harmonization tasks), and thus on the extent of overlap between the two SDKs. Aditya, can you clarify this? But regardless of the precise split, I see nothing wrong with having multiple high-level SDKs that are compatible in their communication protocols. It’s perfectly fine for different app developers to have different needs or tastes, and a plurality of (interoperable) SDKs is healthy for the ecosystem as well as for exercising and stress-testing the underlying libraries and protocols.

Well-thought-out redundancy may be a luxury we’d forego if we were cash-strapped and barely able to get one SDK going; but our situation is the opposite: we need to encourage a vibrant developer ecosystem and multiple independent implementations, and the Dev Fund lets us afford this. As @Matthewdgreen put it: ZOMG Spending: go, go, go!

6 Likes
19

This is not how mission critical infrastructure works. - why not separate out the overlap and have two sdks?

20

Thanks to a Herculean effort by @nuttycom to get this PR completed and the Core team to get it merged a week ago. It should be a lot easier to continue collaborating with Aditya (who has been fantastic). As @mistfpga phrased it, we have “separated out the overlap” such that the complex domain logic can be decoupled from the storage layer as described in detail here. Put simply, we want the hardest bits in one collaborative place, with many eyeballs, broken down into focused building blocks that are easy to audit. This provides a rock-solid foundation.

From there, it actually makes a lot of sense to have multiple SDKs tailored to their specific use-cases. At least one each for native iOS, native Android, React Native, JavaScript (via WASM), pure Rust, etc. These libraries would leverage the complex logic in the foundation such as how to correctly execute a fly-client proof or properly handle a reorg.

An initial sticking point was that the iOS/Android ECC wallets use SQL as a storage layer because it’s ubiquitous and works well with native mobile UI components. Whereas Zecwallet uses an in-memory approach because it is faster. Previously, this made it challenging to share code effectively but, going forward, it will be a lot easier, thanks to the changes linked above (and lots of hard work by Aditya to keep up with Network Upgrades).

On the Engineering side, this has been a collaborative effort, long in the making.

10 Likes
21

@gmale — are you familiar with what this looks like on the lightwalletd side?

Given that Aditya has made performance improvements, once he makes his lightwalletd compatible with both SDKs, is there any reason for those changes not to be merged into the official ECC lightwalletd, and then become part of the infrastructure that we’ve just decided to fund in the lightwalletd-specific grant?

For example, there’s a lot of work coming up to address network layer privacy and the memo fetching kills Zcash privacy for almost all Zcash users problem. It would be kind of bad if that work gets done and most of the ecosystem is still using the old, unsafe system, right?

I’m not as deep into this as you or Aditya so apologies if I’m missing something, but I want to make sure I have a clear mental model of these decisions so I can feel confident that I’m making the best contribution I can in my role on the ZOMG.

(Also, I’m aware of instances in the past where Zecwallet can move faster by being able to quickly tweak both the lightwalletd and zecwallet-light-cli side of things, so I get that this needs to be balanced too.)

I’m also wondering, more generally, why doesn’t ECC consolidate its work around the Zecwallet SDK?

I’d also be happy to do a quick call about this to get up to speed!

1 Like
22

This is a complex and nuanced topic. A quick call would certainly be better especially if @adityapk00 was on it (maybe a light client sync would make sense, someday soon).

However, I’ll try to at least outline my answers from a high level:

It would be kind of bad if that work gets done and most of the ecosystem is still using the old, unsafe system, right?

The bulk of my technical skills are in Android. I have accomplished noteworthy things on mobile but do not ask me to write your iOS app. It will be terrible. I’d preemptively give it a 1-star review!

Similarly, days ago, Sean made a 1-line change in Rust and improved scanning in the ECC libraries by about 30%. Get him to write your Rust code, especially if it involves math. Also, all wallet makers should get that improvement without needing to understand cryptography.

In an ideal world, we want all the Zcash code to be the best it can be. To achieve that, we need excellent foundational modules that we can plug together and build upon. I’d rather @str4d write the fly-client proof system than our iOS guy–although I’m certain @pacu could get that to work because he’s brilliant (and I bet he could get it done by Friday :laughing: )

In your example scenario, as the memo feature improves through thoughtful design and code-review, in an ideal world, lightwalletd would be modular enough to make use of it. Then as further improvements ship, the entire ecosystem could get those benefits simply by updating their dependencies on the relevant modules. If things are properly encapsulated inside small, focused modules, the downstream code changes would be negligible for each project. This becomes critically important in the area of security fixes and audits.

Put simply, a modular approach makes development and security easier yet our funding models sometimes cause us to build things like this.