fix(key-store): reject accounts sharing master privacy keys

yarn-project/end-to-end/src/automine/accounts/multiple_accounts_1_enc_key.test.ts

@@ -11,7 +11,9 @@ import { AutomineTestContext } from '../automine_test_context.js';
 // Verifies that the PXE correctly handles multiple Schnorr accounts sharing the same encryption
 // key (different signing keys). Checks that note discovery and balance tracking remain accurate
 // across three accounts. Uses AUTOMINE_E2E_OPTS with 3 custom accounts sharing one secret.
-describe('automine/accounts/multiple_accounts_1_enc_key', () => {
+// TODO(F-835): unskip once the aztec-nr fix for #24821 is deployed and the PXE restriction on accounts sharing
+// privacy keys is removed. Until then the key store rejects the shared-secret accounts this suite creates.
+describe.skip('automine/accounts/multiple_accounts_1_enc_key', () => {
   let wallet: TestWallet;
   let accounts: AztecAddress[] = [];
   let logger: Logger;

yarn-project/end-to-end/src/automine/token/crowdfunding_and_claim.test.ts

@@ -243,16 +243,22 @@ describe('automine/token/crowdfunding_and_claim', () => {
     // 1) Deploy another instance of the crowdfunding contract
     let otherCrowdfundingContract: CrowdfundingContract;
     {
+      const otherCrowdfundingSecretKey = Fr.random();
+      const otherCrowdfundingPublicKeys = (await deriveKeys(otherCrowdfundingSecretKey)).publicKeys;
       const otherCrowdfundingDeployment = CrowdfundingContract.deploy(
         wallet,
         donationToken.address,
         operatorAddress,
         deadline,
-        { publicKeys: crowdfundingPublicKeys, deployer: operatorAddress },
+        { publicKeys: otherCrowdfundingPublicKeys, deployer: operatorAddress },
       );
 
       const otherCrowdfundingInstance = await otherCrowdfundingDeployment.getInstance();
-      await wallet.registerContract(otherCrowdfundingInstance, CrowdfundingContract.artifact, crowdfundingSecretKey);
+      await wallet.registerContract(
+        otherCrowdfundingInstance,
+        CrowdfundingContract.artifact,
+        otherCrowdfundingSecretKey,
+      );
       ({ contract: otherCrowdfundingContract } = await otherCrowdfundingDeployment.send({
         from: operatorAddress,
         // The contract constructor initializes private storage vars that need the contract's own nullifier key.

yarn-project/end-to-end/src/guides/writing_an_account_contract.test.ts

@@ -92,7 +92,7 @@ describe('guides/writing_an_account_contract', () => {
     const wrongKey = GrumpkinScalar.random();
     const wrongAccountContract = new SchnorrHardcodedKeyAccountContract(wrongKey);
     const wrongAccount = await (wallet as TestWallet).createAccount({
-      secret: secretKey,
+      secret: Fr.random(),
       contract: wrongAccountContract,
       salt: Fr.random(),
     });

yarn-project/end-to-end/src/single-node/bot/bot.test.ts

@@ -188,8 +188,8 @@ describe('single-node/bot/bot', () => {
       }
     });
 
-    // Changes the sender salt between attempts; asserts a new bridge claim is triggered even though
-    // the prior claim is in the store.
+    // Changes the sender private key between attempts; asserts a new bridge claim is triggered even
+    // though the prior claim is in the store.
     it('does not reuse prior bridge claims if recipient address changes', async () => {
       using saveSpy = jest.spyOn(store, 'saveBridgeClaim');
       const config: BotConfig = {
@@ -199,7 +199,7 @@ describe('single-node/bot/bot', () => {
         botMode: 'transfer',
 
         // this bot has a well defined private key and salt
-        senderPrivateKey: new SecretValue(Fr.fromString('0xcafe')),
+        senderPrivateKey: new SecretValue(Fr.fromString('0xbeef')),
         senderSalt: Fr.random(),
 
         l1RpcUrls,
@@ -224,9 +224,9 @@ describe('single-node/bot/bot', () => {
       {
         saveSpy.mockClear();
 
-        // same private key, but different salt derives a different L2 address, so the persisted claim does
-        // not apply and a fresh claim is bridged and saved
-        config.senderSalt = config.senderSalt!.add(Fr.ONE);
+        // a different private key derives a different L2 address, so the persisted claim does not apply
+        // and a fresh claim is bridged and saved
+        config.senderPrivateKey = new SecretValue(Fr.fromString('0xf00d'));
         await expect(Bot.create(config, wallet, aztecNode, aztecNodeAdmin, store)).resolves.toBeDefined();
         expect(saveSpy).toHaveBeenCalledOnce();
       }

yarn-project/key-store/src/key_store.test.ts

@@ -101,6 +101,28 @@ describe('KeyStore', () => {
     }
   });
 
+  it('allows re-registering the same account', async () => {
+    const keyStore = new KeyStore(await openTmpStore('test'));
+
+    const privacyKeys = await deriveKeys(new Fr(8923n));
+    const { address } = await keyStore.addAccount(privacyKeys, new Fr(243523n));
+
+    const { address: reRegisteredAddress } = await keyStore.addAccount(privacyKeys, new Fr(243523n));
+    expect(reRegisteredAddress.equals(address)).toBe(true);
+  });
+
+  it.each(PRIVACY_SECRET_KEY_NAMES)('rejects registering a second account sharing only %s', async keyName => {
+    const keyStore = new KeyStore(await openTmpStore('test'));
+
+    const privacyKeys = await deriveKeys(new Fr(8923n));
+    const { address } = await keyStore.addAccount(privacyKeys, new Fr(243523n));
+
+    const otherPrivacyKeys = await deriveKeys(new Fr(1234n));
+    otherPrivacyKeys[keyName] = privacyKeys[keyName];
+
+    await expect(keyStore.addAccount(otherPrivacyKeys, new Fr(243523n))).rejects.toThrow(address.toString());
+  });
+
   it('rejects registering an account with secret key resulting in infinity public keys', async () => {
     const keyStore = new KeyStore(await openTmpStore('test'));
 

yarn-project/key-store/src/key_store.ts

@@ -106,6 +106,7 @@ export class KeyStore {
    * @param partialAddress - The partial address of the account.
    * @returns The account's complete address.
    * @throws If any of the account's six master public keys would be the point at infinity.
+   * @throws If a different account sharing any of the four master privacy keys is already registered.
    */
   public async addAccount(keys: AccountPrivacyKeys, partialAddress: PartialAddress): Promise<CompleteAddress> {
     const accountKeys = await completeAccountKeys(keys);
@@ -359,6 +360,13 @@ export class KeyStore {
     const masterIncomingViewingPublicKeyHash = await hashPublicKey(publicKeys.ivpkM);
 
     await this.#db.transactionAsync(async () => {
+      await this.#assertPrivacyKeysNotInUse(account, {
+        n: publicKeys.npkMHash,
+        iv: masterIncomingViewingPublicKeyHash,
+        ov: publicKeys.ovpkMHash,
+        t: publicKeys.tpkMHash,
+      });
+
       // Naming of keys is as follows ${account}-${n/iv/ov/t}${sk/pk}_m.
       //
       // The message-signing and fallback keys are not stored: their secret keys are withheld from the key store, and
@@ -384,6 +392,42 @@ export class KeyStore {
     return completeAddress;
   }
 
+  /**
+   * Throws if an account other than `account` is already stored with any of the given master public key hashes, keyed
+   * by key prefix. Must be called within a db transaction so the check and the subsequent writes are atomic.
+   *
+   * Noir does not currently handle different addresses sharing privacy keys correctly: two accounts with the same
+   * nullifier-hiding key compute the same PrivateMutable and PrivateImmutable initialization nullifiers, since those
+   * are derived from the key and the storage slot alone, without the owner address. Until that is fixed, PXE refuses
+   * to register such accounts. Note that a user registering the two accounts in separate PXEs can still run into the
+   * issue, as this check only sees locally registered accounts.
+   *
+   * TODO(F-835): remove this restriction once the aztec-nr fix for #24821 is deployed.
+   */
+  async #assertPrivacyKeysNotInUse(account: AztecAddress, pkMHashes: Record<KeyPrefix, Fr>) {
+    const collidingKeyByStorageSuffix = new Map(
+      KEY_PREFIXES.map(prefix => [
+        `${prefix}pk_m_hash`,
+        { name: MASTER_KEY_NAMES[prefix], pkMHashBuffer: pkMHashes[prefix].toBuffer() },
+      ]),
+    );
+    for await (const [key, value] of this.#keys.entriesAsync()) {
+      const [existingAccount, suffix] = key.split('-');
+      const collidingKey = collidingKeyByStorageSuffix.get(suffix);
+      if (
+        collidingKey &&
+        existingAccount !== account.toString() &&
+        Buffer.from(value).equals(collidingKey.pkMHashBuffer)
+      ) {
+        throw new Error(
+          `Cannot register account ${account.toString()}: account ${existingAccount} is already registered with ` +
+            `the same master ${collidingKey.name} key, and accounts must not share master keys. Derive the new ` +
+            `account from a different secret key.`,
+        );
+      }
+    }
+  }
+
   /**
    * Fetches a stored master key buffer for an account by its storage suffix (e.g. `npk_m`, `ivsk_m`).
    * @throws If the account does not exist in the key store.
@@ -410,3 +454,11 @@ export class KeyStore {
     return buffers;
   }
 }
+
+/** Human-readable master key names by key prefix, for error messages. */
+const MASTER_KEY_NAMES: Record<KeyPrefix, string> = {
+  n: 'nullifier-hiding',
+  iv: 'incoming-viewing',
+  ov: 'outgoing-viewing',
+  t: 'tagging',
+};