Skip to content

fix(security): remediate mobile audit findings and remove Ark - #2561

Merged
ethicnology merged 13 commits into
developfrom
audit-security-wallet
Aug 6, 2026
Merged

fix(security): remediate mobile audit findings and remove Ark#2561
ethicnology merged 13 commits into
developfrom
audit-security-wallet

Conversation

@ethicnology

@ethicnology ethicnology commented Aug 6, 2026

Copy link
Copy Markdown
Member

This PR depends on the audited bull_sdk changes merged by bull_sdk#18.

All four SDK packages are pinned to the resulting immutable main commit:

62977e2f5159458d79ec8089dc853a8edcb84458

Reading order

# Commit Scope
1 fix(wallet): surface the seed cleanup failure on wallet deletion Preserve visibility of failed best-effort seed cleanup.
2 fix(legacy_seed_view): block screen capture on the legacy seed screen Protect displayed mnemonics and passphrases.
3 chore(deps): pin the tor dep to its full commit hash Use an immutable Tor dependency pin.
4 refactor(swaps): extract the swap fee cap into a helper Preparatory refactor.
5 fix(swaps): cap automatic refund fees like claim fees Bound automatic refund fees.
6 fix(payjoin): derive the receiver fee cap from the live network rate Bound Payjoin receiver fee contribution.
7 feat(ark)!: remove the Ark feature Remove unshipped Ark code and dependency.
8 fix(send): keep sub-satoshi precision when reading a bolt11 amount Round BOLT11 msat amounts up for wallet selection.
9 chore(deps): bump bull_sdk to the audit fixes and Ark removal Initial SDK audit integration and descriptor fixture correction.
10 build(ci): hash-pin the toolchain bootstrap in the tools image Verify CI toolchain downloads by SHA-256.
11 chore(deps): pin bull_sdk to the full audit revision Pull hardened LWK/BBQR revisions, bounded u64 FFI encoders, and satoshifier validation/redaction fixes. Adapt LWK WalletBalance BigInt values.
12 chore(deps): include bare descriptor key validation Include SDK validation of the embedded xpub in bare [origin]xpub imports.
13 chore(deps): pin bull_sdk to its merged audit commit Re-pin all SDK packages to the immutable merged bull_sdk/main commit.

Security changes

Seed cleanup failure

SeedRepository.delete returns a Result rather than throwing. The old try/catch could not observe cleanup failures, silently leaving an orphaned seed in secure storage.

Deletion remains best-effort, but an Err result is now logged.

Legacy seed screen privacy

LegacySeedViewScreen displays mnemonics and passphrases but did not enable platform screenshot and app-switcher snapshot protection.

It now follows the existing secret-screen privacy pattern.

Tor dependency pinning

The Tor dependency used an abbreviated Git SHA. It now uses the full immutable 40-character commit hash.

Automatic refund fee cap

Background refund flows sign and broadcast without user confirmation. Their fee estimates are now capped at half the swap amount while retaining the relay minimum so dust refunds can still propagate.

Payjoin receiver fee cap

The receiver previously accepted an attacker-controlled Payjoin fee contribution up to a hardcoded 10,000 sat/vB.

The cap is now derived from the live fastest fee tier, multiplied by three, and constrained to 20-100 sat/vB. Fee lookup failure uses the safe lower bound.

No explicit minimum fee rate is passed to the Payjoin library: it takes the maximum of the caller and sender values, so adding one could only increase the receiver's possible fee contribution.

Ark removal

Ark was never available to ordinary users: its only entry point required both superuser and developer mode, and it had no persisted wallet state to migrate.

This removes:

  • Ark core and feature code
  • Ark setup flows and settings entry points
  • Ark payment-request and transaction-viewer variants
  • Ark service-status probing and revoke flows
  • ark-wallet-dart
  • 5,488 orphaned translation keys across 27 locales

The removed dependency included the audit's highest-risk Ark findings: disabled unilateral exit and a hardcoded private key.

BOLT11 millisatoshi handling

The mobile app parses BOLT11 independently of satoshifier. It previously truncated msats to sats, allowing a non-zero sub-satoshi invoice to appear amountless.

The displayed and wallet-selection satoshi value now rounds up. The exact Lightning amount remains in msats.

Full SDK audit revision

The merged SDK revision includes:

  • Exact BIP21 amount parsing
  • xpub Base58Check validation
  • BIP-380 descriptor checksum validation
  • Bare [origin]xpub key validation
  • Descriptor fingerprint and key-network validation
  • BitBox explicit network and xpub-type APIs
  • Hardened LWK and BBQR revisions
  • Bounded unsigned-64-bit FFI encoders
  • Error redaction and lockfile fixes

LWK now exposes held balances and absolute fees as unsigned BigInt values. The mobile wallet datasource was adapted accordingly.

CI toolchain integrity

The release tools image now verifies immutable SHA-256 hashes for Rustup, FVM, and Android command-line tools before executing or extracting them.

Verification

  • make checks: passed
  • flutter pub get: resolves all SDK packages from 62977e2
  • flutter analyze: no issues
  • Watch-only import tests: passed
  • Full root suite: 936 passing tests

SeedRepository.delete returns a Result and never throws, so the try/catch guarding the orphan seed cleanup was dead code: an Err was discarded and the warning it was meant to log never fired. Handle the Err explicitly and keep the cleanup best effort, then characterize the cleanup with the unit tests issue #2324 shipped without.
LegacySeedViewScreen renders full mnemonics and their passphrases but was the only secret screen without the PrivacyScreen mixin, so Android FLAG_SECURE and iOS snapshot shielding were never applied. It is reachable from Bitcoin settings whenever the user has legacy seeds, not a debug-only screen. Mirror the AllSeedViewScreen pattern: enable on build, disable when the route pops.
The tor (Arti) networking dep was the only git dep pinned by an abbreviated 7-character SHA while every other one uses a full 40-char hash. Short hashes are ambiguity-prone and read like a mutable ref. b92e2bc5a404367298b6016f171df29d1aae66d0 is refs/tags/0.1.0 upstream and already the resolved-ref in the lockfile, so the resolved commit is unchanged and no dependency actually moves.
The half-the-amount fee cap lived inline in _claimFees, which is why the refund paths never got it. Move it into _cappedFees next to _absoluteWithFloor so there is a single place that decides how an absolute fee is floored and capped. No behaviour change: _claimFees is the only caller and the expression is unchanged.
The background watcher signs and broadcasts refunds with no user confirmation, at whatever rate the fee API returns. Claims were capped at half the swap since the amount at stake bounds what is worth spending, but all four refund sites (cooperative and script-path, LN send and chain) used the raw floored estimate. A fee spike, or a compromised custom mempool server, could therefore burn far more than the refund is worth: the new test shows 10000 sats of fees on a 1000 sat swap before this change. Route the refund fees through _cappedFees. The relay floor still wins over the cap so a dust refund stays broadcastable.
The receiver's max effective fee rate was hardcoded to 10000 sat/vB. In payjoin 1.0.0-rc.5 the receiver's mandatory contribution is contributed weight x max(BROADCAST_MIN, sender minfeerate), debited from our change output and rejected only above contributed weight x maxEffectiveFeeRate. The sender's minfeerate is attacker-controlled, so that constant was the only bound: a sender paying the 10000 sat minimum could set minfeerate just under the cap and burn roughly 1000000 sats of the receiver's own change, with no prompt anywhere in the automatic receive path. Derive the cap from the fastest tier instead, clamped to 20-100 sat/vB, which bounds the worst case near 10000 sats and keeps it bounded even if the user-configurable mempool server misreports. A fee lookup failure falls back to the floor rather than failing the receive. Deliberately NOT passing a minFeeRate: the crate takes max(ours, the sender's), so a floor of ours could only raise the burn.
@claude

This comment was marked as outdated.

Ark never shipped: its only entry point was a settings row behind isSuperuser && isDevModeEnabled, so no ordinary user could reach it, and there is no Ark table in the database, so nothing persists to migrate. Removing it deletes core/ark, features/ark and features/ark_setup along with the Ark wallet card, the payment request and transaction viewer variants, the service status probe, the dev-mode revoke path and 5488 now-orphaned translation keys across 27 locales. It also removes the wallet's dependency on ark-wallet-dart, which carried the two most severe findings of the audit: an Ark wallet whose unilateral exit is disabled, leaving funds recoverable only with the server's cooperation, and a hardcoded private key. Analysis is clean and the 931 unit tests still pass, the same count as before.
_tryParseBolt11 truncated the invoice amount with msats ~/ 1000. BOLT11 denominates in millisatoshis and the pico-BTC multiplier makes sub-satoshi amounts expressible, so this discarded up to 999 msats per invoice and reported zero for anything below one satoshi. amountSat is not cosmetic here: SelectBestWalletUsecase compares it against wallet balances, and a zero reads as an amountless invoice rather than a small one. Round up instead, through a helper that can be unit tested. The parser itself needs the Rust bridge and cannot run in unit tests, but the rule can, and it is the rule that was wrong. Note this is the path the app actually uses: satoshifier's Bolt11Parser, fixed separately in the SDK, is not called from here.
Picks up the SDK-side audit work: the BIP21 amount, xpub base58check, BIP-380 descriptor checksum and bolt11 msat fixes in satoshifier, the required network and xpub type on the BitBox transport, the boltz rev pin, and the removal of Ark support. The Ark removal is a breaking SDK change, which is why the wallet dropped its own Ark code first. Turning on descriptor checksum verification immediately rejected a watch-only fixture here too: the same wrong checksum that the SDK carried, corrected to the value BDK accepts. NOTE FOR MERGE: this pins the SDK branch commit 7b343c5. It must be re-pointed at the commit that lands on the SDK's main before this is merged.
The image that produces the release APK bootstrapped its whole toolchain by executing remote code with no integrity check beyond TLS: rustup piped from sh.rustup.rs, FVM piped from an unversioned fvm.app/install.sh, and the Android cmdline-tools zip fetched from a _latest.zip URL. Everything downstream is built by those tools, including the Rust FFI crypto, so a compromised or swapped asset reaches the signed artifact. The rest of the repo already pins carefully, the base image by digest and the reproducibility jars by SHA256; this closes the remaining gap using the same sha256sum -c pattern. rustup now comes from its immutable archive path at a pinned rustup version, FVM from its release tarball reproducing the layout install.sh produces, and the Android zip is checked before use. The base image is pinned to linux/amd64, so single x86_64 digests are correct. Each digest was downloaded and computed here; the rustup one also matches the checksum rustup publishes beside the binary. NOT VERIFIED: the image was not built. This host is aarch64 with no x86 emulation, so the amd64 image cannot run here. The URLs, digests, archive layouts and the sha256sum -c pattern were each checked directly, but the first real build will be the first end-to-end proof.
@ethicnology
ethicnology force-pushed the audit-security-wallet branch from c876e1a to febb158 Compare August 6, 2026 11:26
@ethicnology ethicnology changed the title fix(security): audit remediations, and remove Ark fix(security): remediate mobile audit findings and remove Ark Aug 6, 2026
@ethicnology ethicnology self-assigned this Aug 6, 2026
@ethicnology
ethicnology merged commit 5c3f246 into develop Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant