Skip to content

Commit 225e086

Browse files
committed
fix(transactions): verify server data before labels, binding and export
Addresses the TXUI findings from the security audit: - #2624: exchange labels are written only from an explicit order-completion event through the new TransactionsFacade, wired to the buy and sell completion paths — never from history reads; only completed buy/sell orders are labeled - #2625: neutralize CSV cells starting with =, +, -, @, tab or CR so server-provided swap strings cannot inject spreadsheet formulas - #2663: validate exchange-order binding against on-chain address, amount and direction instead of the server-provided tx id alone - #2664: validate swap binding against the wallet and amount, and deduplicate by verified on-chain identity instead of server ids - #2665: map aggregation failures to a sealed TransactionFailure with a generic localized message instead of rendering raw exception text Regression tests: test/security_audit/issue_2624_test.dart, issue_2625_test.dart, issue_2663_test.dart, issue_2664_test.dart, issue_2665_test.dart
1 parent 47fcfab commit 225e086

27 files changed

Lines changed: 829 additions & 55 deletions

FEATURES.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ graph TB
7474
BUY --> EXCHANGE
7575
BUY --> RECEIVE
7676
BUY --> BULL_PAYJOIN
77+
BUY --> TX_HISTORY
7778
COINS --> UTXO_MGMT
7879
COINS --> LABELS
7980
COINS --> WALLETS
@@ -94,6 +95,7 @@ graph TB
9495
SECRETS --> CORE
9596
SELL --> EXCHANGE
9697
SELL --> BULL_PAYJOIN
98+
SELL --> TX_HISTORY
9799
SEND --> CONSOLIDATION
98100
SEND --> FEES
99101
SEND --> NETWORK

lib/features/buy/buy_locator.dart

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ import 'package:bb_mobile/core/wallet/domain/usecases/get_receive_address_usecas
88
import 'package:bb_mobile/core/wallet/domain/usecases/get_wallets_usecase.dart';
99
import 'package:bb_mobile/features/buy/domain/accelerate_buy_order_usecase.dart';
1010
import 'package:bb_mobile/features/buy/domain/cancel_abandoned_buy_payjoin_usecase.dart';
11+
import 'package:bb_mobile/features/buy/domain/label_completed_buy_order_usecase.dart';
12+
import 'package:bb_mobile/features/transactions/transactions_facade.dart';
1113
import 'package:bb_mobile/features/buy/domain/confirm_buy_order_usecase.dart';
1214
import 'package:bb_mobile/features/buy/domain/create_buy_order_usecase.dart';
1315
import 'package:bb_mobile/features/buy/domain/get_buy_payjoin_enabled_usecase.dart';
@@ -40,6 +42,11 @@ class BuyLocator {
4042
locator.registerFactory<GetBuyPayjoinEnabledUsecase>(
4143
() => GetBuyPayjoinEnabledUsecase(locator<PayjoinPolicyAccess>()),
4244
);
45+
locator.registerFactory<LabelCompletedBuyOrderUsecase>(
46+
() => LabelCompletedBuyOrderUsecase(
47+
transactionsFacade: locator<TransactionsFacade>(),
48+
),
49+
);
4350
registerBlocs(locator);
4451
}
4552

@@ -60,6 +67,7 @@ class BuyLocator {
6067
cancelAbandonedBuyPayjoinUsecase:
6168
locator<CancelAbandonedBuyPayjoinUsecase>(),
6269
getBuyPayjoinEnabledUsecase: locator<GetBuyPayjoinEnabledUsecase>(),
70+
labelCompletedBuyOrderUsecase: locator<LabelCompletedBuyOrderUsecase>(),
6371
),
6472
);
6573
}
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
import 'package:bb_mobile/core/exchange/domain/entity/order.dart';
2+
import 'package:bb_mobile/features/transactions/transactions_facade.dart';
3+
4+
/// Writes the privileged exchange-buy labels when a buy order explicitly
5+
/// completes (issue #2624: labels must never be written from history reads).
6+
class LabelCompletedBuyOrderUsecase {
7+
final TransactionsFacade _transactionsFacade;
8+
9+
LabelCompletedBuyOrderUsecase({required this._transactionsFacade});
10+
11+
Future<void> execute({required Order order}) async {
12+
if (order.orderStatus != OrderStatus.completed) return;
13+
await _transactionsFacade.labelCompletedExchangeOrders([order]);
14+
}
15+
}

lib/features/buy/presentation/buy_bloc.dart

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import 'package:bb_mobile/core/settings/domain/get_settings_usecase.dart';
1111
import 'package:bb_mobile/core/settings/domain/settings_entity.dart';
1212
import 'package:bb_mobile/core/utils/amount_conversions.dart';
1313
import 'package:bb_mobile/core/utils/logger.dart';
14+
import 'package:bb_mobile/features/buy/domain/label_completed_buy_order_usecase.dart';
1415
import 'package:bb_mobile/core/wallet/domain/entities/wallet.dart';
1516
import 'package:bb_mobile/core/wallet/domain/usecases/get_receive_address_usecase.dart';
1617
import 'package:bb_mobile/core/wallet/domain/usecases/get_wallets_usecase.dart';
@@ -41,6 +42,7 @@ class BuyBloc extends Bloc<BuyEvent, BuyState> {
4142
required this._getSettingsUsecase,
4243
required this._cancelAbandonedBuyPayjoinUsecase,
4344
required this._getBuyPayjoinEnabledUsecase,
45+
required this._labelCompletedBuyOrderUsecase,
4446
}) : super(const BuyState()) {
4547
on<_BuyStarted>(_onStarted);
4648
on<_BuyAmountInputChanged>(_onAmountInputChanged);
@@ -68,6 +70,7 @@ class BuyBloc extends Bloc<BuyEvent, BuyState> {
6870
final GetSettingsUsecase _getSettingsUsecase;
6971
final CancelAbandonedBuyPayjoinUsecase _cancelAbandonedBuyPayjoinUsecase;
7072
final GetBuyPayjoinEnabledUsecase _getBuyPayjoinEnabledUsecase;
73+
final LabelCompletedBuyOrderUsecase _labelCompletedBuyOrderUsecase;
7174

7275
Future<void> _onStarted(_BuyStarted event, Emitter<BuyState> emit) async {
7376
try {
@@ -302,6 +305,10 @@ class BuyBloc extends Bloc<BuyEvent, BuyState> {
302305

303306
if (order.isExpired()) await _cancelAbandonedPayjoin(order);
304307

308+
// The explicit order-completion event is the only legitimate writer
309+
// of privileged exchange labels (issue #2624).
310+
await _labelCompletedBuyOrderUsecase.execute(order: order);
311+
305312
emit(state.copyWith(buyOrder: order));
306313
} catch (e) {
307314
log.severe(error: e, trace: StackTrace.current);
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
import 'package:bb_mobile/core/exchange/domain/entity/order.dart';
2+
import 'package:bb_mobile/features/transactions/transactions_facade.dart';
3+
4+
/// Writes the privileged exchange-sell labels when a sell order explicitly
5+
/// completes (issue #2624: labels must never be written from history reads).
6+
class LabelCompletedSellOrderUsecase {
7+
final TransactionsFacade _transactionsFacade;
8+
9+
LabelCompletedSellOrderUsecase({required this._transactionsFacade});
10+
11+
Future<void> execute({required Order order}) async {
12+
if (order.orderStatus != OrderStatus.completed) return;
13+
await _transactionsFacade.labelCompletedExchangeOrders([order]);
14+
}
15+
}

lib/features/sell/presentation/bloc/sell_bloc.dart

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import 'package:bb_mobile/core/wallet/domain/entities/wallet_utxo.dart';
2323
import 'package:bb_mobile/core/wallet/domain/usecases/get_address_at_index_usecase.dart';
2424
import 'package:bb_mobile/core/wallet/domain/usecases/get_wallet_utxos_usecase.dart';
2525
import 'package:bb_mobile/features/labels/labels_facade.dart';
26+
import 'package:bb_mobile/features/sell/domain/label_completed_sell_order_usecase.dart';
2627
import 'package:bb_mobile/features/sell/domain/create_sell_order_usecase.dart';
2728
import 'package:bb_mobile/features/sell/domain/refresh_sell_order_usecase.dart';
2829
import 'package:bb_mobile/features/sell/domain/get_payjoin_usecase.dart';
@@ -72,6 +73,7 @@ class SellBloc extends Bloc<SellEvent, SellState>
7273
required this._getWalletUtxosUsecase,
7374
required this._getOrderUsecase,
7475
required this._labelsFacade,
76+
required this._labelCompletedSellOrderUsecase,
7577
required this._previewBitcoinFeeUsecase,
7678
required this._previewBitcoinFeePresetsUsecase,
7779
}) : super(const SellState.initial()) {
@@ -121,6 +123,7 @@ class SellBloc extends Bloc<SellEvent, SellState>
121123
final GetWalletUtxosUsecase _getWalletUtxosUsecase;
122124
final GetOrderUsecase _getOrderUsecase;
123125
final LabelsFacade _labelsFacade;
126+
final LabelCompletedSellOrderUsecase _labelCompletedSellOrderUsecase;
124127
final PreviewBitcoinFeeUsecase _previewBitcoinFeeUsecase;
125128
final PreviewBitcoinFeePresetsUsecase _previewBitcoinFeePresetsUsecase;
126129
Timer? _pollingTimer;
@@ -923,6 +926,9 @@ class SellBloc extends Bloc<SellEvent, SellState>
923926
return;
924927
}
925928
await _labelPayjoinSellTransaction(latestOrder, null);
929+
// The explicit order-completion event is the only legitimate writer
930+
// of privileged exchange labels (issue #2624).
931+
await _labelCompletedSellOrderUsecase.execute(order: latestOrder);
926932
if (!latestOrder.payjoinOutcome.isOngoing) _stopPolling();
927933
emit(sellSuccessState.copyWith(sellOrder: latestOrder));
928934
} catch (e) {

lib/features/sell/sell_locator.dart

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ import 'package:bb_mobile/core/settings/domain/get_settings_usecase.dart';
1010
import 'package:bb_mobile/core/wallet/domain/usecases/get_address_at_index_usecase.dart';
1111
import 'package:bb_mobile/core/wallet/domain/usecases/get_wallet_utxos_usecase.dart';
1212
import 'package:bb_mobile/features/labels/labels_facade.dart';
13+
import 'package:bb_mobile/features/sell/domain/label_completed_sell_order_usecase.dart';
14+
import 'package:bb_mobile/features/transactions/transactions_facade.dart';
1315
import 'package:bb_mobile/features/sell/domain/create_sell_order_usecase.dart';
1416
import 'package:bb_mobile/features/sell/domain/get_payjoin_usecase.dart';
1517
import 'package:bb_mobile/features/sell/domain/refresh_sell_order_usecase.dart';
@@ -68,6 +70,12 @@ class SellLocator {
6870
),
6971
);
7072

73+
locator.registerFactory<LabelCompletedSellOrderUsecase>(
74+
() => LabelCompletedSellOrderUsecase(
75+
transactionsFacade: locator<TransactionsFacade>(),
76+
),
77+
);
78+
7179
locator.registerFactory<GetAddressAtIndexUsecase>(
7280
() => GetAddressAtIndexUsecase(walletAddressRepository: locator()),
7381
);
@@ -102,6 +110,8 @@ class SellLocator {
102110
getWalletUtxosUsecase: locator<GetWalletUtxosUsecase>(),
103111
getOrderUsecase: locator<GetOrderUsecase>(),
104112
labelsFacade: locator<LabelsFacade>(),
113+
labelCompletedSellOrderUsecase:
114+
locator<LabelCompletedSellOrderUsecase>(),
105115
previewBitcoinFeeUsecase: locator<PreviewBitcoinFeeUsecase>(),
106116
previewBitcoinFeePresetsUsecase:
107117
locator<PreviewBitcoinFeePresetsUsecase>(),

lib/features/transactions/adapters/csv_transaction_export_formatter.dart

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -120,11 +120,19 @@ class CsvTransactionExportFormatter implements TransactionExportFormatter {
120120
swap?.isLnSendSwap == true || swap?.isLnReceiveSwap == true;
121121
final isChainSwap = swap?.isChainSwap == true;
122122
final amountSat = tx.amountSat;
123+
// Prefer the fee the wallet observed on-chain over the one the swap
124+
// provider reports: an export is accounting data and must not carry a
125+
// server-controlled figure when the real one is known locally. The
126+
// provider value stays as the fallback for a leg whose miner fee the
127+
// wallet never saw (0 = unknown here, a broadcast tx always pays a fee).
128+
final onChainFeeSat = wt?.feeSat ?? 0;
123129
final feeSat = isChainSwap
124130
? (wt?.isOutgoing == true
125-
? (swap?.fees?.lockupFee ?? wt?.feeSat ?? 0)
131+
? (onChainFeeSat > 0
132+
? onChainFeeSat
133+
: (swap?.fees?.lockupFee ?? 0))
126134
: (swap?.fees?.claimFee ?? 0))
127-
: (tx.isIncoming ? 0 : (wt?.feeSat ?? 0));
135+
: (tx.isIncoming ? 0 : onChainFeeSat);
128136

129137
final type = _resolveType(tx, swap, payjoin);
130138
final direction = _resolveDirection(tx, wt);
@@ -268,6 +276,11 @@ class CsvTransactionExportFormatter implements TransactionExportFormatter {
268276
String _btc(int sats) => (sats / 100000000).toStringAsFixed(8);
269277

270278
String _escape(String value) {
279+
// Prefix formula-like cells so spreadsheet applications treat them as
280+
// text. This applies even when the cell also needs CSV quoting.
281+
if (value.isNotEmpty && '=+-@\t\r'.contains(value[0])) {
282+
value = "'$value";
283+
}
271284
if (value.contains(',') ||
272285
value.contains('"') ||
273286
value.contains('\n') ||

lib/features/transactions/application/usecases/get_transactions_usecase.dart

Lines changed: 83 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,12 @@ import 'package:bb_mobile/core/exchange/domain/repositories/exchange_order_repos
33
import 'package:bb_mobile/core/settings/domain/repositories/settings_repository.dart';
44
import 'package:bb_mobile/core/swaps/domain/entity/swap.dart';
55
import 'package:bb_mobile/core/swaps/domain/repositories/swap_history_repository.dart';
6+
import 'package:bb_mobile/core/wallet/domain/entities/wallet_transaction.dart';
67
import 'package:bb_mobile/core/wallet/domain/repositories/wallet_transaction_repository.dart';
8+
import 'package:bb_mobile/core/utils/amount_conversions.dart';
79
import 'package:bb_mobile/core/utils/logger.dart';
8-
import 'package:bb_mobile/features/transactions/application/usecases/label_exchange_orders_usecase.dart';
910
import 'package:bb_mobile/features/transactions/domain/entities/transaction.dart';
11+
import 'package:bb_mobile/features/transactions/domain/transaction_failure.dart';
1012
import 'package:bull_payjoin/bull_payjoin.dart';
1113
import 'package:primitives/primitives.dart';
1214

@@ -17,7 +19,6 @@ class GetTransactionsUsecase {
1719
final PayjoinSessions _payjoinSessions;
1820
final ExchangeOrderRepository _mainnetExchangeOrderRepository;
1921
final ExchangeOrderRepository _testnetExchangeOrderRepository;
20-
final LabelExchangeOrdersUsecase _labelExchangeOrdersUsecase;
2122

2223
GetTransactionsUsecase({
2324
required this._settingsRepository,
@@ -26,7 +27,6 @@ class GetTransactionsUsecase {
2627
required this._payjoinSessions,
2728
required this._mainnetExchangeOrderRepository,
2829
required this._testnetExchangeOrderRepository,
29-
required this._labelExchangeOrdersUsecase,
3030
});
3131

3232
Future<List<Transaction>> execute({
@@ -41,7 +41,6 @@ class GetTransactionsUsecase {
4141
: _mainnetExchangeOrderRepository;
4242

4343
final orders = await orderRepository.getOrders();
44-
await _labelExchangeOrdersUsecase.execute(orders: orders);
4544

4645
// Labels must exist before wallet transactions are hydrated.
4746
final (walletTransactions, payjoinResult, swaps) = await (
@@ -76,12 +75,16 @@ class GetTransactionsUsecase {
7675
final broadcastedTransactions = walletTransactions.map((wt) {
7776
Swap? swap;
7877
try {
79-
swap = swaps.firstWhere(
80-
(s) =>
78+
swap = swaps.firstWhere((s) {
79+
final claimedLeg =
8180
(wt.isOutgoing && s.sendTxId == wt.txId) ||
8281
(wt.isIncoming &&
83-
(s.receiveTxId == wt.txId || s.refundTxId == wt.txId)),
84-
);
82+
(s.receiveTxId == wt.txId || s.refundTxId == wt.txId));
83+
return claimedLeg &&
84+
s.walletId == wt.walletId &&
85+
(s.amountSat == 0 || wt.amountSat <= s.amountSat) &&
86+
_transactionPaysSwapAddress(wt, s);
87+
});
8588
} catch (_) {
8689
// If no swap is found, it means the transaction is not a swap
8790
swap = null;
@@ -107,7 +110,18 @@ class GetTransactionsUsecase {
107110

108111
Order? order;
109112
try {
110-
order = orders.firstWhere((o) => o.transactionId == wt.txId);
113+
order = orders.firstWhere((o) {
114+
if (o.transactionId != wt.txId) return false;
115+
if (o.toAddress != null && o.toAddress != wt.toAddress) {
116+
return false;
117+
}
118+
final expectedDirection = o is BuyOrder
119+
? wt.isIncoming
120+
: o is SellOrder
121+
? wt.isOutgoing
122+
: true;
123+
return expectedDirection && _transactionCoversOrderAmount(wt, o);
124+
});
111125
// Remove the order from the list of orders to avoid duplication
112126
// since it's already included in the broadcasted transaction
113127
orders.remove(order);
@@ -196,7 +210,66 @@ class GetTransactionsUsecase {
196210
error: e,
197211
trace: stackTrace,
198212
);
199-
throw Exception('Failed to fetch transactions: $e');
213+
throw TransactionAggregationFailure(e.toString());
214+
}
215+
}
216+
217+
/// The exchange tells us which address a completed order paid. Before the
218+
/// order is attached to one of the user's transactions, that transaction
219+
/// must actually have moved at least what the order claims — otherwise a
220+
/// wrong or hostile server figure gets attributed to unrelated funds.
221+
///
222+
/// `>=` rather than `==` on purpose: payouts are batched, so one transaction
223+
/// can legitimately carry several orders and more sats than a single one.
224+
bool _transactionCoversOrderAmount(WalletTransaction wt, Order order) {
225+
final double declared;
226+
switch (order) {
227+
case BuyOrder(:final payoutAmount, :final payoutCurrency):
228+
if (payoutCurrency != 'BTC') return true;
229+
declared = payoutAmount;
230+
case SellOrder(:final payinAmount, :final payinCurrency):
231+
if (payinCurrency != 'BTC') return true;
232+
declared = payinAmount;
233+
default:
234+
return true;
200235
}
236+
final declaredSat = ConvertAmount.btcToSats(declared);
237+
if (declaredSat <= 0) return true;
238+
return wt.amountSat >= declaredSat;
239+
}
240+
241+
/// A swap leg is only this transaction's if the transaction actually pays
242+
/// the address the swap is built around. Matching on the server-reported
243+
/// txid alone lets a wrong id graft a swap onto an unrelated payment.
244+
bool _transactionPaysSwapAddress(WalletTransaction wt, Swap swap) {
245+
// Liquid outputs expose the unconfidential address while a swap address is
246+
// confidential, so the two are not comparable here; the txid + wallet +
247+
// amount checks remain. TODO(swaps): unblind before comparing.
248+
if (!wt.isBitcoin) return true;
249+
250+
final expected = <String>{
251+
if (wt.isOutgoing)
252+
...switch (swap) {
253+
LnSendSwap(:final paymentAddress) => [paymentAddress],
254+
ChainSwap(:final paymentAddress) => [paymentAddress],
255+
_ => <String>[],
256+
},
257+
if (wt.isIncoming)
258+
...switch (swap) {
259+
LnReceiveSwap(:final receiveAddress) => [?receiveAddress],
260+
ChainSwap(:final receiveAddress, :final refundAddress) => [
261+
?receiveAddress,
262+
?refundAddress,
263+
],
264+
LnSendSwap(:final refundAddress) => [?refundAddress],
265+
},
266+
};
267+
// Nothing to compare against (a recovered swap carries no address): fall
268+
// back to the other checks rather than dropping a legitimate leg.
269+
if (expected.isEmpty) return true;
270+
271+
return wt.outputs.any(
272+
(output) => output.address != null && expected.contains(output.address),
273+
);
201274
}
202275
}

0 commit comments

Comments
 (0)