What the developer wrote
Authored by SNeedlewoods
fix signed unsigned comparison
What changed, and why it matters
This commit fixes a comparison between a signed and unsigned number in Monero's wallet code that estimates the current blockchain height. Before the fix, mixing signed and unsigned values could lead to incorrect behavior when calculating block height near a future network fork. The patch makes the calculation use only unsigned numbers and keeps the safety check that returns an error if the estimate would go negative. This is a correctness and robustness fix rather than a direct theft-of-funds vulnerability, but bad height estimates could affect transaction creation or fee calculations.
Treat as a low-severity correctness fix. Apply the patch. Review callers of get_approximate_blockchain_height() to confirm they handle a return value of 0 safely, and consider adding static-analysis rules to catch signed/unsigned comparisons in wallet code.
Security signals we found
signed/unsigned integer comparison
potential arithmetic underflow guard bypass due to implicit conversion
incorrect blockchain height approximation could affect transaction validity decisions
no explicit memory-safety bug in diff
Evidence from the diff
In wallet2::get_approximate_blockchain_height(), the original code compared approx_blockchain_height (uint64_t) directly with (fork_time - now) / seconds_per_block (time_t / time_t, a signed arithmetic expression). On platforms where time_t is signed, this mixed signed/unsigned comparison triggers the usual arithmetic conversions: the signed value is converted to uint64_t, so a negative result becomes a huge positive number, making the > check behave unexpectedly. The patch explicitly casts the elapsed-block counts to uint64_t and restructures the logic so the guard against underflow is preserved. There is no direct buffer overflow or memory corruption, but the bug could produce a wildly wrong approximate blockchain height, which downstream wallet logic uses for unlock-time checks, fee estimation, and relay decisions.
Changed components
src/wallet/wallet2.cppwallet2::get_approximate_blockchain_height()Inspect captured patch +12 / −5
diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp
index e7b8b81..20b35fa 100644
--- a/src/wallet/wallet2.cpp
+++ b/src/wallet/wallet2.cpp
@@ -12799,13 +12799,20 @@ uint64_t wallet2::get_approximate_blockchain_height() const
uint64_t approx_blockchain_height = fork_block;
const time_t now = time(NULL);
if (now > fork_time)
- approx_blockchain_height += (now - fork_time) / seconds_per_block;
- else if (approx_blockchain_height > (fork_time - now) / seconds_per_block)
- approx_blockchain_height -= (fork_time - now) / seconds_per_block;
+ {
+ const uint64_t blocks_since_last_fork = static_cast<uint64_t>((now - fork_time) / seconds_per_block);
+ approx_blockchain_height += blocks_since_last_fork;
+ }
else
{
- LOG_ERROR("Failed to approximate blockchain height from future fork block: " << approx_blockchain_height);
- return 0;
+ const uint64_t blocks_until_next_fork = static_cast<uint64_t>((fork_time - now) / seconds_per_block);
+ if (approx_blockchain_height > blocks_until_next_fork)
+ approx_blockchain_height -= blocks_until_next_fork;
+ else
+ {
+ LOG_ERROR("Failed to approximate blockchain height from future fork block: " << approx_blockchain_height);
+ return 0;
+ }
}
// testnet and stagenet got some huge rollbacks, so the estimation is way off
const uint64_t approximate_rolled_back_blocks = m_nettype == TESTNET ? 26600 : m_nettype == STAGENET ? 48600 : 33600;
Why this scored 26/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.