[UTXO-BUG] LOW: Duplicate input dedup, missing validations, and test coverage gaps

[ArokyaMatthew]

Vulnerability Class

Low — Edge cases, code quality, test gaps (25 RTC each)

Findings

LOW-2: Duplicate Input Box IDs Not Rejected

The same box_id can appear twice in inputs. The validation loop counts input_total += value for each occurrence, so a 100 RTC box counted twice gives input_total = 200. The spend-phase rowcount check catches it accidentally, but there's no explicit dedup.

Fix: Added explicit dedup before the validation loop:

input_box_ids = [i['box_id'] for i in inputs]
if len(input_box_ids) != len(set(input_box_ids)):
    return False

LOW-3: No Spending Proof Verification in UTXO Layer
The spending_proof field in inputs is completely ignored by apply_transaction(). Signature verification only happens at the endpoint layer (utxo_endpoints.py). Any internal code path calling apply_transaction() directly can spend any UTXO without authorization.

Fix: Documented with a test. This is by design for Phase 1, but a footgun for Phase 2+.

LOW-4: Test Coverage Gaps
Added tests for previously uncovered scenarios:

Duplicate input rejection
Self-transfer (from == to)
Spending proof non-verification (behavior documentation)
Tests Added
test_duplicate_input_rejected
test_self_transfer
test_spending_proof_not_verified_in_utxo_layer
All 37 tests pass.

Files Changed
node/utxo_db.py — 8 lines added (dedup check)
node/test_utxo_db.py — 3 test cases added
Ref: Bounty #2819

MY WALLET IS aroky-x86-miner

[github-actions]

Welcome to RustChain! Thanks for your first pull request.

Before we review, please make sure:

• Your PR has a BCOS-L1 or BCOS-L2 label
• New code files include an SPDX license header
• You've tested your changes against the live node

Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150)

A maintainer will review your PR soon. Thanks for contributing!

[zhuzhushiwojia]

Good catch on LOW-2 and LOW-3! A few observations:

LOW-2 (Duplicate Input):
The fix looks correct - using a set to detect duplicates is efficient. However, consider adding a specific error message that mentions which box_ids are duplicated.

LOW-3 (Spending Proof):
This is a significant security concern for Phase 2+. The current design relies on endpoint-level validation, but internal code paths could bypass this. Consider adding a warning comment in apply_transaction() or creating a decorator for functions that require proof verification.

Test Coverage:
The new tests look solid. Good edge case coverage.

Overall: Good work finding these edge cases!

[zhuzhushiwojia]

Code Review - PR #2073

Positive Observations

1. Good defense in depth - Adding explicit duplicate input detection before the validation loop is a solid practice. Even though the rowcount check catches this today, relying on accidental protection is fragile.

2. Clear comments - The code includes helpful comments explaining WHY the check exists. This helps future maintainers understand the security rationale.

3. Comprehensive test coverage - Adding tests for test_duplicate_input_rejected, test_self_transfer, and test_spending_proof_not_verified_in_utxo_layer ensures these edge cases are covered.

Suggestions / Questions

1. Error handling: Consider if the error message could be more specific (e.g., "Duplicate input box_id detected").

2. LOW-3 (Spending proof not verified): The PR documents this as "by design for Phase 1", but it might be worth adding a warning docstring for internal callers - this is a potential security footgun.

3. Test coverage: Consider adding a test for the case where duplicate inputs have different values.

---

Overall: LGTM ✅ - The changes improve security and add valuable test coverage.

[zhuzhushiwojia]

This is a thorough audit! The test cases and validation improvements will strengthen the codebase.

[geldbert]

Thanks for the PR! Here are my observations from reviewing the UTXO validation improvements:

Observation 1: Test Documentation Quality

test_spending_proof_not_verified_in_utxo_layer (lines 371-388) has excellent documentation intent, but the test name is slightly misleading. The name suggests it's testing that proofs "not verified" - but actually the test verifies that the UTXO layer accepts any spending_proof without validation. Consider renaming to test_spending_proof_accepted_without_verification_utxo_layer to clarify the actual behavior being asserted.

Observation 2: Duplicate Input Edge Case

The duplicate input detection (lines 319-323) correctly handles the direct case where box_id appears twice, but doesn't guard against inputs that are semantically equivalent but structurally different. For example, if ordering or whitespace in spending_proof differs:

# These could theoretically hash differently but spend same box
{'box_id': 'abc123', 'spending_proof': 'sig'}
{'box_id': 'abc123', 'spending_proof': ' sig'}  # note space

This is probably an acceptable trade-off (dedup by exact dict equality), but worth documenting the assumption that box_id is the canonical unique identifier for duplicate detection.

Observation 3: Positive Finding - Self-Transfer Test Coverage

The test_self_transfer addition (lines 355-370) fills an actual gap. Prior to this PR, there was no explicit test that UTXO logic correctly handles self-transfers where from == to. The balance assertion at line 368 (assertEqual(self.db.get_balance('alice'), 100 * UNIT)) is the critical check that prevents double-counting in this edge case.

[ArokyaMatthew]

Thanks for the reviews @zhuzhushiwojia @geldbert!

RE: Error message for duplicate inputs — Agreed, will add in a follow-up if the maintainer prefers a more descriptive error path (currently returns False silently like all other validation failures in apply_transaction()).

RE: Spending proof naming — @geldbert good point. The test name documents the risk ("not verified") rather than the behavior ("accepted without verification"). Either reads correctly but yours is more precise.

RE: Duplicate inputs with different spending_proof — The dedup is intentionally keyed on box_id alone, since box_id is the primary key of the UTXO being consumed. Two inputs with the same box_id but different spending_proof are still trying to spend the same UTXO — the proof content is irrelevant to the dedup logic.

Appreciate the thorough reviews!

[ArokyaMatthew]

Addressed feedback in the latest commit:

• Added .. warning:: docstring in apply_transaction() about spending proof non-verification (@zhuzhushiwojia)
• Renamed test to test_spending_proof_accepted_without_verification (@geldbert)
• Documented that dedup is keyed on box_id alone, independent of spending_proof content (@geldbert)
  Thanks for the reviews!

[ArokyaMatthew]

@Scottcjn , please review

[Scottcjn]

✅ Merged. Good defense-in-depth — the duplicate input dedup catches an input_total inflation vector that the spend-phase rowcount only catches accidentally. The spending_proof documentation is also valuable for future contributors.

25 RTC paid.

Wallet: ArokyaMatthew | Reason: bounty #2819 LOW duplicate input dedup + validation docs