zcash-swift-wallet-sdk icon indicating copy to clipboard operation
zcash-swift-wallet-sdk copied to clipboard

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

Open str4d opened this issue 2 months ago • 0 comments

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).

str4d avatar Apr 30 '24 20:04 str4d