Critical Bugs

[oli737]

=== CRITICAL BUGS IN NERDMINER V2 ===

1. ESP32 FREEZE ON SHARE FOUND (Bugs F1 & F2)

   • Affected File: utils.cpp (within "checkValid()")
   • Issue A: The function uses 'memcpy(diff_target, &target, 32)', which copies the address
     of the pointer instead of the actual data, loading memory garbage.
   • Issue B: The loop 'for(uint8_t i=31; i>=0; i--)' uses an unsigned integer (uint8_t)
     which cannot be negative. Decrementing past 0 wraps it to 255, creating an
     infinite loop that immediately freezes the ESP32.

2. CORRUPTED TARGET COMPARISON (Bug BUG-3)

   • Affected File: utils.cpp (within "calculateMiningData()")
   • Issue: The function calls 'to_byte_array(target, 32, mMiner.bytearray_target)'. The size
     argument should be 64 (for 64 hex characters) instead of 32.
   • Impact: Only the first 16 bytes of the 32-byte target are written, leaving the rest
     uninitialized. Valid blocks/shares cannot be correctly validated.

3. STACK OVERFLOW VULNERABILITY (Bug BUG-2)

   • Affected File: utils.cpp (within "calculateMiningData()")
   • Issue: The calculation of the target buffer offset ('zeros - 2') derived from pool 'nbits'
     lacks boundary verification.
   • Impact: A custom or malicious pool sending very low difficulty exponents causes a negative
     offset, triggering out-of-bounds stack corruption and crashing the ESP32.

[Gheop]

Confirmed on hardware, and I can add two more issues in the same function that
aren't in the original report. I've been running 6 T-Display-S3 miners for several
weeks and audited this code after reading this issue.

Confirming the reported bugs

All three are still present in main (e3a04b7):

// utils.cpp:128
memcpy(diff_target, &target, 32);   // copies the pointer's own address off the
                                    // stack, not the 32 target bytes

// utils.cpp:132
for (uint8_t i = 31; i >= 0; i--)   // i is unsigned, so i >= 0 is always true:
                                    // after 0 it wraps to 255

// utils.cpp:203
to_byte_array(target, 32, mMiner.bytearray_target);   // 64 hex chars = 32 bytes,
                                    // so only 16 of them were ever written

Two more, not in the report

4. The XOR swap that follows to_byte_array is hardcoded for 16 bytes.

for (size_t j = 0; j < 8; j++)      // swaps j with size_target-1-j

Eight iterations fully reverse a 16-byte array. Once bug 3 is fixed and
size_target becomes 32, this only reverses the outer 8 bytes on each side and
leaves bytes 8..23 untouched. Fixing bug 3 alone makes the target worse, so the
two must be fixed together. size_target / 2 handles both.

5. The loop never breaks when the hash is already below target.

It only breaks on hash[i] > diff_target[i]. When hash[i] < diff_target[i],
meaning the hash is valid and the comparison should stop right there: it keeps
walking down into less significant bytes, where a later hash[j] > target[j] makes
it wrongly return "invalid".

Byte order (worth stating, it's the subtle part)

hash and mMiner.bytearray_target are both little-endian, most significant
byte at index 31:

• diff_from_target() reads the hash with le256todouble()
• the 32-zero-bits test is hash[29] == 0 && hash[28] == 0, i.e. high bytes at
  high indices
• the target is byte-reversed at the end of calculateMiningData()

So they compare directly, most significant byte first, and the reverse_bytes()
call inside checkValid() is wrong: it flips the target to big-endian while the
hash stays little-endian.

Reproducing the freeze

Standalone host test (no hardware needed), original vs fixed on known vectors.
Difficulty-1 target, so byte 27 is 0xFF:

case                             | expected | fixed | original
---------------------------------+----------+-------+---------
all-zero hash (well below)       |   true   | true  | true    <- INFINITE LOOP
high byte set (clearly above)    |   false  | false | true    <- INFINITE LOOP
exactly equal to target          |   true   | true  | false
above on a high byte             |   false  | false | false

fixed: 4/4 correct        original: 2/4 correct

Two of the four cases never terminate: I had to add an iteration counter and bail
at 100k to get the run to finish. That matches the freeze described here: on the
ESP32 this runs inside the task that handles the pool connection, so the symptom is
"hashing continues, screen keeps updating, but stratum is dead".

Fix

bool checkValid(unsigned char* hash, unsigned char* target) {
  bool valid = true;
  unsigned char diff_target[32];
  memcpy(diff_target, target, 32);          // was &target

  for (int i = 31; i >= 0; i--) {           // was uint8_t
    if (hash[i] > diff_target[i]) { valid = false; break; }
    if (hash[i] < diff_target[i]) { valid = true;  break; }
    // equal so far: keep going with the next less significant byte
  }
  return valid;
}

and in calculateMiningData():

size_t size_target = to_byte_array(target, 64, mMiner.bytearray_target);  // was 32
for (size_t j = 0; j < size_target / 2; j++)                              // was < 8

Note the reverse_bytes(diff_target, 32) line is dropped, per the byte-order
reasoning above.

How likely is this to bite in practice

checkValid() only runs for a share with 32 leading zero bits that is actually
submitted, so on a pool handing out a high share difficulty it can stay dormant for
a long time: mine never hit it. Worth noting tx_mining_submit() is called
before checkValid(), so a found block still reaches the pool; what breaks is the
isValid flag (wrong in every case, since it compares against stack garbage) and
the freeze when it does trigger.

Branch with the fix and the host test, based on current main:
https://github.com/Gheop/NerdMiner_v2/tree/fix/checkvalid-797

It touches src/utils.cpp only (+17 / -11) and adds test/checkvalid_test.cpp,
which builds with plain g++ and needs no hardware:

g++ -O0 -o t test/checkvalid_test.cpp && ./t

Running the fix on 6 miners since today, ~1.8 MH/s total, no regression and no SHA
mismatches. Happy to turn the branch into a PR if that's useful.