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
13 changes: 13 additions & 0 deletions src/mpcCoreKit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@ export class Web3AuthMPCCoreKit implements ICoreKit {

public torusSp: TSSTorusServiceProvider | null = null;

// new user indication
// only true during new user sign up, after reinit or rehydration, the flag will be always false
public newUser: boolean = false;

private options: Web3AuthOptionsWithDefaults;

private storageLayer: TorusStorageLayer | null = null;
Expand Down Expand Up @@ -1093,6 +1097,7 @@ export class Web3AuthMPCCoreKit implements ICoreKit {

// mutation function
private async handleNewUser(importTssKey?: string, isSfaKey?: boolean) {
this.newUser = true;
await this.atomicSync(async () => {
// Generate or use hash factor and initialize tkey with it.
let factorKey: BN;
Expand Down Expand Up @@ -1146,6 +1151,7 @@ export class Web3AuthMPCCoreKit implements ICoreKit {
}

private async handleExistingUser() {
this.newUser = false;
await this.tKey.initialize({ neverInitializeNewKey: true });
if (this.options.disableHashedFactorKey) {
return;
Expand Down Expand Up @@ -1283,6 +1289,12 @@ export class Web3AuthMPCCoreKit implements ICoreKit {

private async checkIfFactorKeyValid(factorKey: BN): Promise<boolean> {
this.checkReady();
const factorKeyPrivate = factorKeyCurve.keyFromPrivate(factorKey.toBuffer());
const factorPubX = factorKeyPrivate.getPublic().getX().toString("hex").padStart(64, "0");
const existingFactorEnc = this.tkey.metadata.factorEncs?.[this.tkey.tssTag]?.[factorPubX];
if (!existingFactorEnc) {
return false;
}
const factorKeyMetadata = await this.tKey?.readMetadata<StringifiedType>(factorKey);
if (!factorKeyMetadata || factorKeyMetadata.message === "KEY_NOT_FOUND" || factorKeyMetadata.message === "SHARE_DELETED") {
return false;
Expand Down Expand Up @@ -1403,6 +1415,7 @@ export class Web3AuthMPCCoreKit implements ICoreKit {
this.tkey = null;
this.torusSp = null;
this.storageLayer = null;
this.newUser = false;
this.state = { accountIndex: 0 };
}

Expand Down
31 changes: 27 additions & 4 deletions tests/factors.spec.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import assert from "node:assert";
import test from "node:test";

import { EllipticPoint, KeyType, Point, secp256k1 } from "@tkey/common-types";
import { EllipticPoint, getPubKeyPoint, KeyType, Point, secp256k1 } from "@tkey/common-types";
import { factorKeyCurve } from "@tkey/tss";
import { tssLib as tssLibDKLS } from "@toruslabs/tss-dkls-lib";
import { tssLib as tssLibFROST } from "@toruslabs/tss-frost-lib";
Expand Down Expand Up @@ -158,7 +158,7 @@ export const FactorManipulationTest = async (testVariable: FactorTestVariable) =
});

// enable mfa

let browserFactor: string;
await t.test("enable MFA", async function () {
const instance = await newInstance();
assert.strictEqual(instance.status, COREKIT_STATUS.LOGGED_IN);
Expand All @@ -179,7 +179,7 @@ export const FactorManipulationTest = async (testVariable: FactorTestVariable) =
const instance2 = await newInstance();
assert.strictEqual(instance2.status, COREKIT_STATUS.REQUIRED_SHARE);

const browserFactor = await instance2.getDeviceFactor();
browserFactor = await instance2.getDeviceFactor();

const factorBN = new BN(recoverFactor, "hex")

Expand Down Expand Up @@ -210,9 +210,32 @@ export const FactorManipulationTest = async (testVariable: FactorTestVariable) =
} else {
await signSecp256k1Data({ coreKitInstance: instance3, msg: "hello world" });
}

});

// replace factor
await t.test("replace factor", async function () {
const instance = await newInstance();

const deviceFactorKeyBN = new BN(browserFactor, "hex")
await instance.inputFactorKey(deviceFactorKeyBN);
assert.strictEqual(instance.status, COREKIT_STATUS.LOGGED_IN);

const newFactorkey = await instance.createFactor({ shareType: TssShareType.DEVICE });
await instance.inputFactorKey(new BN(newFactorkey, "hex"));

assert.strictEqual(instance.status, COREKIT_STATUS.LOGGED_IN);


const deviceFactorPub = getPubKeyPoint(deviceFactorKeyBN);
Copy link

Choose a reason for hiding this comment

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

Bug: Test uses wrong import and missing curve parameter

The test imports getPubKeyPoint from @tkey/common-types but all source code imports it from @tkey/tss and calls it with two arguments: the key and factorKeyCurve. The test call getPubKeyPoint(deviceFactorKeyBN) only passes one argument. When this Point is passed to deleteFactor, it's compared against Points created using factorKeyCurve. Using a different function or missing the curve parameter will cause incorrect Point comparisons, making the test unreliable. The factorKeyCurve is already imported in the test file but not used with getPubKeyPoint.

Additional Locations (1)

Fix in Cursor Fix in Web

await instance.deleteFactor(deviceFactorPub, browserFactor);

try {
await instance.inputFactorKey(deviceFactorKeyBN);
throw Error("should not be able to deleted input factor");
} catch (e) {
assert(e instanceof Error);
}
Copy link

Choose a reason for hiding this comment

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

Bug: Test assertion passes even when test should fail

The test's catch block uses assert(e instanceof Error) which will pass for both the expected error from inputFactorKey (when it correctly rejects a deleted factor) AND the manually thrown error on line 234 (when inputFactorKey incorrectly succeeds). If inputFactorKey fails to reject the deleted factor key, the test will still pass because the fallback throw Error(...) is also caught and satisfies the instanceof Error check. This masks potential regressions in the production code.

Fix in Cursor Fix in Web

Copy link

Choose a reason for hiding this comment

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

Bug: Test fails silently when expected error not thrown

The test's error handling logic is flawed. The catch block checks assert(e instanceof Error), which passes for both the expected error from inputFactorKey AND the manually thrown error at line 234. If inputFactorKey succeeds unexpectedly, the test throws "should not be able to deleted input factor", catches it, and the assertion still passes since that error is also an Error instance. This masks test failures - the test will pass even when the deleted factor key is incorrectly accepted.

Fix in Cursor Fix in Web

});
});
};

Expand Down