-
Notifications
You must be signed in to change notification settings - Fork 29
Require ek in sweep and clearMemos #830
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,6 +42,8 @@ pragma language_version >= 0.23.0; | |
| * | ||
| * - `wit_ConfidentialTokenEK`: verified by `ElGamal_assertDecryptsTo`, | ||
| * which re-derives `pk` and asserts it equals the on-chain stored pk. | ||
| * `sweep` and `clearMemos` have no plaintext to decrypt, so they assert the | ||
| * same binding directly via `_assertOwnsEncryptionKey`. | ||
| * | ||
| * - `wit_PlaintextBalance(ct)`: verified by `ElGamal_assertDecryptsTo`, | ||
| * which asserts `Dec(ct, EK) == claimedValue`. The ciphertext is passed | ||
|
|
@@ -814,12 +816,13 @@ module ConfidentialFungibleToken { | |
| * @description Sweeps the caller's incoming `pending` pool into their | ||
| * `spendable` balance. Credits land in `pending` (see the dual-balance note); | ||
| * this `sweep` is the ONLY path from pending into spendable, and only the owner | ||
| * can invoke it (the account is derived from the caller's witness secret). So a | ||
| * third party spamming credits cannot force a victim's spendable ciphertext to | ||
| * change, which is what would otherwise invalidate the victim's in-flight spend | ||
| * proofs (a liveness grief). | ||
| * can invoke it (the caller proves BOTH witness secrets: the account secret | ||
| * identifies the account, the encryption secret proves the caller can read it). | ||
| * So a third party spamming credits cannot force a victim's spendable ciphertext | ||
| * to change, which is what would otherwise invalidate the victim's in-flight | ||
| * spend proofs (a liveness grief). | ||
| * | ||
| * @circuitInfo k=13, rows=2879 | ||
| * @circuitInfo k=13, rows=5688 | ||
| * | ||
| * @notice Purely homomorphic: adds the two ciphertexts (same key) and resets | ||
| * pending to Enc(0). No plaintext is needed; the wallet already learned the | ||
|
|
@@ -829,14 +832,14 @@ module ConfidentialFungibleToken { | |
| * | ||
| * - Contract is initialized. | ||
| * - The caller is registered. | ||
| * - The caller holds the encryption secret registered for their account. | ||
| * | ||
| * @return {Bytes<32>} - The caller's accountId. | ||
| */ | ||
| export circuit sweep(): Bytes<32> { | ||
| assertInitialized(); | ||
| const accountId = _computeAccountId(); | ||
| assert(_encryptionKeys.member(disclose(accountId)), | ||
| "ConfidentialFungibleToken: not registered"); | ||
| _assertOwnsEncryptionKey(accountId); | ||
| const newSpendable = ElGamal_add( | ||
| _balances.lookup(disclose(accountId)), | ||
| _pending.lookup(disclose(accountId))); | ||
|
|
@@ -976,7 +979,7 @@ module ConfidentialFungibleToken { | |
| * @description Clears the caller's memo list, letting a wallet that has folded | ||
| * its memos into its local balance cache prune on-chain memo storage. | ||
| * | ||
| * @circuitInfo k=13, rows=2316 | ||
| * @circuitInfo k=13, rows=5127 | ||
| * | ||
| * @warning Clearing memos is destructive and can permanently lock funds. The | ||
| * on-chain balance ciphertext is not directly decryptable (discrete-log | ||
|
|
@@ -989,12 +992,15 @@ module ConfidentialFungibleToken { | |
| * Requirements: | ||
| * | ||
| * - Contract is initialized. | ||
| * - The caller is registered. | ||
| * - The caller holds the encryption secret registered for their account. | ||
| * | ||
| * @return {[]} - Empty tuple. | ||
| */ | ||
| export circuit clearMemos(): [] { | ||
| assertInitialized(); | ||
| const accountId = _computeAccountId(); | ||
| _assertOwnsEncryptionKey(accountId); | ||
| if (_memos.member(disclose(accountId))) { | ||
| _memos.insert(disclose(accountId), default<List<EcdhMask_Ciphertext>>); | ||
| } | ||
|
|
@@ -1026,6 +1032,26 @@ module ConfidentialFungibleToken { | |
| return computeAccountId(wit_ConfidentialTokenSK()); | ||
| } | ||
|
|
||
| /** | ||
| * @description Asserts `accountId` is registered and that the caller holds the | ||
| * encryption secret registered for it, by re-deriving the public key and | ||
| * comparing it to ledger state. | ||
| * | ||
| * @notice Circuits that move or destroy value prove this implicitly, via | ||
| * `ElGamal_assertDecryptsTo` on a balance. `sweep` and `clearMemos` have no | ||
| * plaintext to decrypt, so they assert it directly: holding the account secret | ||
| * alone must not be enough to clear a memo list or merge pending value, since | ||
| * doing both leaves the holder unable to state a balance they can no longer | ||
| * learn. | ||
| */ | ||
| circuit _assertOwnsEncryptionKey(accountId: Bytes<32>): [] { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 followup: This check presumes sk and ek hold independent values; a wallet deriving ek from sk voids it (holding one means holding both). #797 accepts that premise, and the root cause is tracked in #792 (H-01). Cross-reference only, nothing to change here. added by claude (dev3-midnight-basic-review)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| assert(_encryptionKeys.member(disclose(accountId)), | ||
| "ConfidentialFungibleToken: not registered"); | ||
| assert(ElGamal_derivePk(wit_ConfidentialTokenEK()) == | ||
| _encryptionKeys.lookup(disclose(accountId)), | ||
| "ConfidentialFungibleToken: wrong encryption key"); | ||
| } | ||
|
|
||
| /** | ||
| * @description Derives the public `accountId` from a secret key, as | ||
| * `persistentHash(sk)`. Pure, so a wallet can compute its own account | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -840,6 +840,79 @@ describe.skipIf(isLiveBackend())('ConfidentialFungibleToken: memos', () => { | |
| }); | ||
| }); | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Encryption-key authority on the non-value circuits | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| describe.skipIf(isLiveBackend())( | ||
| 'ConfidentialFungibleToken: encryption-key authority', | ||
| () => { | ||
| beforeEach(async () => { | ||
| cft = await ConfidentialFungibleTokenSimulator.create( | ||
| NAME, | ||
| SYMBOL, | ||
| DECIMALS, | ||
| ); | ||
| for (const u of [ALICE, BOB]) { | ||
| await cft.privateState.switchIdentity(u.secretKey, u.encryptionKey); | ||
| await cft.register(); | ||
| } | ||
| await cft.privateState.switchIdentity( | ||
| ALICE.secretKey, | ||
| ALICE.encryptionKey, | ||
| ); | ||
| await cft._mint(ALICE.accountId, 1234n); | ||
| }); | ||
|
|
||
| // Alice's account secret with Bob's encryption secret | ||
| const asAttacker = () => | ||
| cft.privateState.switchIdentity(ALICE.secretKey, BOB.encryptionKey); | ||
|
|
||
| it('rejects clearMemos from a caller without the registered encryption key', async () => { | ||
| await asAttacker(); | ||
| await expect(cft.clearMemos()).rejects.toThrow('wrong encryption key'); | ||
| }); | ||
|
|
||
| it('rejects sweep from a caller without the registered encryption key', async () => { | ||
| await asAttacker(); | ||
| await expect(cft.sweep()).rejects.toThrow('wrong encryption key'); | ||
| }); | ||
|
|
||
| it('rejects both from an unregistered caller', async () => { | ||
| await cft.privateState.switchIdentity( | ||
| CHARLIE.secretKey, | ||
| CHARLIE.encryptionKey, | ||
| ); | ||
| await expect(cft.clearMemos()).rejects.toThrow('not registered'); | ||
| await expect(cft.sweep()).rejects.toThrow('not registered'); | ||
| }); | ||
|
|
||
| it('leaves the account spendable after a blocked prune-and-sweep', async () => { | ||
| await asAttacker(); | ||
| await expect(cft.clearMemos()).rejects.toThrow(); | ||
| await expect(cft.sweep()).rejects.toThrow(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⚪ nitpick: added by claude (dev3-midnight-basic-review)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. whoops, fixed b861c32 |
||
|
|
||
| // Alice still has the memo, so she can still learn the credit and spend. | ||
| await cft.privateState.switchIdentity( | ||
| ALICE.secretKey, | ||
| ALICE.encryptionKey, | ||
| ); | ||
| expect( | ||
| (await cft.getPublicState()).CFT__memos.lookup( | ||
| ALICE.accountId, | ||
| ).length(), | ||
| ).toBe(1n); | ||
|
|
||
| await cft.sweep(); | ||
| await cft.privateState.cachePlaintext( | ||
| await cft.balanceOf(ALICE.accountId), | ||
| 1234n, | ||
| ); | ||
| await cft._burn(1234n); | ||
| }); | ||
| }, | ||
| ); | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Dual-balance grief fix (spendable vs pending; owner-only sweep) | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❔ question:
clearMemosis now caller-authenticating (proves both secrets) but still returns[]and is absent from the module header's caller-identity-gating list, so a wrapper cannot gate it. That is #804 (L-06) — is it planned for this release branch, since it changes an exported signature?added by claude (dev3-midnight-basic-review)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the plan is to resolve all issues on the release branch, then yes. L-06 should be fixed with its own PR though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, own PR. Worth landing after #821, which already changes this signature to take
expectedEpoch.added by claude (dev3-midnight-basic-review)