Don't persist BIP39 mnemonic password; require it on load and spend

The mnemonic password is a second factor whose value depends on not being
stored next to the seed. The prior flow both persisted it and plumbed it
through the sign path, getting the worst of both. Switch to a supply model:

- Drop WalletData.mnemonicPassword; fromMnemonicWithPassword no longer writes it.
- loadSpendingKey / getChainAddress derive from the supplied password.
- loadExisting + RailgunEngine.loadExistingWallet take an optional
  mnemonicPassword; keys/address are re-derived each session.
- assertMnemonicPasswordMatchesID verifies the supplied password reproduces
  the wallet ID on load and on every spend, turning a wrong/missing password
  into a clear error instead of a silently divergent key.

Tests: supply-model load, no-password regression, wrong/missing-password
rejection, and sign+verify with a supplied password (plus negative cases).

Author: zy0n

README.md

@@ -42,7 +42,10 @@ RAILGUN Wallet
 
 - `railgunEngine.createWalletFromMnemonic()` (instance method)
 - `railgunEngine.createWalletFromMnemonicWithPassword()` (instance method)
+  - The BIP39 mnemonic password is **not stored** by the engine. The caller owns it and
+    must supply it again to `loadExistingWallet()` and to every spend.
 - `railgunEngine.loadExistingWallet()` (instance method)
+  - Pass the BIP39 mnemonic password as the third argument for wallets created with one.
 
 View-only RAILGUN Wallet
 

src/models/wallet-types.ts

@@ -36,7 +36,6 @@ export type AddressKeys = {
 
 export type WalletData = {
   mnemonic: string;
-  mnemonicPassword?: string;
   index: number;
   creationBlockNumbers: Optional<number[][]>;
 };

src/railgun-engine.ts

@@ -2098,13 +2098,25 @@ class RailgunEngine extends EventEmitter {
    * Load existing wallet
    * @param {string} encryptionKey - encryption key of wallet
    * @param {string} id - wallet ID
+   * @param {string} mnemonicPassword - BIP39 mnemonic password, if the wallet was created
+   *   with one. Not stored by the engine; must be supplied on every load (and on spend).
    * @returns id
    */
-  async loadExistingWallet(encryptionKey: string, id: string): Promise<RailgunWallet> {
+  async loadExistingWallet(
+    encryptionKey: string,
+    id: string,
+    mnemonicPassword?: string,
+  ): Promise<RailgunWallet> {
     if (isDefined(this.wallets[id])) {
       return this.wallets[id] as RailgunWallet;
     }
-    const wallet = await RailgunWallet.loadExisting(this.db, encryptionKey, id, this.prover);
+    const wallet = await RailgunWallet.loadExisting(
+      this.db,
+      encryptionKey,
+      id,
+      this.prover,
+      mnemonicPassword,
+    );
     await this.loadWallet(wallet);
     return wallet;
   }

src/transaction/__tests__/transaction-erc20.test.ts

@@ -851,6 +851,8 @@ describe('transaction-erc20', function test() {
 
     expect(signature).to.deep.equal(signEDDSA(privateKey, msg));
 
+    // Mnemonic-password wallet: the password is supplied (never stored) and must be
+    // threaded through sign -> getSpendingKeyPair to derive the correct spending key.
     const mnemonicPassword = 'test mnemonic password';
     const passwordWallet = await RailgunWallet.fromMnemonicWithPassword(
       db,
@@ -861,20 +863,32 @@ describe('transaction-erc20', function test() {
       undefined,
       prover,
     );
-    const signatureWithMnemonicPassword = await wallet.sign(
+    const passwordSpendingKeyPair = await passwordWallet.getSpendingKeyPair(
+      testEncryptionKey,
+      mnemonicPassword,
+    );
+    const signatureWithMnemonicPassword = await passwordWallet.sign(
       publicInputs,
       testEncryptionKey,
       mnemonicPassword,
     );
-    const passwordSpendingKeyPair = await passwordWallet.getSpendingKeyPair(testEncryptionKey);
 
     assert.isTrue(
-      verifyEDDSA(
-        msg,
-        signatureWithMnemonicPassword,
-        passwordSpendingKeyPair.pubkey,
-      ),
+      verifyEDDSA(msg, signatureWithMnemonicPassword, passwordSpendingKeyPair.pubkey),
     );
+    expect(signatureWithMnemonicPassword).to.deep.equal(
+      signEDDSA(passwordSpendingKeyPair.privateKey, msg),
+    );
+
+    // Signing a mnemonic-password wallet without the password must fail loudly rather
+    // than silently derive a different, unrelated key.
+    await expect(passwordWallet.sign(publicInputs, testEncryptionKey)).to.be.rejectedWith(
+      'Incorrect mnemonic password for wallet.',
+    );
+    // Supplying a password for a wallet that has none must likewise fail.
+    await expect(
+      wallet.sign(publicInputs, testEncryptionKey, mnemonicPassword),
+    ).to.be.rejectedWith('Incorrect mnemonic password for wallet.');
   });
 
   it('Should request one hardware wallet batch approval and sign with the returned sub-session', async () => {

src/wallet/__tests__/railgun-wallet.test.ts

@@ -96,20 +96,77 @@ describe('railgun-wallet', () => {
       new Prover(testArtifactsGetter),
     );
 
+    // A mnemonic password produces a distinct wallet from the no-password wallet.
+    expect(passwordWallet.id).to.not.equal(wallet.id);
+
+    // The mnemonic password is never stored, so it must be supplied again on load.
     const loadedWallet = await RailgunWallet.loadExisting(
       db,
       testEncryptionKey,
       passwordWallet.id,
       new Prover(testArtifactsGetter),
+      mnemonicPassword,
     );
 
-    expect(passwordWallet.id).to.not.equal(wallet.id);
     expect(loadedWallet.id).to.equal(passwordWallet.id);
+    expect(await loadedWallet.getChainAddress(testEncryptionKey, mnemonicPassword)).to.equal(
+      await passwordWallet.getChainAddress(testEncryptionKey, mnemonicPassword),
+    );
+    // The chain address also differs from the no-password wallet.
+    expect(
+      await passwordWallet.getChainAddress(testEncryptionKey, mnemonicPassword),
+    ).to.not.equal(await wallet.getChainAddress(testEncryptionKey));
+  });
+
+  it('Should produce the same id/address for a no-password wallet (regression)', async () => {
+    // Loading without a password must reproduce the wallet unchanged — guards the
+    // empty-passphrase derivation path against regressions.
+    const loadedWallet = await RailgunWallet.loadExisting(
+      db,
+      testEncryptionKey,
+      wallet.id,
+      new Prover(testArtifactsGetter),
+    );
+    expect(loadedWallet.id).to.equal(wallet.id);
     expect(await loadedWallet.getChainAddress(testEncryptionKey)).to.equal(
-      await passwordWallet.getChainAddress(testEncryptionKey),
+      await wallet.getChainAddress(testEncryptionKey),
     );
   });
 
+  it('Should reject loading a mnemonic-password wallet with a wrong/missing password', async () => {
+    const mnemonicPassword = 'test mnemonic password';
+    const passwordWallet = await RailgunWallet.fromMnemonicWithPassword(
+      db,
+      testEncryptionKey,
+      testMnemonic,
+      mnemonicPassword,
+      0,
+      undefined, // creationBlockNumbers
+      new Prover(testArtifactsGetter),
+    );
+
+    // Missing password.
+    await expect(
+      RailgunWallet.loadExisting(
+        db,
+        testEncryptionKey,
+        passwordWallet.id,
+        new Prover(testArtifactsGetter),
+      ),
+    ).to.be.rejectedWith('Incorrect mnemonic password for wallet.');
+
+    // Wrong password.
+    await expect(
+      RailgunWallet.loadExisting(
+        db,
+        testEncryptionKey,
+        passwordWallet.id,
+        new Prover(testArtifactsGetter),
+        'wrong password',
+      ),
+    ).to.be.rejectedWith('Incorrect mnemonic password for wallet.');
+  });
+
   it('Should load existing view-only wallet', async () => {
     const viewOnlyWallet2 = await ViewOnlyWallet.loadExisting(
       db,

src/wallet/railgun-wallet.ts

@@ -42,25 +42,28 @@ class RailgunWallet extends AbstractWallet {
    */
   private async loadSpendingKey(
     encryptionKey: string,
-    mnemonicPasswordOverride?: string,
+    mnemonicPassword?: string,
   ): Promise<WalletNode> {
-    const { mnemonic, mnemonicPassword, index } = (await RailgunWallet.read(
+    const { mnemonic, index } = (await RailgunWallet.read(
       this.db,
       this.id,
       encryptionKey,
     )) as WalletData;
-    return deriveNodes(mnemonic, index, mnemonicPasswordOverride ?? mnemonicPassword).spending;
+    RailgunWallet.assertMnemonicPasswordMatchesID(this.id, mnemonic, index, mnemonicPassword);
+    return deriveNodes(mnemonic, index, mnemonicPassword).spending;
   }
 
   /**
-   * Helper to get the ethereum/whatever address is associated with this wallet
+   * Helper to get the ethereum/whatever address is associated with this wallet.
+   * The BIP39 mnemonic password (if any) must be supplied; it is never stored.
    */
-  async getChainAddress(encryptionKey: string): Promise<string> {
-    const { mnemonic, mnemonicPassword, index } = (await AbstractWallet.read(
+  async getChainAddress(encryptionKey: string, mnemonicPassword?: string): Promise<string> {
+    const { mnemonic, index } = (await AbstractWallet.read(
       this.db,
       this.id,
       encryptionKey,
     )) as WalletData;
+    RailgunWallet.assertMnemonicPasswordMatchesID(this.id, mnemonic, index, mnemonicPassword);
     return Mnemonic.to0xAddress(mnemonic, index, mnemonicPassword);
   }
 
@@ -78,6 +81,23 @@ class RailgunWallet extends AbstractWallet {
     );
   }
 
+  /**
+   * The BIP39 mnemonic password is never persisted; it must be supplied by the caller
+   * on load and on every spend. Verify the supplied password reproduces this wallet's
+   * ID — a mismatch means the wrong (or a missing) password was supplied, which would
+   * otherwise silently derive a different, unrelated key. Fail loudly instead.
+   */
+  private static assertMnemonicPasswordMatchesID(
+    id: string,
+    mnemonic: string,
+    index: number,
+    mnemonicPassword: string = '',
+  ): void {
+    if (RailgunWallet.generateID(mnemonic, index, mnemonicPassword) !== id) {
+      throw new Error('Incorrect mnemonic password for wallet.');
+    }
+  }
+
   private static async createWallet(
     id: string,
     db: Database,
@@ -142,12 +162,10 @@ class RailgunWallet extends AbstractWallet {
   ): Promise<RailgunWallet> {
     const id = RailgunWallet.generateID(mnemonic, index, mnemonicPassword);
 
-    // Write encrypted mnemonic to DB
-    const walletData: WalletData = { mnemonic, index, creationBlockNumbers };
-    if (mnemonicPassword !== '') {
-      walletData.mnemonicPassword = mnemonicPassword;
-    }
-    await AbstractWallet.write(db, id, encryptionKey, walletData);
+    // Write encrypted mnemonic to DB. The BIP39 mnemonic password is intentionally NOT
+    // persisted: storing it next to the mnemonic would defeat its purpose as a second
+    // factor. The caller owns the password and must supply it on load and on spend.
+    await AbstractWallet.write(db, id, encryptionKey, { mnemonic, index, creationBlockNumbers });
 
     return this.createWallet(
       id,
@@ -172,16 +190,21 @@ class RailgunWallet extends AbstractWallet {
     encryptionKey: string,
     id: string,
     prover: Prover,
+    mnemonicPassword?: string,
   ): Promise<RailgunWallet> {
     // Get encrypted mnemonic and index from DB
-    const { mnemonic, mnemonicPassword, index, creationBlockNumbers } = (await AbstractWallet.read(
+    const { mnemonic, index, creationBlockNumbers } = (await AbstractWallet.read(
       db,
       id,
       encryptionKey,
     )) as WalletData;
     if (!mnemonic) {
       throw new Error('Incorrect wallet type.');
     }
+    // The mnemonic password is not stored — the supplied one must reproduce this wallet's
+    // ID. Verifying here turns a wrong/missing password into a clear error instead of
+    // silently loading a different, unrelated wallet's keys.
+    RailgunWallet.assertMnemonicPasswordMatchesID(id, mnemonic, index, mnemonicPassword);
 
     return this.createWallet(
       id,