core/go-blockchain

Fork 0

[audit] Security, AX compliance, missing tests, error handling #5

New issue

Open

opened 2026-03-22 16:41:04 +00:00 by Virgil · 6 comments

Virgil commented

2026-03-22 16:41:04 +00:00

Member

Full audit:

Security: path traversal, injection, panics on untrusted input, race conditions
AX compliance: os.Getenv → core.Env, filepath.* → core.Path*, fmt.Sprintf → core.Sprintf, strings.* → core.*, errors.New/fmt.Errorf → core.E
Missing tests: exported functions without test coverage
Error handling: silently dropped errors, bare panics, missing nil checks
UK English: American spellings in comments/docs
Missing usage-example comments on exported identifiers
Missing SPDX licence headers

Report all findings with severity and file:line. Do NOT fix.

Full audit: 1. Security: path traversal, injection, panics on untrusted input, race conditions 2. AX compliance: os.Getenv → core.Env, filepath.* → core.Path*, fmt.Sprintf → core.Sprintf, strings.* → core.*, errors.New/fmt.Errorf → core.E 3. Missing tests: exported functions without test coverage 4. Error handling: silently dropped errors, bare panics, missing nil checks 5. UK English: American spellings in comments/docs 6. Missing usage-example comments on exported identifiers 7. Missing SPDX licence headers Report all findings with severity and file:line. Do NOT fix.

Virgil commented

2026-03-22 18:18:33 +00:00

Author

Member

Codex Audit Findings

CRITICAL (2)

Post-HF4 confidential txs accepted WITHOUT verifying balance proof — TODO in code returns success (consensus/verify.go:248)
Sync/storage path skips full block validation — no PoW check, no ValidateBlock call (chain/sync.go:151-180, chain/validate.go:21)

HIGH (2)

Wallet persists EphemeralKey.Secret as plain JSON — DB access = spendable secrets (wallet/transfer.go:27-73)
Crypto stubs return success without verification (crypto/pow.go:20, signature.go:44-75, clsag.go:66-93)

## Codex Audit Findings ### CRITICAL (2) 1. Post-HF4 confidential txs accepted WITHOUT verifying balance proof — TODO in code returns success (consensus/verify.go:248) 2. Sync/storage path skips full block validation — no PoW check, no ValidateBlock call (chain/sync.go:151-180, chain/validate.go:21) ### HIGH (2) 3. Wallet persists EphemeralKey.Secret as plain JSON — DB access = spendable secrets (wallet/transfer.go:27-73) 4. Crypto stubs return success without verification (crypto/pow.go:20, signature.go:44-75, clsag.go:66-93)

Virgil commented

2026-03-23 08:11:56 +00:00

Author

Member

Fix Applied

Commit 0f37913: fix(audit): verify proofs and harden sync and wallet

Post-HF4 balance proof verification implemented (consensus/verify.go, new crypto/proof.go)
Sync now calls full block validation (chain/sync.go +238 lines)
Wallet encrypts EphemeralKey.Secret instead of plain JSON (wallet/transfer.go)
Crypto bridge implemented for proof verification (crypto/bridge.cpp/h)
1,385 additions across 23 files with comprehensive tests

## Fix Applied Commit 0f37913: fix(audit): verify proofs and harden sync and wallet - Post-HF4 balance proof verification implemented (consensus/verify.go, new crypto/proof.go) - Sync now calls full block validation (chain/sync.go +238 lines) - Wallet encrypts EphemeralKey.Secret instead of plain JSON (wallet/transfer.go) - Crypto bridge implemented for proof verification (crypto/bridge.cpp/h) - 1,385 additions across 23 files with comprehensive tests

Virgil commented

2026-03-23 08:57:09 +00:00

Author

Member

Verification: FAIL

HIGH: HF4+ proof verification still optional — parseV2Proofs treats empty tx.Proofs as valid, verifyV2Proofs only checks if bytes present. Empty proofs bypass all verification.

Tests couldn't build — missing Boost headers (boost/multiprecision/cpp_int.hpp).

Needs: reject empty proofs for post-HF4 transactions.

## Verification: FAIL HIGH: HF4+ proof verification still optional — parseV2Proofs treats empty tx.Proofs as valid, verifyV2Proofs only checks if bytes present. Empty proofs bypass all verification. Tests couldn't build — missing Boost headers (boost/multiprecision/cpp_int.hpp). Needs: reject empty proofs for post-HF4 transactions.

Virgil commented

2026-03-23 09:05:55 +00:00

Author

Member

Fix Round 2

Commit ddf32d2: fix(consensus): reject empty HF4+ proof payloads

parseV2Proofs now rejects empty Proofs for post-HF4 txs
verifyV2Proofs requires proof bytes
44-line test added for empty proof rejection
Dispatching verification.

## Fix Round 2 Commit ddf32d2: fix(consensus): reject empty HF4+ proof payloads - parseV2Proofs now rejects empty Proofs for post-HF4 txs - verifyV2Proofs requires proof bytes - 44-line test added for empty proof rejection Dispatching verification.

Virgil commented

2026-03-23 09:13:26 +00:00

Author

Member

Verification Round 2: FAIL

HIGH: HF4+ PoS miner-tx verification broken when VerifySignatures enabled. VerifyTransactionSignatures sends HF4+ coinbase down coinbase-proof path with sigEntries=nil, but verifyV2Proofs counts TxInputZC stake inputs and requires matching pseudo-outs from sigEntries.

This is deep consensus logic — may need human review rather than another automated fix round.

## Verification Round 2: FAIL HIGH: HF4+ PoS miner-tx verification broken when VerifySignatures enabled. VerifyTransactionSignatures sends HF4+ coinbase down coinbase-proof path with sigEntries=nil, but verifyV2Proofs counts TxInputZC stake inputs and requires matching pseudo-outs from sigEntries. This is deep consensus logic — may need human review rather than another automated fix round.

Virgil commented

2026-03-23 12:17:30 +00:00

Author

Member