fix: sanitize Electrum fee estimates (#3499)
What the developer wrote
Authored by Seth For Privacy
fix: sanitize Electrum fee estimates (#3499)
* fix: sanitize Electrum fee estimates
* chore: drop test changes from this PR
Removes the test additions/modifications introduced by this branch so the
test design is left to the maintainers. Production code is unchanged.
What changed, and why it matters
This commit hardens how Cake Wallet handles Bitcoin fee estimates from Electrum servers. Previously, a malicious or misbehaving Electrum server could return extreme or negative fee numbers, which could lead to users paying far too much, far too little, or creating invalid transactions. The patch now clamps fee rates between 0 and 2000 sat/vB, rejects all-zero fee lists, and refuses to build transactions when the computed fee is zero or negative. Most of the rest of the diff is just code formatting cleanup.
Treat this as a defensive hardening fix. Review whether the 2000 sat/vB cap is appropriate for all supported networks and fee markets, and consider adding unit tests for the sanitizer and `_isValidFeeRates()` behavior. The commit message notes tests were intentionally dropped from this PR.
Security signals we found
Untrusted Electrum server input is now bounded and validated before use in fee selection
Negative or zero fee rates are rejected, reducing risk of fee-underpayment or invalid transactions
Excessive fee rates are capped, limiting maximum overpayment from malicious server responses
Transaction creation now fails on non-positive fees rather than only on exactly-zero fees
Evidence from the diff
The change introduces _sanitizeFeeRate() in electrum.dart, which floors negative estimatefee results at 0 and caps them at 2000 sat/vB (per internal ticket CW-1597). feeRates() now applies this sanitizer to all three priority estimates. In electrum_wallet.dart, updateFeeRates() replaces a fragile != [0, 0, 0] check with _isValidFeeRates(), requiring exactly three positive values before updating cached rates. Two transaction-building paths now throw BitcoinTransactionNoFeeException when fee <= 0 instead of only when fee == 0. The remaining diff is whitespace/formatting.
Changed components
cw_bitcoin/lib/electrum.dartcw_bitcoin/lib/electrum_wallet.dartElectrum fee estimation flowBitcoin transaction fee validationInspect captured patch +44 / −23
diff --git a/cw_bitcoin/lib/electrum.dart b/cw_bitcoin/lib/electrum.dart
index 44aa096c..b3bc6e84 100644
--- a/cw_bitcoin/lib/electrum.dart
+++ b/cw_bitcoin/lib/electrum.dart
@@ -579,14 +579,29 @@ class ElectrumClient {
return [];
});
+ // Floor at 0 so unavailable/-1 estimates never become negative rates;
+ // cap at 2000 sat/vB per CW-1597.
+ static const int _maxFeeRate = 2000;
+
+ static int _sanitizeFeeRate(double feeRate) {
+ final rate = (stringDoubleToBitcoinAmount(feeRate.toString()) / 1000).round();
+ if (rate < 0) {
+ return 0;
+ }
+ if (rate > _maxFeeRate) {
+ return _maxFeeRate;
+ }
+ return rate;
+ }
+
Future<List<int>> feeRates({BasedUtxoNetwork? network}) async {
try {
- final topDoubleString = await estimatefee(p: 1);
- final middleDoubleString = await estimatefee(p: 5);
- final bottomDoubleString = await estimatefee(p: 10);
- final top = (stringDoubleToBitcoinAmount(topDoubleString.toString()) / 1000).round();
- final middle = (stringDoubleToBitcoinAmount(middleDoubleString.toString()) / 1000).round();
- final bottom = (stringDoubleToBitcoinAmount(bottomDoubleString.toString()) / 1000).round();
+ final topDouble = await estimatefee(p: 1);
+ final middleDouble = await estimatefee(p: 5);
+ final bottomDouble = await estimatefee(p: 10);
+ final top = _sanitizeFeeRate(topDouble);
+ final middle = _sanitizeFeeRate(middleDouble);
+ final bottom = _sanitizeFeeRate(bottomDouble);
return [bottom, middle, top];
} catch (_) {
diff --git a/cw_bitcoin/lib/electrum_wallet.dart b/cw_bitcoin/lib/electrum_wallet.dart
index 5d3af65f..857317a9 100644
--- a/cw_bitcoin/lib/electrum_wallet.dart
+++ b/cw_bitcoin/lib/electrum_wallet.dart
@@ -750,6 +750,9 @@ abstract class ElectrumWalletBase
}
}
+ static bool _isValidFeeRates(List<int> feeRates) =>
+ feeRates.length == 3 && feeRates.every((rate) => rate > 0);
+
@action
Future<void> updateFeeRates() async {
if (await checkIfMempoolAPIIsEnabled() && type == WalletType.bitcoin) {
@@ -776,7 +779,7 @@ abstract class ElectrumWalletBase
}
final feeRates = await electrumClient.feeRates(network: network);
- if (feeRates != [0, 0, 0]) {
+ if (_isValidFeeRates(feeRates)) {
_feeRates = feeRates;
} else if (isTestnet) {
_feeRates = [1, 1, 1];
@@ -1024,7 +1027,7 @@ abstract class ElectrumWalletBase
vinOutpoints: utxoDetails.vinOutpoints,
);
- if (fee == 0) {
+ if (fee <= 0) {
throw BitcoinTransactionNoFeeException();
}
@@ -1205,7 +1208,7 @@ abstract class ElectrumWalletBase
));
}
- if (fee == 0) {
+ if (fee <= 0) {
throw BitcoinTransactionNoFeeException();
}
@@ -2513,16 +2516,16 @@ abstract class ElectrumWalletBase
Map<String, ElectrumTransactionInfo> historiesWithDetails,
BitcoinAddressType type,
) async {
-
final addressesByType =
- walletAddresses.allAddresses.where((addr) => addr.type == type).toList();
+ walletAddresses.allAddresses.where((addr) => addr.type == type).toList();
final receiveStandard = getAddressBranchByType(hidden: false, legacy: false, type: type);
- final changeStandard = getAddressBranchByType(hidden: true, legacy: false, type: type);
+ final changeStandard = getAddressBranchByType(hidden: true, legacy: false, type: type);
final receiveLegacy = getAddressBranchByType(hidden: false, legacy: true, type: type);
- final changeLegacy = getAddressBranchByType(hidden: true, legacy: true, type: type);
+ final changeLegacy = getAddressBranchByType(hidden: true, legacy: true, type: type);
- walletAddresses.hiddenAddresses.addAll([...changeStandard, ...changeLegacy].map((e) => e.address));
+ walletAddresses.hiddenAddresses
+ .addAll([...changeStandard, ...changeLegacy].map((e) => e.address));
await walletAddresses.saveAddressesInBox();
await Future.wait(addressesByType.map((addressRecord) async {
final history = await _fetchAddressHistory(addressRecord, await getCurrentChainTip());
@@ -2637,13 +2640,13 @@ abstract class ElectrumWalletBase
Future<void> fetchTransactionsForAddressTypeBatch(
Map<String, ElectrumTransactionInfo> historiesWithDetails, BitcoinAddressType type) async {
-
final receiveStandard = getAddressBranchByType(hidden: false, legacy: false, type: type);
- final changeStandard = getAddressBranchByType(hidden: true, legacy: false, type: type);
+ final changeStandard = getAddressBranchByType(hidden: true, legacy: false, type: type);
final receiveLegacy = getAddressBranchByType(hidden: false, legacy: true, type: type);
- final changeLegacy = getAddressBranchByType(hidden: true, legacy: true, type: type);
+ final changeLegacy = getAddressBranchByType(hidden: true, legacy: true, type: type);
- walletAddresses.hiddenAddresses.addAll([...changeStandard, ...changeLegacy].map((e) => e.address));
+ walletAddresses.hiddenAddresses
+ .addAll([...changeStandard, ...changeLegacy].map((e) => e.address));
await walletAddresses.saveAddressesInBox();
await fetchTransactionsForAddressesBranchBatch(
@@ -2665,7 +2668,7 @@ abstract class ElectrumWalletBase
await fetchTransactionsForAddressesBranchBatch(
historiesWithDetails,
type,
- receiveLegacy,
+ receiveLegacy,
isHidden: false,
isLegacyDerivation: true,
);
@@ -2749,10 +2752,13 @@ abstract class ElectrumWalletBase
}
}
- List<BitcoinAddressRecord> getAddressBranchByType({required bool hidden, required bool legacy, required BitcoinAddressType
- type}) => walletAddresses.allAddresses.where((addr) => addr.type == type && addr.isHidden == hidden && addr.isLegacyDerivation == legacy)
- .toList()
- ..sort((a, b) => a.index.compareTo(b.index));
+ List<BitcoinAddressRecord> getAddressBranchByType(
+ {required bool hidden, required bool legacy, required BitcoinAddressType type}) =>
+ walletAddresses.allAddresses
+ .where((addr) =>
+ addr.type == type && addr.isHidden == hidden && addr.isLegacyDerivation == legacy)
+ .toList()
+ ..sort((a, b) => a.index.compareTo(b.index));
int _highestUsedIndex(List<BitcoinAddressRecord> addresses) {
for (int i = addresses.length - 1; i >= 0; i--) {
Why this scored 59/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.