Add domain to secretToScalar

[andrew-fleming]

Fixes #792

Summary by CodeRabbit

• Security Enhancements

  • Added domain separation to ElGamal secret-to-scalar and encryption-key derivation.
  • Prevented account identifiers from being directly reused to derive encryption scalars.
  • Centralized account-ID derivation for consistent behavior across modules.

• Documentation

  • Clarified hashing boundaries and safe reuse of secret material.

• Tests

  • Added coverage for domain-separation tags, argument ordering, and collision prevention.

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fc58b121-143c-4e0c-9909-c031522d3b08

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6aa9098f-ab6a-4c66-b669-89e4a6ed1157

📥 Commits

Reviewing files that changed from the base of the PR and between 677f352 and 7544633.

📒 Files selected for processing (5)

• contracts/src/crypto/ElGamal.compact
• contracts/src/crypto/test/ElGamal.test.ts
• contracts/src/token/ConfidentialFungibleToken.compact
• contracts/src/token/test/ConfidentialFungibleToken.test.ts
• contracts/src/utils/Utils.compact

Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.

---

Walkthrough

The change adds domain separation to ElGamal scalar and encryption-key derivation. Account-ID derivation remains an untagged, globally consistent hash delegated through Utils. Tests verify separation and tag ordering.

Changes

Domain-separated key derivation

Layer / File(s)	Summary
Domain-separated scalar derivation contracts/src/crypto/ElGamal.compact, contracts/src/crypto/test/ElGamal.test.ts	secretToScalar hashes a fixed tag and secret as a two-element vector before reduction. Tests verify separation from account identifiers and the tag position.
Account-ID and encryption-key integration contracts/src/token/ConfidentialFungibleToken.compact, contracts/src/token/test/ConfidentialFungibleToken.test.ts, contracts/src/utils/Utils.compact	The token module delegates computeAccountId to Utils. Encryption-key derivation uses the ElGamal domain tag. Documentation records the account-ID hashing contract.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 75446

The change is merge-ready after normal checks and review; no actionable merge-blocking risk remains.

Suggested reviewers: 0xisk

Poem

I’m a rabbit with a tagged little key,
Hashing secrets where domains must be.
Account IDs keep their global trail,
While private paths no longer sail
On the same hash without a veil.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes address issue #792 by domain-separating secretToScalar from account-ID hashing and adding supporting tests and documentation.
Out of Scope Changes check	✅ Passed	The implementation, utility reuse, documentation, tests, and formatting changes directly support the linked issue and stated objectives.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. (3 skipped: 3 unsupported.)
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely identifies the main change: adding domain separation to secretToScalar.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[0xisk]

2. Untagged computeAccountId is a deviation from the audit recommendation

❔ question: H-01 asked for tags on both derivations; this keeps the id untagged and guards the shared domain with a doc rule a future module can miss. Global identity needs one shared derivation, untagged is not required: tagging it (e.g. OZ:accountId:v1) keeps cross-module identity, satisfies the recommendation fully, and it's still alpha. Deliberate, or worth tagging now?

added by claude (dev3-midnight-basic-review)

[andrew-fleming]

Deliberate. A global id tag is a convention and not a security issue when we include a tag in secretToScalar. Changing the convention deserves its own discussion

[0xisk]

3. Tag position now differs across the library

🔵 followup: this mandates tag-first, while ShieldedAccessControl.compact:744 hashes [value, pad(32, "ShieldedAccessControl:nullifier")] tag-last.

Write the rule down once (Utils or a crypto-conventions note) so a module copying SAC's shape for a private derivation does not recreate H-01.

added by claude (dev3-midnight-basic-review)

[0xisk]

@andrew-fleming will that be covered here?

[andrew-fleming]

Out of scope, let's keep the PR focused on addressing the fix. The rule for this implementation is already documented