Skip to content

reject zero-value and reused HTLC secrets - #245

Merged
karagozemin merged 5 commits into
karagozemin:masterfrom
Horlarmmy:reject-zero-and-reused-htlc-secrets
Sep 1, 2026
Merged

reject zero-value and reused HTLC secrets#245
karagozemin merged 5 commits into
karagozemin:masterfrom
Horlarmmy:reject-zero-and-reused-htlc-secrets

Conversation

@Horlarmmy

Copy link
Copy Markdown
Contributor

What & Why

Prevents invalid or previously observed HTLC secrets from entering the swap flow. All-zero preimages/hashlocks and reused preimages are now rejected at the SDK, coordinator service, and API route layers. This closes a cross-chain safety gap where a malicious or buggy resolver could replay a preimage across orders, or submit a degenerate zero secret.

Closes #242

Touched surface

  • packages/sdk/ — shared TypeScript SDK
  • coordinator/ — order book + REST/WS service
  • frontend/ — React + Vite bridge UI
  • resolver/ — community resolver runner
  • relayer/ — legacy v1 listener / watchdog
  • contracts/ — Solidity v2 (HTLCEscrow, ResolverRegistry)
  • soroban/ — Stellar Soroban contracts
  • docs/ — documentation only
  • CI / config

Settlement & refund semantics

  • Bridge settlement semantics changed
  • Refund semantics changed
  • Settlement-critical invariant changed
  • None of the above — this PR cannot move, hold, or release user funds

Tests run

  • pnpm --filter @oversync/sdk build && pnpm --filter @oversync/sdk exec tsc --noEmit
  • pnpm --filter @oversync/sdk test
  • pnpm --filter @oversync/coordinator exec tsc --noEmit && pnpm --filter @oversync/coordinator test
  • pnpm --filter @oversync/resolver exec tsc --noEmit && pnpm --filter @oversync/resolver test
  • pnpm --filter @oversync/frontend exec tsc --noEmit && pnpm --filter @oversync/frontend test
  • pnpm --filter @oversync/contracts compile && ...
  • cd soroban && stellar contract build && cargo test --release
  • (cd contracts && forge test ...)
  • pnpm test:e2e
  • node scripts/verify-addresses.mjs
  • node scripts/check-evidence-links.mjs

SDK
pnpm --filter @oversync/sdk test
10 files passed (10)
128 tests passed (128)
Coordinator
pnpm --filter @oversync/coordinator exec tsc --noEmit
(clean)
pnpm --filter @oversync/coordinator test
7 passed | 7 failed (14)
86 tests passed (86)
The 7 failing suites all error with "No such built-in module: node:sqlite"
on Node 22. This is a pre-existing infra issue (needs Node 23+).

UI / evidence artefacts

n/a

Secrets, logging, and PII risk

  • No secrets, private keys, RPC credentials, .env content, wallet mnemonics, or preimages added to the repo
  • No new console.* / logger.* line that prints secrets, preimages, signed payloads, or PII
  • No new Vite/build flag that exposes devtools output in production
  • None of the above

Public proof links (SCF / investor evidence)

n/a

Breaking change & rollback

  • Breaking change? No — callers submitting valid 32-byte non-zero hex preimages are unaffected. Only all-zero and reused preimages are now rejected.
  • Migration or feature flag required? No

Reviewer checklist

@vercel

vercel Bot commented Aug 31, 2026

Copy link
Copy Markdown

@Horlarmmy is attempting to deploy a commit to the karagoz's projects Team on Vercel.

A member of the Team first needs to authorize it.

@drips-wave

drips-wave Bot commented Aug 31, 2026

Copy link
Copy Markdown

@Horlarmmy Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@karagozemin

Copy link
Copy Markdown
Owner

Sıfır değer kontrolleri doğru yönde ve ek testler geçiyor; fakat reuse kontrolünde önemli bir boşluk var: findByPreimage SQLiteta ham metin eşitliği kullanıyor, while reveal büyük/küçük harfli 0x hex biçimlerini kabul ediyor. Aynı 32-byte preimage farklı casing ile gönderildiğinde WHERE preimage = ? eşleşmeyip ikinci orderda tekrar kullanılabilir. Lookup ve hash karşılaştırmalarını canonical lowercase bytes üzerinden yapacak şekilde düzeltip, mevcut master üzerine rebase/merge edin; ardından yeniden inceleyeceğim.

@Horlarmmy

Copy link
Copy Markdown
Contributor Author

You're right. findByPreimage was doing raw SQLite WHERE preimage = ? while reveal() accepted any casing. Added canonical = preimage.toLowerCase() right after format validation in SecretService.reveal() so all downstream storage and queries use a single canonical form. Also moved the zero-hashlock check into OrderService.announce() directly (not just zod .refine()) so the guard works regardless of call path. New test confirms mixed-case 0xAbCd... vs 0xabcd... is now detected as reuse.

@karagozemin

Copy link
Copy Markdown
Owner

Thanks for the follow-up. The new canonical-lowercase path fixes the mixed-case preimage reuse gap, and the added casing test passes. I also verified the SDK suite (128/128) and coordinator typecheck.

I still cannot merge this revision against the current master:

  • The full coordinator suite is 141/146: 5 failures remain in test/order-transitions.test.ts and test/snapshot.test.ts because this branch carries fixtures with equal/reversed destination timelocks; update them to satisfy the current destination < source invariant. One transition-route assertion also expects the stale route behavior and gets 404 on current code.
  • Please rebase/merge current master and rerun the full coordinator suite.
  • announceSchema accepts mixed-case hashlocks and announce() stores them verbatim, while SecretService.reveal() compares the computed lowercase digest by exact string equality. Either canonicalize hashlocks before storage/lookup or compare normalized values, and add an uppercase-hashlock regression test.

Once the branch is rebased and all coordinator tests pass, I can re-review the mergeable PR.

@Horlarmmy
Horlarmmy force-pushed the reject-zero-and-reused-htlc-secrets branch from c11f042 to 68b28ad Compare August 31, 2026 17:16
@Horlarmmy

Copy link
Copy Markdown
Contributor Author

All conflicts fixed @karagozemin

@karagozemin

Copy link
Copy Markdown
Owner

Reviewed the latest head. I cannot merge this revision yet: GitHub still reports mergeable=false and mergeable_state=dirty, so the branch conflicts with current master. The PR description also reports the coordinator suite is not green (7 failures on Node 22 due missing node:sqlite), and the only current check is a Vercel authorization failure. Please rebase onto current master, resolve conflicts, run the coordinator suite in a supported Node version, and rerun the checks before requesting re-review.

…eused-htlc-secrets

# Conflicts:
#	coordinator/test/order-service.test.ts
@karagozemin
karagozemin merged commit 0910a64 into karagozemin:master Sep 1, 2026
1 check failed
@Horlarmmy

Copy link
Copy Markdown
Contributor Author

Thank you @karagozemin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[SCF high][easy] Reject zero-value and reused HTLC secrets

2 participants