From 46de4cbfee8b042e8928596792e861931a4ee556 Mon Sep 17 00:00:00 2001 From: DanS Date: Mon, 19 Oct 2026 18:55:02 -0500 Subject: [PATCH] =?UTF-8?q?fix(fund):=20Tier-B=20parser/aggregation=20hard?= =?UTF-8?q?ening=20=E2=80=94=20range-check=20amounts,=20int-satoshi=20sums?= =?UTF-8?q?,=20strict=20balance=20parse?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fund-correctness audit, Tier-B (parse/validation/display only — no send/sweep/encrypt call touched; each change can only reject bad input or remove drift, never over-report): - lite_result_parsers: cap every backend-reported amount at the 21,000,000 DRGX supply (note value, outgoing output value + running sum, received amount, the net-amount fallback magnitude), and replace the two bare `return false` paths in the outgoing-metadata loop — which left ok=false with an EMPTY error and silently rejected the whole tx list — with fail() carrying a reason. - network_refresh_service: accumulate per-address balances in integer satoshis (util::toSats) instead of summing doubles, so many notes can't drift by an ulp per addition (ten 0.1 notes now sum to exactly 1.0); skip non-finite amounts. - readBalanceString: strict decimal parse (digits / one '.' / optional leading '-') instead of std::stod, which silently accepted "1.5abc", whitespace, hex, exponents and nan/inf. Tests: testLiteParserAmountRangeChecks + integer-satoshi drift + strict-parse cases. Co-Authored-By: Claude Opus 4.8 --- src/services/network_refresh_service.cpp | 32 +++++++++---- src/wallet/lite_result_parsers.cpp | 21 +++++++-- tests/test_phase4.cpp | 59 ++++++++++++++++++++++++ 3 files changed, 101 insertions(+), 11 deletions(-) diff --git a/src/services/network_refresh_service.cpp b/src/services/network_refresh_service.cpp index 8e1c2da..56fd49b 100644 --- a/src/services/network_refresh_service.cpp +++ b/src/services/network_refresh_service.cpp @@ -3,6 +3,7 @@ #include "rpc/rpc_busy_gate.h" #include "../util/logger.h" #include "../util/i18n.h" // util::tr for the price-fetch-failed default +#include "../util/amount_format.h" // util::toSats: integer-satoshi accumulation #include #include @@ -34,7 +35,20 @@ std::optional readOptional(const json& source, const char* key) std::optional readBalanceString(const json& source, const char* key) { try { - if (source.contains(key)) return std::stod(source[key].get()); + if (source.contains(key)) { + const std::string text = source[key].get(); + // Strict: plain decimal only (digits, one '.', optional leading '-'). std::stod would accept + // '1.5abc', whitespace, '+', hex, exponents and nan/inf, silently. + if (text.empty()) return std::nullopt; + for (std::size_t i = 0; i < text.size(); ++i) { + const char c = text[i]; + if (!((c >= '0' && c <= '9') || c == '.' || (c == '-' && i == 0))) return std::nullopt; + } + char* end = nullptr; + const double value = std::strtod(text.c_str(), &end); + if (end != text.c_str() + text.size() || !std::isfinite(value)) return std::nullopt; + return value; + } } catch (...) {} return std::nullopt; } @@ -52,15 +66,17 @@ void applyBalancesFromUnspent(std::vector& addresses, const json& u // Partition each note/utxo by its per-entry "confirmations": `total` (minconf=0 — DISPLAY, includes // the user's own pending 0-conf change) vs `spendable` (confirmations>=1 — what z_sendmany, run at // minconf=1, can actually spend). This lets a single z_listunspent(0)/listunspent(0) feed both. - std::map total; - std::map spendable; + // Accumulated in integer satoshis (not doubles) so many notes can't drift by an ulp per addition. + std::map total; + std::map spendable; for (const auto& output : unspent) { auto address = readOptional(output, "address"); auto amount = readOptional(output, "amount"); - if (!address || !amount) continue; - total[*address] += *amount; + if (!address || !amount || !std::isfinite(*amount)) continue; + const std::int64_t sats = util::toSats(*amount); + total[*address] += sats; auto conf = readOptional(output, "confirmations"); - if (conf && *conf >= 1) spendable[*address] += *amount; + if (conf && *conf >= 1) spendable[*address] += sats; } // The address lists are rebuilt fresh (default 0) each refresh, so hard-set both — an address with no @@ -68,8 +84,8 @@ void applyBalancesFromUnspent(std::vector& addresses, const json& u for (auto& info : addresses) { auto t = total.find(info.address); auto s = spendable.find(info.address); - info.balance = (t != total.end()) ? t->second : 0.0; - info.spendableBalance = (s != spendable.end()) ? s->second : 0.0; + info.balance = (t != total.end()) ? static_cast(t->second) / 1e8 : 0.0; + info.spendableBalance = (s != spendable.end()) ? static_cast(s->second) / 1e8 : 0.0; } } diff --git a/src/wallet/lite_result_parsers.cpp b/src/wallet/lite_result_parsers.cpp index 0918ad6..3a7663e 100644 --- a/src/wallet/lite_result_parsers.cpp +++ b/src/wallet/lite_result_parsers.cpp @@ -79,6 +79,9 @@ bool requireArray(const json& value, const std::string& path, Result& result) return true; } +// 21,000,000 DRGX in zatoshi: no single backend-reported amount can legitimately exceed the supply. +constexpr std::int64_t kMaxMoneyZat = static_cast(21000000) * 100000000LL; + bool jsonToUnsigned(const json& value, std::uint64_t& output) { try { @@ -282,6 +285,9 @@ bool parseSpendableOutput(const json& value, if (!readRequiredStringField(value, "created_in_txid", output.createdInTxid, result)) return false; if (!readOptionalSignedField(value, "created_in_block", output.createdInBlock, result)) return false; if (!readRequiredUnsignedField(value, "value", output.value, result)) return false; + if (output.value > static_cast(kMaxMoneyZat)) { + return fail(result, LiteResultParserError::InvalidFieldValue, path + ".value", "note value exceeds the maximum supply"); + } if (!readOptionalBoolField(value, "spent", output.spent, result)) return false; if (!readOptionalBoolField(value, "unconfirmed_spent", output.unconfirmedSpent, result)) return false; output.spendable = !output.pending && !output.spent && !output.unconfirmedSpent; @@ -352,8 +358,13 @@ bool parseTransactionRecord(const json& value, if (!parseTransactionOutput(metadata[index], path + ".outgoing_metadata[" + std::to_string(index) + "]", parsedOutput, result)) return false; // Reject negative values and guard the running sum: signed overflow is UB, and a hostile // backend could otherwise overflow the total into a bogus (even negative) amount. - if (parsedOutput.value < 0) return false; - if (output.amount > std::numeric_limits::max() - parsedOutput.value) return false; + const std::string outputPath = path + ".outgoing_metadata[" + std::to_string(index) + "]"; + if (parsedOutput.value < 0 || parsedOutput.value > kMaxMoneyZat) { + return fail(result, LiteResultParserError::InvalidFieldValue, outputPath + ".value", "outgoing output value is negative or exceeds the maximum supply"); + } + if (output.amount > kMaxMoneyZat - parsedOutput.value) { + return fail(result, LiteResultParserError::InvalidFieldValue, path + ".outgoing_metadata", "outgoing output total exceeds the maximum supply"); + } output.amount += parsedOutput.value; output.outgoingMetadata.push_back(std::move(parsedOutput)); } @@ -362,7 +373,8 @@ bool parseTransactionRecord(const json& value, // totals aren't understated. Display-only; tolerant of a missing/odd field. if (output.amount == 0 && value.contains("amount") && !value.at("amount").is_null()) { std::int64_t net = 0; - if (jsonToSigned(value.at("amount"), net) && net != std::numeric_limits::min()) { + if (jsonToSigned(value.at("amount"), net) && net != std::numeric_limits::min() + && (net < 0 ? -net : net) <= kMaxMoneyZat) { output.amount = net < 0 ? -net : net; } } @@ -372,6 +384,9 @@ bool parseTransactionRecord(const json& value, output.direction = LiteTransactionDirection::Receive; if (!readRequiredStringField(value, "address", output.address, result)) return false; if (!readRequiredSignedField(value, "amount", output.amount, result)) return false; + if (output.amount < 0 || output.amount > kMaxMoneyZat) { + return fail(result, LiteResultParserError::InvalidFieldValue, path + ".amount", "received amount is negative or exceeds the maximum supply"); + } if (!readOptionalStringField(value, "memo", output.memo, result)) return false; if (!readOptionalSignedField(value, "position", output.position, result)) return false; return true; diff --git a/tests/test_phase4.cpp b/tests/test_phase4.cpp index f41df05..fb3b717 100644 --- a/tests/test_phase4.cpp +++ b/tests/test_phase4.cpp @@ -3010,6 +3010,36 @@ void testNetworkRefreshResultModels() EXPECT_EQ(confirmedCache[0].txid, std::string("confirmed")); EXPECT_EQ(confirmedIds.count("confirmed"), static_cast(1)); EXPECT_EQ(confirmedBlock, 222); + + // Tier-B: per-address balances accumulate in integer satoshis (no IEEE drift over many notes). + { + std::vector addrs(1); + addrs[0].address = "zs1drift"; + json notes = json::array(); + for (int i = 0; i < 10; ++i) + notes.push_back(json{{"address", "zs1drift"}, {"amount", 0.1}, {"confirmations", 3}}); + Refresh::applyShieldedBalancesFromUnspent(addrs, notes); + EXPECT_TRUE(addrs[0].balance == 1.0); // exact; double summation would give 0.9999999999999999 + EXPECT_TRUE(addrs[0].spendableBalance == 1.0); + } + + // Tier-B: readBalanceString rejects anything but a plain decimal (std::stod accepted trailing garbage). + { + auto bad = Refresh::parseCoreRefreshResult( + json{{"private", "1.5abc"}, {"transparent", ""}, {"total", "nan"}}, json(), true, json::object(), false); + EXPECT_TRUE(!bad.shieldedBalance.has_value()); + EXPECT_TRUE(!bad.transparentBalance.has_value()); + EXPECT_TRUE(!bad.totalBalance.has_value()); + auto bad2 = Refresh::parseCoreRefreshResult( + json{{"private", " 1.0"}, {"transparent", "0x10"}, {"total", "1e3"}}, json(), true, json::object(), false); + EXPECT_TRUE(!bad2.shieldedBalance.has_value()); + EXPECT_TRUE(!bad2.transparentBalance.has_value()); + EXPECT_TRUE(!bad2.totalBalance.has_value()); + auto good = Refresh::parseCoreRefreshResult( + json{{"private", "1.25000000"}, {"transparent", "0.50000000"}, {"total", "1.75000000"}}, json(), true, json::object(), false); + EXPECT_NEAR(*good.shieldedBalance, 1.25, 1e-9); + EXPECT_NEAR(*good.totalBalance, 1.75, 1e-9); + } } void testOperationStatusPollParsing() @@ -11923,6 +11953,34 @@ void testLiteForeignSendFallsBackToRecordAmount() EXPECT_EQ(own.transactions.transactions[0].amount, static_cast(5)); } +// Tier-B: the parser range-checks amounts (reject negative / over-supply) and fails with a reason instead +// of a silent bare `return false` that rejected the whole list with an empty error. +void testLiteParserAmountRangeChecks() +{ + using namespace dragonx::wallet; + // Negative receive amount rejected with a reason. + auto neg = parseLiteTransactionsResponse(std::string(R"([{"txid":"a","datetime":1,"address":"zs1","amount":-5}])")); + EXPECT_TRUE(!neg.ok); + EXPECT_TRUE(!neg.errorMessage.empty()); + // Absurd receive amount (> 21M DRGX) rejected. + auto big = parseLiteTransactionsResponse(std::string(R"([{"txid":"b","datetime":1,"address":"zs1","amount":2100000000000001}])")); + EXPECT_TRUE(!big.ok); + // Max supply itself is accepted. + auto edge = parseLiteTransactionsResponse(std::string(R"([{"txid":"c","datetime":1,"address":"zs1","amount":2100000000000000}])")); + EXPECT_TRUE(edge.ok); + // A negative outgoing output now fails with a message instead of a silent reject. + auto sneg = parseLiteTransactionsResponse(std::string(R"([{"txid":"d","datetime":1,"outgoing_metadata":[{"address":"zs1","value":-1}]}])")); + EXPECT_TRUE(!sneg.ok); + EXPECT_TRUE(!sneg.errorMessage.empty()); + // Sum of outputs over the cap is rejected. + auto sover = parseLiteTransactionsResponse(std::string(R"([{"txid":"e","datetime":1,"outgoing_metadata":[{"address":"a","value":2000000000000000},{"address":"b","value":2000000000000000}]}])")); + EXPECT_TRUE(!sover.ok); + EXPECT_TRUE(!sover.errorMessage.empty()); + // Absurd note value rejected. + auto note = parseLiteNotesResponse(std::string(R"({"unspent_notes":[{"address":"zs1","created_in_txid":"t","value":9000000000000000}],"utxos":[],"pending_notes":[],"pending_utxos":[]})")); + EXPECT_TRUE(!note.ok); +} + // A tolerated partial refresh where the notes/utxo command failed (hasAddresses but // !hasSpendableOutputs) must NOT zero every per-address balance — it keeps the last-known // values so a correct total isn't accompanied by a misleading all-zero breakdown. @@ -19683,6 +19741,7 @@ int main() testLiteSendShowsRecipientFromOutgoing(); testLiteSendAmountIsNegative(); testLiteForeignSendFallsBackToRecordAmount(); + testLiteParserAmountRangeChecks(); testLitePartialRefreshKeepsPriorAddressBalances(); testLitePerAddressSpendableAndViewOnly(); testLitePerAddressBalances();