Electric-Coin-Company / zcash-swift-wallet-sdk

iOS light client Framework proof-of-concept
MIT License
37 stars 34 forks source link

`Synchronizer.proposeShielding` crashes when there are insufficient funds to shield #1420

Closed str4d closed 3 days ago

str4d commented 2 weeks ago

In #1382 (specifically in 84ac6252feb88b9dbf9865651df3bc478225f5a1) we adjusted Synchronizer.proposeShielding to return null when there are either no funds to shield, or they do not meet the shielding threshold (equivalent to the Rust type Option<Proposal>). However this only altered the API, as the FFI still always created a proposal; the backend changes need to be bundled together separately into binary releases of the -ffi backend.

When the -ffi backend was updated to enable zcashlc_propose_shielding to return an optional proposal, the FFI representation was not hooked up to ZcashRustBackend.proposeShielding at all.

The problem is in this code: https://github.com/Electric-Coin-Company/zcash-swift-wallet-sdk/blob/31a584801302c04f8c644f652d44d25406007f5e/Sources/ZcashLightClientKit/Rust/ZcashRustBackend.swift#L735-L741 https://github.com/Electric-Coin-Company/zcash-swift-wallet-sdk/blob/31a584801302c04f8c644f652d44d25406007f5e/Sources/ZcashLightClientKit/Rust/ZcashRustBackend.swift#L760-L764

This attempts to construct a Data from a null pointer, when it should instead check for nullability and then either return null, or construct the Data from the known-non-null pointer.

The bug has been observed very infrequently, most likely because wallets generally only try to shield when there are any funds at all, and in that case the funds are most likely non-trivial in value (and thus above the shielding threshold).