feat: trezor integration

[ethicnology]

Testing and finalizing @anipy1 contribution to be merged in develop

[claude]

Claude finished @ethicnology's task in 8m 5s —— View job

---

PR Review: Trezor integration (trezor → develop)

• Gather context (diff vs origin/develop, repo docs)
• Review Trezor feature code for correctness bugs, security issues, regressions
• Post inline review comments — not possible: this workflow's granted tools only allow updating this comment (no PR-review API access), so findings are posted here with file/line references instead
• Post summary

---

🔴 Blockers

1. Compile error — lib/features/import_wallet/import_wallet_page.dart:113-129

The Trezor menu entry was inserted inside the BitBox TabMenuVerticalButton(...) argument list. The BitBox button's constructor opened at line 113 is never closed before const Gap(16), at line 123, and the stray ], / ), at lines 128–129 leave the brackets unbalanced — Gap and the Trezor button become positional arguments to a constructor that has none. This cannot compile, so the whole app is currently broken on this branch (the "Analyze and Test" CI run should confirm). The newly added settings_cubit.dart import on line 15 is also unused, which suggests an intended edit (an environment gate?) got mangled in a merge. Fix by closing the BitBox button before adding the Trezor entry:

              TabMenuVerticalButton(
                title: Platform.isAndroid
                    ? context.loc.importWalletBitBox
                    : context.loc.importWalletBitBoxNova,
                onTap: () => context.pushNamed(
                  BitBoxRoute.importBitBox.name,
                  extra: const BitBoxRouteParams(
                    requestedDeviceType: SignerDeviceEntity.bitbox02,
                  ),
                ),
              ),              // ← close the BitBox button
              const Gap(16),
              TabMenuVerticalButton(
                title: context.loc.importWalletTrezor,
                onTap: () => context.pushNamed(TrezorRoute.importTrezor.name),
              ),
            ],

Fix this →

2. Regression — duplicated verify buttons and Lightning leak, lib/features/receive/ui/screens/receive_qr_screen.dart:65-67

The three new rows were added above the pre-existing showAddressVerification block (lines 68–74), which still renders the Ledger and BitBox buttons. Result:

• A Ledger or BitBox wallet's receive screen now shows the "Verify address" button twice.
• The new rows have no !isLightning guard, so all three buttons (including Trezor's) also appear on the Lightning receive screen, where there is no on-chain address to verify — showAddressVerification existed precisely to exclude that case.

Fix: drop the two new Ledger/BitBox lines and move the Trezor entry inside the guarded block, extending the guard:

final showAddressVerification = !isLightning && (isLedger || isBitBox || isTrezor);
...
if (showAddressVerification) ...[
  if (isLedger) const Column(children: [VerifyAddressOnLedgerButton()]),
  if (isBitBox) const Column(children: [VerifyAddressOnBitBoxButton()]),
  if (isTrezor) const Column(children: [VerifyAddressOnTrezorButton()]),
  Gap(gap),
],

Fix this →

🟠 Security

3. Signed transaction from the deeplink callback is broadcast without validation — lib/features/trezor/data/trezor_connect_datasource.dart:232-241 / send_screen.dart SignTrezorButton

The signing round-trip goes out to Trezor Suite and comes back through the bullbitcoin://trezor-callback custom-scheme deeplink. The returned serializedTx hex is handed straight to SendCubit.updateSignedBitcoinTx and later broadcast; nothing checks that it is actually the transaction the user built. Custom schemes are not exclusive: on iOS any app can register bullbitcoin:// and any app can fire that URI on both platforms, so the trust boundary here is only whatever request-matching the forked trezor_connect package does in handleCallback. I couldn't audit the fork from this sandbox (network egress is blocked), so please verify it binds callbacks to a per-request nonce/id. Independent of that, a cheap and robust defense-in-depth fix at the app layer: after signing, decode serializedTx with bdk and assert its inputs (prevout set) and outputs (scriptPubkey + amount) exactly match the unsigned PSBT before returning success. That reduces the worst case from "wrong transaction broadcast with the user's blessing" to a visible error.

4. Supply chain — pubspec.yaml:55-58

trezor_connect is pinned to a personal fork (github.com/anipy1/trezor_connect @ b649c02) that sits in the middle of the signing pipeline. The commit pin is immutable (good), and the TODO to return to cake-tech upstream is already there — but since this package parses the callback that produces broadcastable transactions, it deserves an explicit code audit before this merges, and the upstream migration should be tracked as a release blocker rather than a comment.

🟡 Worth verifying

5. Possible double-handling of callback URIs — lib/features/trezor/ui/trezor_deeplink_listener.dart:31-44

app_links ≥ 6.0 delivers the initial (cold-start) link through uriLinkStream as well, so subscribing to the stream and calling getInitialLink() means a cold-start callback gets passed to handleCallback twice. Also, trezor_operation_base_cubit.dart:17-18 refers to "the trezor_connect package's app_links callback", which suggests the package may subscribe to deeplinks internally — if it does, this listener makes every callback processed twice. Worth confirming against the fork and keeping exactly one delivery path.

6. Send-to-self outputs are marked as change — trezor_connect_datasource.dart:196-208

Any output carrying bip32Derivation is sent to Trezor as an addressPath change output. For a send-to-self (recipient address belongs to the same wallet), BDK populates bip32Derivation on that output too, so it may be treated as change and skipped in the on-device confirmation display. Amounts are still enforced, so this is a UX/verification-completeness question rather than a theft vector, but it's worth a manual test.

Notes (non-blocking)

• trezor_import_landing_screen.dart:342 hardcodes isTestnet: false — acknowledged in trezor_capabilities.dart, fine for now.
• iOS registers the whole bullbitcoin scheme (Info.plist), while Android filters on host trezor-callback. GoRouter's onEnter only blocks trezor-callback URIs, so on iOS any app can now open arbitrary bullbitcoin://<anything> URIs that will hit the router as unknown routes. Consider blocking the whole unknown-bullbitcoin-scheme space in onEnter.
• The TrezorOperationBaseCubit epoch/grace-period design is solid and well-tested (trezor_operation_base_cubit_test.dart covers the stale-completion races nicely). Same for the address-verification double check (device-side address: pin + app-side comparison) in verifyAddress.
• The derivationPath.replaceAll('h', "'") normalization is safe for valid BIP32 paths (charset is digits/m///h), no issue.

Summary

The feature architecture is clean (datasource → repository → usecase → cubit, typed errors, good test coverage of the tricky lifecycle races). However, the branch currently cannot compile due to the malformed widget tree in import_wallet_page.dart (#1), and the receive screen change is a visible regression for existing Ledger/BitBox users (#2). Both need fixing before merge; #3/#4 deserve a security pass on the callback pipeline given it produces broadcastable transactions.

(Inline review comments could not be posted: this workflow only grants comment-update and CI-read tools, no PR review API.)

[BullishNode]

Request changes — two findings

1. [P1] A previously signed transaction survives edits and can be broadcast instead of the transaction currently shown

SendCubit.updateSignedBitcoinTx stores the Trezor-signed raw transaction, but neither backClicked nor the amount-change path clears it.

Reproduction:

1. Build and sign transaction A with Trezor.
2. Navigate back and change the amount or recipient, producing transaction B.
3. Return to confirmation. The screen displays the state for B, but signedBitcoinTx still contains A.
4. onConfirmTransactionClicked sees a non-null signed transaction, skips rebuilding and signing, and broadcasts A.

This creates normal-flow wrong-payment potential. It is P1 rather than P0 because transaction A was previously reviewed and approved on the device, but the app must never broadcast it while showing the user the details of B.

Fix: invalidate signedBitcoinTx and all related finalized artifacts whenever any transaction-defining input changes, and again when rebuilding the unsigned PSBT. Add a regression test covering sign A → back/edit → review B → confirm, asserting that A cannot be broadcast.

2. [P1 security, medium confidence] The import callback uses predictable, unauthenticated request correlation

The app accepts any URI matching bullbitcoin://trezor-callback and forwards it to the connector after only checking the scheme and host (TrezorDeeplinkListener). The pinned connector generates the callback ID from the current millisecond timestamp and accepts the matching payload without authenticating its origin (connector source). During import, the returned xpub and descriptor are then used to construct the wallet (repository mapping).

A malicious local app that can observe or estimate when the request was created could race a forged callback containing its own xpub and descriptor. If successful, Bull Bitcoin would import an attacker-controlled watch-only wallet and later show receive addresses controlled by that attacker.

Confidence is medium because this requires a hostile local app plus successful timing observation/guessing, and no end-to-end exploit was demonstrated. The underlying trust-boundary weakness is nevertheless concrete and should be fixed before shipping xpub import.

Fix: use a cryptographically random, single-use state value; bind it to the expected operation and request; reject missing, mismatched, expired, or replayed state; strictly validate the response schema and account data; and prefer an app-owned verified universal/App Link where feasible.

[ethicnology]

Merge blocked upstream dependency trezor_connect (anipy1/trezor_connect@b649c02, fork of cake-tech/trezor_connect) need to be prioritized for review.

We need either cake-tech to merge @anipy1 's PR and then review their dependency or make our own fork, still a review of this new dependency is required before integrating it to the codebase.

• fix: surface callback errors via TrezorCallbackException cake-tech/trezor_connect#1
• fix: surface launchUrl failure via TrezorLaunchException cake-tech/trezor_connect#2

Also registerCallback() uses a predictable timestamp as the sole correlation token for Trezor Suite deeplink callbacks. This must be reviewed and patched before this branch merges. Finding #1 (stale signed tx) is fixed and tested in 541a5bb; finding #2 remains open and gates the merge.

cc: @i5hi @thibistaken @BullishNode