DragonX/ObsidianDragon
1
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 4 Wiki Activity

fix(audit): batch 1 — latent unlock/path/rescan correctness bugs

Browse Source 
Four correctness fixes surfaced by the codebase audit:

1. Lite address-book path bug: AddressBook::getDefaultPath() hardcoded
   "ObsidianDragon" while Settings::getDefaultPath() uses DRAGONX_APP_NAME,
   so the Lite build wrote addressbook.json into the full-node app's config
   dir instead of ObsidianDragonLite/. Now uses DRAGONX_APP_NAME on all
   platforms (no macOS path change — the getConfigDir consolidation, which
   would move the macOS dir, is deferred).

2. PIN-unlock lockout bypass: the PIN path duplicated unlockWallet's
   success/lockout logic and its RPC-error branch bumped the attempt counter
   but skipped the escalating-lockout math entirely — a PIN user hitting an
   RPC error escaped the lockout curve. Extracted App::applyUnlockSuccess()
   and App::applyUnlockFailure() and routed all unlock paths (passphrase,
   PIN vault-fail, PIN RPC-fail) through them, so the lockout curve now
   applies uniformly. (noVault stays a mode-switch, not a failed attempt.)

3. Witness/rescan reset drift: the ~11-line rescan+witness completion reset
   was copy-pasted at four sites (app.cpp x2, app_network.cpp x2); adding a
   witness field and missing a copy would leave stale progress. Folded into
   App::resetWitnessRescanProgress(). Left the distinct phase-transition
   reset in App::update() untouched (it sets witness_phase, not 0).

4. Truncation underflow: send_tab/receive_tab's size_t TruncateAddress
   copies computed (maxLen - 3) without guarding maxLen <= 3, which wraps
   and throws std::out_of_range on short inputs. Added the guard.

Full-node + Lite build clean; ctest 1/1; source-hygiene clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
DanS
2026-07-02 15:54:36 -05:00
parent 530bf24abf
commit f8150434d4
7 changed files with 89 additions and 94 deletions
Download Patch File Download Diff File Expand all files Collapse all files

24
src/app.cpp
View File

@@ -777,17 +777,8 @@ void App::update()
// poll (which hits the still-running pre-restart daemon, rescanning=false)
// would fire a false "complete" the instant rescan was clicked.
ui::Notifications::instance().success("Blockchain rescan complete");
state_.sync.rescanning = false;
rescan_confirmed_active_ = false;
resetWitnessRescanProgress();
state_.sync.rescan_progress = 1.0f;
state_.sync.rescan_status.clear();
state_.sync.building_witnesses = false;
state_.sync.witness_phase = 0;
state_.sync.witness_progress = 0.0f;
state_.sync.witness_remaining = 0;
witness_rebuild_total_blocks_ = 0;
witness_seen_txids_.clear();
witness_total_txs_ = 0;
}
// else: rescanning=false but not yet confirmed → pre-restart daemon; keep waiting.
};
@@ -1048,18 +1039,9 @@ void App::update()
if (state_.sync.rescanning) {
ui::Notifications::instance().success("Blockchain rescan complete");
}
state_.sync.rescanning = false;
rescan_confirmed_active_ = false;
state_.sync.rescan_progress = 1.0f;
state_.sync.rescan_status.clear();
// Witness rebuild finishes with the rescan it's part of.
state_.sync.building_witnesses = false;
state_.sync.witness_phase = 0;
state_.sync.witness_progress = 0.0f;
state_.sync.witness_remaining = 0;
witness_rebuild_total_blocks_ = 0;
witness_seen_txids_.clear();
witness_total_txs_ = 0;
resetWitnessRescanProgress();
state_.sync.rescan_progress = 1.0f;
} else if (foundWitness) {
// Witness rebuild is the tail phase of a rescan — keep rescanning set so
// the broader gating holds, but surface the witness-specific progress.

8
src/app.h
View File

@@ -473,6 +473,14 @@ private:
std::string transactionHistoryCacheWalletIdentity() const;
bool ensureTransactionHistoryCacheUnlockedFor(const std::string& walletIdentity);
void unlockTransactionHistoryCacheWithPassphrase(const std::string& passphrase);
// Shared main-thread continuations for a wallet unlock attempt, so the passphrase
// and PIN paths cannot drift. applyUnlockFailure applies the escalating lockout
// curve on every failed path (a PIN RPC failure used to skip it).
void applyUnlockSuccess(const std::string& passphrase, int timeout);
void applyUnlockFailure(const std::string& errorMessage);
// Clear all rescan + witness-rebuild progress/accumulator state. Shared by the four
// rescan-completion sites so a new witness field can't be forgotten in one copy.
void resetWitnessRescanProgress();
void loadTransactionHistoryCacheIfAvailable();
void storeTransactionHistoryCacheIfAvailable();
void wipePendingTransactionHistoryCachePassphrase();

40
src/app_network.cpp
View File

@@ -924,6 +924,24 @@ void App::applyPendingSendBalanceDeltas(bool includeAggregateBalances)
}
}
void App::resetWitnessRescanProgress()
{
// Common core of the four rescan-completion sites. Deliberately does NOT set
// rescan_progress (callers set it to 1.0 only on success) or runtime_rescan_active_
// (only the manual-rescan path uses it), and is distinct from the witness phase-transition
// reset in App::update() which sets witness_phase to the new phase rather than clearing it.
rescan_confirmed_active_ = false;
state_.sync.rescanning = false;
state_.sync.rescan_status.clear();
state_.sync.building_witnesses = false;
state_.sync.witness_phase = 0;
state_.sync.witness_progress = 0.0f;
state_.sync.witness_remaining = 0;
witness_rebuild_total_blocks_ = 0;
witness_seen_txids_.clear();
witness_total_txs_ = 0;
}
std::string App::transactionHistoryCacheWalletIdentity() const
{
std::vector<std::string> shieldedAddresses;
@@ -1156,17 +1174,8 @@ void App::refreshCoreData()
// rescan_confirmed_active_ ensures we actually observed this rescan running (set
// by the getrescaninfo / daemon-log pollers) before declaring it done.
if (state_.sync.rescanning && rescan_confirmed_active_) {
state_.sync.rescanning = false;
rescan_confirmed_active_ = false;
resetWitnessRescanProgress();
state_.sync.rescan_progress = 1.0f;
state_.sync.rescan_status.clear();
state_.sync.building_witnesses = false;
state_.sync.witness_phase = 0;
state_.sync.witness_progress = 0.0f;
state_.sync.witness_remaining = 0;
witness_rebuild_total_blocks_ = 0;
witness_seen_txids_.clear();
witness_total_txs_ = 0;
// Notes/witnesses were rebuilt — force a fresh history + balance pull.
transactions_dirty_ = true;
last_tx_block_height_ = -1;
@@ -2652,16 +2661,7 @@ void App::runtimeRescan(int startHeight)
}
return [this, ok, err]() {
runtime_rescan_active_ = false;
rescan_confirmed_active_ = false;
state_.sync.rescanning = false;
state_.sync.rescan_status.clear();
state_.sync.building_witnesses = false;
state_.sync.witness_phase = 0;
state_.sync.witness_progress = 0.0f;
state_.sync.witness_remaining = 0;
witness_rebuild_total_blocks_ = 0;
witness_seen_txids_.clear();
witness_total_txs_ = 0;
resetWitnessRescanProgress();
if (ok) {
state_.sync.rescan_progress = 1.0f;
transactions_dirty_ = true;

93
src/app_security.cpp
View File

@@ -396,6 +396,38 @@ void App::processDeferredEncryption() {
}
}
// Shared main-thread success continuation for both the passphrase and PIN unlock paths.
// walletpassphrase has already succeeded on the worker thread, so set the unlocked state
// directly rather than issuing another RPC round-trip.
void App::applyUnlockSuccess(const std::string& passphrase, int timeout) {
lock_unlock_in_progress_ = false;
lock_error_msg_.clear();
lock_attempts_ = 0;
memset(lock_passphrase_buf_, 0, sizeof(lock_passphrase_buf_));
last_interaction_ = std::chrono::steady_clock::now();
state_.encrypted = true;
state_.locked = false;
state_.unlocked_until = std::time(nullptr) + timeout;
unlockTransactionHistoryCacheWithPassphrase(passphrase);
}
// Shared main-thread failure continuation: bump the attempt counter, surface the error,
// and apply the escalating lockout curve. Every failed unlock path routes through here so
// none can silently skip the lockout math (the PIN RPC-error path previously did).
void App::applyUnlockFailure(const std::string& errorMessage) {
lock_unlock_in_progress_ = false;
lock_attempts_++;
lock_error_msg_ = errorMessage;
lock_error_timer_ = 3.0f;
memset(lock_passphrase_buf_, 0, sizeof(lock_passphrase_buf_));
const float baseDelay = ui::schema::UI().drawElement("security", "lockout-base-delay").sizeOr(2.0f);
const int maxAttempts = (int)ui::schema::UI().drawElement("security", "max-attempts-before-lockout").sizeOr(5.0f);
if (lock_attempts_ >= maxAttempts) {
lock_lockout_timer_ = baseDelay * (float)(1 << std::min(lock_attempts_ - maxAttempts, 8));
}
DEBUG_LOGF("[App] Wallet unlock failed (attempt %d)\n", lock_attempts_);
}
void App::unlockWallet(const std::string& passphrase, int timeout) {
if (!rpc_ || !rpc_->isConnected() || !worker_) return;
lock_unlock_in_progress_ = true;
@@ -411,30 +443,11 @@ void App::unlockWallet(const std::string& passphrase, int timeout) {
util::SecureVault::secureZero(passphrase.data(), passphrase.size());
return [this, ok, err_msg, timeout, passphrase = std::move(cachePassphrase)]() mutable {
lock_unlock_in_progress_ = false;
if (ok) {
lock_error_msg_.clear();
lock_attempts_ = 0;
memset(lock_passphrase_buf_, 0, sizeof(lock_passphrase_buf_));
last_interaction_ = std::chrono::steady_clock::now();
// Set unlock state immediately — walletpassphrase
// already succeeded, no need for another RPC round-trip.
state_.encrypted = true;
state_.locked = false;
state_.unlocked_until = std::time(nullptr) + timeout;
unlockTransactionHistoryCacheWithPassphrase(passphrase);
applyUnlockSuccess(passphrase, timeout);
} else {
lock_attempts_++;
lock_error_msg_ = TR("incorrect_passphrase");
lock_error_timer_ = 3.0f;
memset(lock_passphrase_buf_, 0, sizeof(lock_passphrase_buf_));
float baseDelay = ui::schema::UI().drawElement("security", "lockout-base-delay").sizeOr(2.0f);
int maxAttempts = (int)ui::schema::UI().drawElement("security", "max-attempts-before-lockout").sizeOr(5.0f);
if (lock_attempts_ >= maxAttempts) {
lock_lockout_timer_ = baseDelay * (float)(1 << std::min(lock_attempts_ - maxAttempts, 8));
}
DEBUG_LOGF("[App] Wallet unlock failed (attempt %d): %s\n", lock_attempts_, err_msg.c_str());
DEBUG_LOGF("[App] Passphrase unlock RPC error: %s\n", err_msg.c_str());
applyUnlockFailure(TR("incorrect_passphrase"));
}
util::SecureVault::secureZero(passphrase.data(), passphrase.size());
};
@@ -980,21 +993,15 @@ void App::renderLockScreen() {
if (!vaultOk) {
bool noVault = !vault_ || !vault_->hasVault();
return [this, noVault]() {
lock_unlock_in_progress_ = false;
if (noVault) {
// Vault file missing — switch to passphrase mode
// Vault file missing — switch to passphrase mode. Not a failed
// attempt, so no counter bump / lockout escalation.
lock_unlock_in_progress_ = false;
lock_error_msg_ = TR("pin_not_set");
lock_use_pin_ = false;
lock_error_timer_ = 3.0f;
} else {
lock_attempts_++;
lock_error_msg_ = TR("incorrect_pin");
}
lock_error_timer_ = 3.0f;
float baseDelay = ui::schema::UI().drawElement("security", "lockout-base-delay").sizeOr(2.0f);
int maxAttempts = (int)ui::schema::UI().drawElement("security", "max-attempts-before-lockout").sizeOr(5.0f);
if (lock_attempts_ >= maxAttempts) {
lock_lockout_timer_ = baseDelay * (float)(1 << std::min(lock_attempts_ - maxAttempts, 8));
applyUnlockFailure(TR("incorrect_pin"));
}
};
}
@@ -1015,25 +1022,15 @@ void App::renderLockScreen() {
if (rpcOk) {
return [this, timeout, passphrase = std::move(passphrase)]() mutable {
lock_unlock_in_progress_ = false;
lock_error_msg_.clear();
lock_attempts_ = 0;
memset(lock_passphrase_buf_, 0, sizeof(lock_passphrase_buf_));
last_interaction_ = std::chrono::steady_clock::now();
// Set unlock state immediately — walletpassphrase
// already succeeded, no need for another RPC round-trip.
state_.encrypted = true;
state_.locked = false;
state_.unlocked_until = std::time(nullptr) + timeout;
unlockTransactionHistoryCacheWithPassphrase(passphrase);
applyUnlockSuccess(passphrase, timeout);
util::SecureVault::secureZero(passphrase.data(), passphrase.size());
};
} else {
// Vault decrypt succeeded but the RPC failed — still a failed unlock,
// so route through applyUnlockFailure so the lockout curve applies
// (this path previously bumped the counter but skipped the lockout math).
return [this, rpcErr, passphrase = std::move(passphrase)]() mutable {
lock_unlock_in_progress_ = false;
lock_attempts_++;
lock_error_msg_ = "Unlock failed: " + rpcErr;
lock_error_timer_ = 3.0f;
applyUnlockFailure("Unlock failed: " + rpcErr);
util::SecureVault::secureZero(passphrase.data(), passphrase.size());
};
}

10
src/data/address_book.cpp
View File

@@ -10,6 +10,7 @@
#include "../util/logger.h"
#include "../util/platform.h"
#include "../config/version.h" // DRAGONX_APP_NAME (per-variant: ObsidianDragon / ObsidianDragonLite)
#ifdef _WIN32
#include <shlobj.h>
@@ -29,10 +30,13 @@ AddressBook::~AddressBook() = default;
std::string AddressBook::getDefaultPath()
{
// Co-locate with settings.json in the per-variant config dir. Use DRAGONX_APP_NAME
// (not a hardcoded "ObsidianDragon") so the Lite build writes to ObsidianDragonLite/
// rather than polluting the full-node app's directory — matching Settings::getDefaultPath().
#ifdef _WIN32
char path[MAX_PATH];
if (SUCCEEDED(SHGetFolderPathA(NULL, CSIDL_APPDATA, NULL, 0, path))) {
std::string dir = std::string(path) + "\\ObsidianDragon";
std::string dir = std::string(path) + "\\" DRAGONX_APP_NAME;
fs::create_directories(dir);
return dir + "\\addressbook.json";
}
@@ -43,7 +47,7 @@ std::string AddressBook::getDefaultPath()
struct passwd* pw = getpwuid(getuid());
home = pw->pw_dir;
}
std::string dir = std::string(home) + "/Library/Application Support/ObsidianDragon";
std::string dir = std::string(home) + "/Library/Application Support/" DRAGONX_APP_NAME;
fs::create_directories(dir);
return dir + "/addressbook.json";
#else
@@ -52,7 +56,7 @@ std::string AddressBook::getDefaultPath()
struct passwd* pw = getpwuid(getuid());
home = pw->pw_dir;
}
std::string dir = std::string(home) + "/.config/ObsidianDragon";
std::string dir = std::string(home) + "/.config/" DRAGONX_APP_NAME;
fs::create_directories(dir);
return dir + "/addressbook.json";
#endif

4
src/ui/windows/receive_tab.cpp
View File

@@ -72,7 +72,9 @@ static bool s_generating_address = false;
// Helpers
// ============================================================================
static std::string TruncateAddress(const std::string& addr, size_t maxLen = 40) {
if (addr.length() <= maxLen) return addr;
// Guard maxLen <= 3 before the unsigned (maxLen - 3): otherwise it wraps and the
// trailing substr(length - halfLen) throws std::out_of_range on short inputs.
if (maxLen <= 3 || addr.length() <= maxLen) return addr;
size_t halfLen = (maxLen - 3) / 2;
return addr.substr(0, halfLen) + "..." + addr.substr(addr.length() - halfLen);
}

4
src/ui/windows/send_tab.cpp
View File

@@ -148,7 +148,9 @@ static double GetAvailableBalance(App* app) {
}
static std::string TruncateAddress(const std::string& addr, size_t maxLen = 40) {
if (addr.length() <= maxLen) return addr;
// Guard maxLen <= 3 before the unsigned (maxLen - 3): otherwise it wraps and the
// trailing substr(length - halfLen) throws std::out_of_range on short inputs.
if (maxLen <= 3 || addr.length() <= maxLen) return addr;
size_t halfLen = (maxLen - 3) / 2;
return addr.substr(0, halfLen) + "..." + addr.substr(addr.length() - halfLen);
}