Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion contracts/src/crypto/ElGamal.compact
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,13 @@ pragma language_version >= 0.23.0;
* scalar. Feeding a raw `persistentHash` output into `ecMulGenerator` would
* occasionally exceed the Jubjub scalar field order and fault at runtime.
*
* @dev Domain separation. `persistentHash<Vector<N, Bytes<32>>>` is NOT
* domain-separating on its own. Two circuits hashing the same arity and
* element type share one hash domain, and the hash cannot tell which slot was
* "meant" as a tag. `secretToScalar` therefore prefixes a fixed constant, and
* that constant is placed FIRST so no call to `expandRandomness` (whose two
* slots are both caller-supplied) can reproduce the tuple by ordinary use.
*
* @dev TRUST ASSUMPTION — subgroup membership. This is the module's most
* load-bearing assumption. Every `JubjubPoint` reaching a curve operation is
* assumed to be in the Jubjub prime-order subgroup, including points supplied by
Expand Down Expand Up @@ -102,11 +109,34 @@ module ElGamal {
* @description Maps a 32-byte secret to a valid Jubjub scalar. See the
* module-level hash-to-scalar note for why `degradeToTransient` is required.
*
* @dev Domain separation here is load-bearing, not cosmetic. This output is
* the private key protecting every ciphertext held under an account. Public

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrew-fleming will that be covered here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, out of scope. Filed as #854.

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

* account identifiers elsewhere in the library are derived as
* `persistentHash([secretKey])` (`Utils.computeAccountId`), and that
Comment on lines +113 to +115

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "account identifiers elsewhere in the library" reads library-wide, but only Utils.computeAccountId has this shape. ShieldedAccessControl.computeAccountId hashes [secretKey, instanceSalt, pad(32, "ShieldedAccessControl:accountId")], so it is already tagged and salted and is not at risk here. Scoping the sentence to accounts derived via Utils.computeAccountId makes the threat precise.

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

* identifier is stored in the clear as a ledger map key. Untagged, this
* circuit would compute that same hash, so a secret used in both roles would
* have its encryption key recoverable from a published identifier by applying
* `degradeToTransient`. The tag makes the two derivations unrelated, so a
* wallet MAY safely derive its account secret and its encryption secret from
* common key material.
*
* @dev The tag is FIRST, deliberately. `expandRandomness` hashes
* `[seed, tag]` with both slots caller-supplied, so tagging this circuit as
* `[secret, tag]` would make `secretToScalar(x)` identical to
* `expandRandomness(x, <this tag>)` reachable by passing this domain string
* to the parameter that exists to receive domain strings. Leading with the
* tag means reproducing this output through `expandRandomness` would require
* passing the domain constant as the *seed*. Do not normalise the orderings.
*
* @param secret - The 32-byte secret to map.
* @return A Field guaranteed to be a valid Jubjub scalar.
*/
export pure circuit secretToScalar(secret: Bytes<32>): Field {
return degradeToTransient(persistentHash<Vector<1, Bytes<32>>>([secret]));
return degradeToTransient(
persistentHash<Vector<2, Bytes<32>>>(
[pad(32, "ElGamal:secretToScalar"), secret]
Comment thread
0xisk marked this conversation as resolved.
)
);
}

/**
Expand Down
42 changes: 41 additions & 1 deletion contracts/src/crypto/test/ElGamal.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import type { JubjubPoint } from '@midnight-ntwrk/compact-runtime';
import {
CompactTypeBytes,
CompactTypeVector,
convertBytesToField,
type JubjubPoint,
persistentHash,
} from '@midnight-ntwrk/compact-runtime';
import { beforeAll, describe, expect, it } from 'vitest';
import {
type Ciphertext,
Expand All @@ -23,6 +29,9 @@ const b32 = (label: string): Uint8Array => {
const EK_A = b32('elgamal-ek-A');
const EK_B = b32('elgamal-ek-B');

// Mirrors the `pad(32, ...)` tag `secretToScalar` prefixes its input with.
const SECRET_TO_SCALAR_TAG = b32('ElGamal:secretToScalar');

// Explicit encryption randomness. Any value below the Jubjub scalar field
// order (~2^252) is a valid scalar; these small constants keep the tests
// deterministic and let us assert that distinct randomness yields distinct
Expand Down Expand Up @@ -61,6 +70,37 @@ describe('ElGamal', () => {
it('returns a positive scalar', async () => {
expect(await contract.secretToScalar(EK_A)).toBeGreaterThan(0n);
});

// Guards the domain separation on `secretToScalar` (rationale in its @dev
// notes). Account identifiers are the same hash, untagged, and public.
it('is not recoverable from a published account identifier', async () => {
// What any observer can do: take the identifier and apply the circuit's
// own truncation.
const accountId = persistentHash(
new CompactTypeVector(1, new CompactTypeBytes(32)),
[EK_A],
);

expect(await contract.secretToScalar(EK_A)).not.toBe(
convertBytesToField(31, accountId, 'attacker'),
);
});

Comment thread
0xisk marked this conversation as resolved.
// Both pin the tag's POSITION, and flip if the order becomes `[secret, tag]`
it('is not reproducible by passing the domain tag as expandRandomness tag', async () => {
expect(
await contract.expandRandomness(EK_A, SECRET_TO_SCALAR_TAG),
).not.toBe(await contract.secretToScalar(EK_A));
});

// An accepted residual, not a guarantee: `expandRandomness` is itself
// untagged, so its seed slot can carry this tag. Removable by tagging
// `expandRandomness` (arity 3, tag first)
it('can be reproduced via expandRandomness with the tag in the seed slot', async () => {
expect(await contract.expandRandomness(SECRET_TO_SCALAR_TAG, EK_A)).toBe(
await contract.secretToScalar(EK_A),
);
});
});

// -------------------------------------------------------------------------
Expand Down
39 changes: 27 additions & 12 deletions contracts/src/token/ConfidentialFungibleToken.compact
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,17 @@ pragma language_version >= 0.23.0;
* follows the same pattern as `FungibleToken`: a witness-derived
* `accountId = persistentHash(secretKey)`. Encryption uses a separate witness
* `wit_ConfidentialTokenEK` to derive an ElGamal keypair `(ek, pk)` via
* `ElGamal_derivePk`, where `pk = g^degradeToTransient(persistentHash(EK))`.
* `ElGamal_derivePk`, where
* `pk = g^degradeToTransient(persistentHash([ElGamal domain tag, EK]))`.
*
* @notice Sharing key material between the two witnesses is PERMITTED. The
* account identifier is an untagged hash of the SK and is published as a ledger
* map key; the encryption scalar is a DOMAIN-SEPARATED hash of the EK. Because
* the two derivations are tagged differently, a wallet may return the same
* value from `wit_ConfidentialTokenSK` and `wit_ConfidentialTokenEK`, or derive
* both from one master secret, without the published identifier revealing the
* encryption key. The domain separation is what makes that safe. Do not remove
* it. See `ElGamal.secretToScalar`.
*
* @notice Supply. The mint/burn building blocks (`_mint`/`_burn`/`_burnFrom`)
* live here; total-supply tracking is an optional add-on
Expand Down Expand Up @@ -223,6 +233,7 @@ module ConfidentialFungibleToken {
import CompactStandardLibrary;
import "../crypto/ElGamal" prefix ElGamal_;
import "../crypto/EcdhMask" prefix EcdhMask_;
import "../utils/Utils" prefix Utils_;

// ---------------------------------------------------------------------------
// Types
Expand Down Expand Up @@ -491,7 +502,7 @@ module ConfidentialFungibleToken {
* `wit_ConfidentialTokenEK` and initializes their balance to an encryption of
* zero. Registration is a prerequisite for sending or receiving.
*
* @circuitInfo k=13, rows=6203
* @circuitInfo k=13, rows=8100
*
* @notice Returns the caller's `accountId`. A composing contract can gate on
* the returned value (e.g. assert it is KYC-approved) to restrict who may
Expand Down Expand Up @@ -716,7 +727,7 @@ module ConfidentialFungibleToken {
* `extensions/ConfidentialFungibleTokenPublicSupply`) if the deployment tracks
* `totalSupply`.
*
* @circuitInfo k=14, rows=14673
* @circuitInfo k=15, rows=16570
*
* Requirements:
*
Expand All @@ -740,7 +751,7 @@ module ConfidentialFungibleToken {
* credits `to`, and pushes an encrypted memo to `to`. The amount is hidden;
* the (sender, recipient) pair is public.
*
* @circuitInfo k=16, rows=41617
* @circuitInfo k=16, rows=43514
*
* @notice Returns the caller's (sender's) `accountId` for caller-side gating.
* The sender is derived from the same authentication `_debit` performs, so the
Expand Down Expand Up @@ -778,7 +789,7 @@ module ConfidentialFungibleToken {
* operations (`mint`/`burn`) live in a separate layer outside this conserving
* surface, and only they may break conservation.
*
* @circuitInfo k=16, rows=41606
* @circuitInfo k=16, rows=43503
*
* @notice Like `transfer`, the credit lands in the recipient's pending pool
* (see the dual-balance note); the caller sweeps their own incoming value.
Expand Down Expand Up @@ -852,7 +863,7 @@ module ConfidentialFungibleToken {
* refunds any prior escrow, so allowances replace rather than stack. The cap
* is not public.
*
* @circuitInfo k=16, rows=43067
* @circuitInfo k=16, rows=44964
*
* @notice Returns the caller's (approver's) `accountId` for caller-side gating.
*
Expand Down Expand Up @@ -936,7 +947,7 @@ module ConfidentialFungibleToken {
* caller via `approve`) and credits it to `to`, reducing both escrow copies by
* `value`. The amount is hidden.
*
* @circuitInfo k=16, rows=63207
* @circuitInfo k=16, rows=65104
*
* @notice Returns the caller's (spender's) `accountId` for caller-side gating.
*
Expand Down Expand Up @@ -1027,15 +1038,19 @@ module ConfidentialFungibleToken {
}

/**
* @description Derives the public `accountId` from a secret key, as
* `persistentHash(sk)`. Pure, so a wallet can compute its own account
* identifier off-chain (no proof) before transacting.
* @description Derives the public `accountId` from a secret key. Pure, so a
* wallet can compute its own account identifier off-chain (no proof) before
* transacting.
*
* @dev Delegates to `Utils.computeAccountId` rather than reimplementing the
* hash. The identifier is deliberately GLOBAL. The same key material yields
* the same id in every module that derives it.
*
* @param {Bytes<32>} sk - The account secret key.
* @return {Bytes<32>} - The derived accountId.
*/
export pure circuit computeAccountId(sk: Bytes<32>): Bytes<32> {
return persistentHash<Vector<1, Bytes<32>>>([sk]);
return Utils_computeAccountId(sk);
}

/**
Expand Down Expand Up @@ -1119,7 +1134,7 @@ module ConfidentialFungibleToken {
* recipient credit. The building block for a composing contract's
* `burnFrom`; it performs no supply accounting (see `_burn`).
*
* @circuitInfo k=16, rows=36274
* @circuitInfo k=16, rows=38171
*
* Requirements:
*
Expand Down
87 changes: 84 additions & 3 deletions contracts/src/token/test/ConfidentialFungibleToken.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,20 +27,29 @@ const padTag = (s: string): Uint8Array => {
// Helpers
// ---------------------------------------------------------------------------

/** The domain-separation tag `ElGamal.secretToScalar` prefixes its input with. */
const SECRET_TO_SCALAR_TAG = padTag('ElGamal:secretToScalar');

/**
* @description Derives the expected pk for a given EK, mirroring the
* in-circuit `_derivePk`:
* pk = ecMulGenerator(degradeToTransient(persistentHash([ek])))
* pk = ecMulGenerator(degradeToTransient(persistentHash([TAG, ek])))
*
* The `convertBytesToField` call mirrors `degradeToTransient`, producing the
* field element that `ecMulGenerator` expects.
*
* @note The tag is what keeps this scalar unrelated to the account identifier,
* which is `persistentHash([sk])` (untagged, and public as a ledger map key).
* Without it, a secret used in both roles would have its encryption key
* recoverable from the published identifier. See `buildAccountIdHash` below —
* that one is deliberately untagged and must stay so.
*
* @note The field-element derivation from EK uses 31 bytes of the hash output
* (empirically determined); the effective collision resistance is therefore 248 bits.
*/
const derivePk = (ek: Uint8Array) => {
const rt_type = new CompactTypeVector(1, new CompactTypeBytes(32));
const ekHash = persistentHash(rt_type, [ek]);
const rt_type = new CompactTypeVector(2, new CompactTypeBytes(32));
Comment thread
0xisk marked this conversation as resolved.
const ekHash = persistentHash(rt_type, [SECRET_TO_SCALAR_TAG, ek]);
Comment thread
0xisk marked this conversation as resolved.
const ekField = convertBytesToField(31, ekHash, 'derivePk');
return ecMulGenerator(ekField);
};
Expand Down Expand Up @@ -154,6 +163,22 @@ describe.skipIf(isLiveBackend())(
expect(storedPk).toEqual(expectedPk);
});

it('keeps the encryption scalar unrelated to the public accountId when SK and EK are the same secret', async () => {
Comment thread
0xisk marked this conversation as resolved.
await cft.privateState.switchIdentity(ALICE.secretKey, ALICE.secretKey);
await cft.register();

const ledger = await cft.getPublicState();
const storedPk = ledger.CFT__encryptionKeys.lookup(ALICE.accountId);

// What any observer can derive from the published accountId.
expect(storedPk).not.toEqual(
ecMulGenerator(
convertBytesToField(31, ALICE.accountId, 'publicAccountId'),
),
);
expect(storedPk).toEqual(derivePk(ALICE.secretKey));
});

it('should store distinct pks for distinct EKs', async () => {
await cft.privateState.switchIdentity(
ALICE.secretKey,
Expand Down Expand Up @@ -279,6 +304,62 @@ describe.skipIf(isLiveBackend())(
// building blocks, so this suite never touches the composed mint/burn/totalSupply.
// ---------------------------------------------------------------------------

describe.skipIf(isLiveBackend())(
'ConfidentialFungibleToken: shared SK/EK through the value path',
() => {
beforeEach(async () => {
cft = await ConfidentialFungibleTokenSimulator.create(
NAME,
SYMBOL,
DECIMALS,
);
});

// Every party uses ONE secret for both witnesses.
const shared = (u: typeof ALICE) =>
cft.privateState.switchIdentity(u.secretKey, u.secretKey);

const decryptsTo = (ct: any, u: typeof ALICE, value: bigint) =>
elgamal.assertDecryptsTo(
ct,
elgamal.derivePk(u.secretKey),
u.secretKey,
value,
);

it('mints, transfers and sweeps with one secret in both roles', async () => {
for (const u of [ALICE, BOB]) {
await shared(u);
await cft.register();
}

await shared(ALICE);
await cft._mint(ALICE.accountId, 100n);
await cft.sweep();
await cft.privateState.cachePlaintext(
await cft.balanceOf(ALICE.accountId),
100n,
);

// `_debit` re-derives Alice's pk from the shared secret and asserts her
// balance decrypts to the claimed 100.
await cft.transfer(BOB.accountId, 40n);

await shared(BOB);
await cft.sweep();

const aliceBalance = await cft.balanceOf(ALICE.accountId);
const bobBalance = await cft.balanceOf(BOB.accountId);

expect(() => decryptsTo(aliceBalance, ALICE, 60n)).not.toThrow();
expect(() => decryptsTo(bobBalance, BOB, 40n)).not.toThrow();

// Confirm the binding is real
expect(() => decryptsTo(aliceBalance, ALICE, 61n)).toThrow();
});
},
);

describe.skipIf(isLiveBackend())('ConfidentialFungibleToken: transfer', () => {
beforeEach(async () => {
cft = await ConfidentialFungibleTokenSimulator.create(
Expand Down
10 changes: 10 additions & 0 deletions contracts/src/utils/Utils.compact
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,16 @@ module Utils {
* ## ID Derivation
* `accountId = persistentHash(secretKey)`
*
* @dev The absence of a domain-separation tag is DELIBERATE. This identifier
* is global by design: the same key material yields the same identity in
* every module deriving one, so a user who wishes to carry one identity
* across modules can. Modules wanting a per-deployment, unlinkable identity
* should not use this circuit.
*
* @dev NOT FOR PRIVATE DERIVATION. This returns an identifier, not a secret,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* and the construction is untagged. A circuit deriving a PRIVATE value from a
* secret MUST use its own domain-separation tag.
*
* @param {Bytes<32>} secretKey - A 32-byte cryptographically secure random value.
*
* @returns {Bytes<32>} accountId - The computed account identifier.
Expand Down