Merge pull request #750 from LIT-Protocol/feature/escal-11-pkp-address-derivation-issue

Ansonhkg · web-flow · commit 498ce4290078 · 2024-12-30T15:38:02.000Z
Fix: apply low-s `s` flips also to `v` on ecdsa signature shares combination
diff --git a/local-tests/tests/testDelegatingCapacityCreditsNFTToAnotherWalletToPkpSign.ts b/local-tests/tests/testDelegatingCapacityCreditsNFTToAnotherWalletToPkpSign.ts
@@ -1,3 +1,5 @@
+import { ethers } from 'ethers';
+
 import { getEoaSessionSigsWithCapacityDelegations } from 'local-tests/setup/session-sigs/get-eoa-session-sigs';
 import { TinnyEnvironment } from 'local-tests/setup/tinny-environment';
 
@@ -92,5 +94,21 @@ export const testDelegatingCapacityCreditsNFTToAnotherWalletToPkpSign = async (
     throw new Error(`Expected "recid" to be parseable as a number`);
   }
 
+  const signature = ethers.utils.joinSignature({
+    r: '0x' + res.r,
+    s: '0x' + res.s,
+    recoveryParam: res.recid,
+  });
+  const recoveredPubKey = ethers.utils.recoverPublicKey(
+    alice.loveLetter,
+    signature
+  );
+  if (recoveredPubKey !== `0x${res.publicKey.toLowerCase()}`) {
+    throw new Error(`Expected recovered public key to match res.publicKey`);
+  }
+  if (recoveredPubKey !== `0x${bob.pkp.publicKey.toLowerCase()}`) {
+    throw new Error(`Expected recovered public key to match bob.pkp.publicKey`);
+  }
+
   console.log('✅ res:', res);
 };
diff --git a/local-tests/tests/testUseCapacityDelegationAuthSigWithUnspecifiedCapacityTokenIdToPkpSign.ts b/local-tests/tests/testUseCapacityDelegationAuthSigWithUnspecifiedCapacityTokenIdToPkpSign.ts
@@ -1,3 +1,5 @@
+import { ethers } from 'ethers';
+
 import { getEoaSessionSigsWithCapacityDelegations } from 'local-tests/setup/session-sigs/get-eoa-session-sigs';
 import { TinnyEnvironment } from 'local-tests/setup/tinny-environment';
 
@@ -94,5 +96,23 @@ export const testUseCapacityDelegationAuthSigWithUnspecifiedCapacityTokenIdToPkp
       throw new Error(`Expected "recid" to be parseable as a number`);
     }
 
+    const signature = ethers.utils.joinSignature({
+      r: '0x' + res.r,
+      s: '0x' + res.s,
+      recoveryParam: res.recid,
+    });
+    const recoveredPubKey = ethers.utils.recoverPublicKey(
+      alice.loveLetter,
+      signature
+    );
+    if (recoveredPubKey !== `0x${res.publicKey.toLowerCase()}`) {
+      throw new Error(`Expected recovered public key to match res.publicKey`);
+    }
+    if (recoveredPubKey !== `0x${bob.pkp.publicKey.toLowerCase()}`) {
+      throw new Error(
+        `Expected recovered public key to match bob.pkp.publicKey`
+      );
+    }
+
     console.log('✅ res:', res);
   };
diff --git a/local-tests/tests/testUseCapacityDelegationAuthSigWithUnspecifiedDelegateesToPkpSign.ts b/local-tests/tests/testUseCapacityDelegationAuthSigWithUnspecifiedDelegateesToPkpSign.ts
@@ -1,3 +1,5 @@
+import { ethers } from 'ethers';
+
 import { getEoaSessionSigsWithCapacityDelegations } from 'local-tests/setup/session-sigs/get-eoa-session-sigs';
 import { TinnyEnvironment } from 'local-tests/setup/tinny-environment';
 
@@ -86,6 +88,26 @@ export const testUseCapacityDelegationAuthSigWithUnspecifiedDelegateesToPkpSign
       throw new Error(`Expected "signature" to start with 0x`);
     }
 
+    const signature = ethers.utils.joinSignature({
+      r: '0x' + runWithSessionSigs.r,
+      s: '0x' + runWithSessionSigs.s,
+      recoveryParam: runWithSessionSigs.recid,
+    });
+    const recoveredPubKey = ethers.utils.recoverPublicKey(
+      alice.loveLetter,
+      signature
+    );
+    if (recoveredPubKey !== `0x${runWithSessionSigs.publicKey.toLowerCase()}`) {
+      throw new Error(
+        `Expected recovered public key to match runWithSessionSigs.publicKey`
+      );
+    }
+    if (recoveredPubKey !== `0x${bob.pkp.publicKey.toLowerCase()}`) {
+      throw new Error(
+        `Expected recovered public key to match bob.pkp.publicKey`
+      );
+    }
+
     // recid must be parseable as a number
     if (isNaN(runWithSessionSigs.recid)) {
       throw new Error(`Expected "recid" to be parseable as a number`);
diff --git a/local-tests/tests/testUseEoaSessionSigsToPkpSign.ts b/local-tests/tests/testUseEoaSessionSigsToPkpSign.ts
@@ -1,3 +1,5 @@
+import { ethers } from 'ethers';
+
 import { log } from '@lit-protocol/misc';
 import { getEoaSessionSigs } from 'local-tests/setup/session-sigs/get-eoa-session-sigs';
 import { TinnyEnvironment } from 'local-tests/setup/tinny-environment';
@@ -58,5 +60,25 @@ export const testUseEoaSessionSigsToPkpSign = async (
     throw new Error(`Expected "recid" to be parseable as a number`);
   }
 
+  const signature = ethers.utils.joinSignature({
+    r: '0x' + runWithSessionSigs.r,
+    s: '0x' + runWithSessionSigs.s,
+    recoveryParam: runWithSessionSigs.recid,
+  });
+  const recoveredPubKey = ethers.utils.recoverPublicKey(
+    alice.loveLetter,
+    signature
+  );
+  if (recoveredPubKey !== `0x${runWithSessionSigs.publicKey.toLowerCase()}`) {
+    throw new Error(
+      `Expected recovered public key to match runWithSessionSigs.publicKey`
+    );
+  }
+  if (recoveredPubKey !== `0x${alice.pkp.publicKey.toLowerCase()}`) {
+    throw new Error(
+      `Expected recovered public key to match alice.pkp.publicKey`
+    );
+  }
+
   log('✅ testUseEoaSessionSigsToPkpSign');
 };
diff --git a/local-tests/tests/testUsePkpSessionSigsToPkpSign.ts b/local-tests/tests/testUsePkpSessionSigsToPkpSign.ts
@@ -1,3 +1,5 @@
+import { ethers } from 'ethers';
+
 import { log } from '@lit-protocol/misc';
 import { getPkpSessionSigs } from 'local-tests/setup/session-sigs/get-pkp-session-sigs';
 import { TinnyEnvironment } from 'local-tests/setup/tinny-environment';
@@ -58,5 +60,25 @@ export const testUsePkpSessionSigsToPkpSign = async (
     throw new Error(`Expected "recid" to be parseable as a number`);
   }
 
+  const signature = ethers.utils.joinSignature({
+    r: '0x' + res.r,
+    s: '0x' + res.s,
+    recoveryParam: res.recid,
+  });
+  const recoveredPubKey = ethers.utils.recoverPublicKey(
+    alice.loveLetter,
+    signature
+  );
+  if (recoveredPubKey !== `0x${res.publicKey.toLowerCase()}`) {
+    throw new Error(`Expected recovered public key to match res.publicKey`);
+  }
+  if (
+    recoveredPubKey !== `0x${alice.authMethodOwnedPkp.publicKey.toLowerCase()}`
+  ) {
+    throw new Error(
+      `Expected recovered public key to match alice.authMethodOwnedPkp.publicKey`
+    );
+  }
+
   log('✅ res:', res);
 };
diff --git a/local-tests/tests/testUseValidLitActionCodeGeneratedSessionSigsToPkpSign.ts b/local-tests/tests/testUseValidLitActionCodeGeneratedSessionSigsToPkpSign.ts
@@ -1,3 +1,5 @@
+import { ethers } from 'ethers';
+
 import { log } from '@lit-protocol/misc';
 import { LIT_NETWORK } from '@lit-protocol/constants';
 import { getLitActionSessionSigs } from 'local-tests/setup/session-sigs/get-lit-action-session-sigs';
@@ -58,5 +60,25 @@ export const testUseValidLitActionCodeGeneratedSessionSigsToPkpSign = async (
     throw new Error(`Expected "recid" to be parseable as a number`);
   }
 
+  const signature = ethers.utils.joinSignature({
+    r: '0x' + res.r,
+    s: '0x' + res.s,
+    recoveryParam: res.recid,
+  });
+  const recoveredPubKey = ethers.utils.recoverPublicKey(
+    alice.loveLetter,
+    signature
+  );
+  if (recoveredPubKey !== `0x${res.publicKey.toLowerCase()}`) {
+    throw new Error(`Expected recovered public key to match res.publicKey`);
+  }
+  if (
+    recoveredPubKey !== `0x${alice.authMethodOwnedPkp.publicKey.toLowerCase()}`
+  ) {
+    throw new Error(
+      `Expected recovered public key to match alice.authMethodOwnedPkp.publicKey`
+    );
+  }
+
   log('✅ res:', res);
 };
diff --git a/local-tests/tests/testUseValidLitActionIpfsCodeGeneratedSessionSigsToPkpSign.ts b/local-tests/tests/testUseValidLitActionIpfsCodeGeneratedSessionSigsToPkpSign.ts
@@ -1,3 +1,5 @@
+import { ethers } from 'ethers';
+
 import { log } from '@lit-protocol/misc';
 import { getLitActionSessionSigsUsingIpfsId } from 'local-tests/setup/session-sigs/get-lit-action-session-sigs';
 import { TinnyEnvironment } from 'local-tests/setup/tinny-environment';
@@ -66,5 +68,26 @@ export const testUseValidLitActionIpfsCodeGeneratedSessionSigsToPkpSign =
       throw new Error(`Expected "recid" to be parseable as a number`);
     }
 
+    const signature = ethers.utils.joinSignature({
+      r: '0x' + res.r,
+      s: '0x' + res.s,
+      recoveryParam: res.recid,
+    });
+    const recoveredPubKey = ethers.utils.recoverPublicKey(
+      alice.loveLetter,
+      signature
+    );
+    if (recoveredPubKey !== `0x${res.publicKey.toLowerCase()}`) {
+      throw new Error(`Expected recovered public key to match res.publicKey`);
+    }
+    if (
+      recoveredPubKey !==
+      `0x${alice.authMethodOwnedPkp.publicKey.toLowerCase()}`
+    ) {
+      throw new Error(
+        `Expected recovered public key to match alice.authMethodOwnedPkp.publicKey`
+      );
+    }
+
     log('✅ res:', res);
   };
diff --git a/packages/crypto/src/lib/crypto.ts b/packages/crypto/src/lib/crypto.ts
@@ -202,14 +202,20 @@ export const combineEcdsaShares = async (
     Buffer.from(share.signatureShare, 'hex')
   );
 
-  const [r, s, v] = await ecdsaCombine(variant!, presignature, signatureShares);
+  const [r, s, recId] = await ecdsaCombine(
+    variant!,
+    presignature,
+    signatureShares
+  );
 
   const publicKey = Buffer.from(anyValidShare.publicKey, 'hex');
   const messageHash = Buffer.from(anyValidShare.dataSigned!, 'hex');
 
-  await ecdsaVerify(variant!, messageHash, publicKey, [r, s, v]);
+  await ecdsaVerify(variant!, messageHash, publicKey, [r, s, recId]);
 
-  const signature = splitSignature(Buffer.concat([r, s, Buffer.from([v])]));
+  const signature = splitSignature(
+    Buffer.concat([r, s, Buffer.from([recId + 27])])
+  );
 
   return {
     r: signature.r.slice('0x'.length),
diff --git a/packages/lit-node-client-nodejs/src/lib/helpers/get-signatures.ts b/packages/lit-node-client-nodejs/src/lib/helpers/get-signatures.ts
@@ -259,7 +259,7 @@ export const getSignatures = async <T>(params: {
     const encodedSig = joinSignature({
       r: '0x' + signature.r,
       s: '0x' + signature.s,
-      v: signature.recid,
+      recoveryParam: signature.recid,
     });
 
     signatures[key] = {
diff --git a/packages/types/src/lib/interfaces.ts b/packages/types/src/lib/interfaces.ts
@@ -42,7 +42,7 @@ export interface AuthSig {
   /**
    * The signature produced by signing the `signMessage` property with the corresponding private key for the `address` property.
    */
-  sig: any;
+  sig: string;
 
   /**
    * The method used to derive the signature (e.g, `web3.eth.personal.sign`).
@@ -598,7 +598,7 @@ export interface SigResponse {
   r: string;
   s: string;
   recid: number;
-  signature: string; // 0x...
+  signature: `0x${string}`;
   publicKey: string; // pkp public key (no 0x prefix)
   dataSigned: string;
 }
diff --git a/packages/wasm/rust/src/ecdsa.rs b/packages/wasm/rust/src/ecdsa.rs
@@ -58,22 +58,22 @@ where
         presignature: Uint8Array,
         signature_shares: Vec<Uint8Array>,
     ) -> JsResult<EcdsaSignature> {
-        let (big_r, s) = Self::combine_inner(presignature, signature_shares)?;
-        Self::signature_into_js(big_r.to_affine(), s)
+        let (big_r, s, was_flipped) = Self::combine_inner(presignature, signature_shares)?;
+        Self::signature_into_js(big_r.to_affine(), s, was_flipped)
     }
 
     pub(crate) fn combine_inner(
         presignature: Uint8Array,
         signature_shares: Vec<Uint8Array>,
-    ) -> JsResult<(C::ProjectivePoint, C::Scalar)> {
+    ) -> JsResult<(C::ProjectivePoint, C::Scalar, bool)> {
         let signature_shares = signature_shares
             .into_iter()
             .map(Self::scalar_from_js)
             .collect::<JsResult<Vec<_>>>()?;
 
         let big_r: C::AffinePoint = Self::point_from_js(presignature)?;
-        let s = Self::sum_scalars(signature_shares)?;
-        Ok((C::ProjectivePoint::from(big_r), s))
+        let (s, was_flipped) = Self::sum_scalars(signature_shares)?;
+        Ok((C::ProjectivePoint::from(big_r), s, was_flipped))
     }
 
     pub fn verify(
@@ -108,13 +108,14 @@ where
         Ok(())
     }
 
-    fn sum_scalars(values: Vec<C::Scalar>) -> JsResult<C::Scalar> {
+    fn sum_scalars(values: Vec<C::Scalar>) -> JsResult<(C::Scalar, bool)> {
         if values.is_empty() {
             return Err(JsError::new("no shares provided"));
         }
         let mut acc: C::Scalar = values.into_iter().sum();
+        let acc_flipped = acc.is_high().into();
         acc.conditional_assign(&(-acc), acc.is_high());
-        Ok(acc)
+        Ok((acc, acc_flipped))
     }
 
     pub fn derive_key(id: Uint8Array, public_keys: Vec<Uint8Array>) -> JsResult<Uint8Array> {
@@ -168,10 +169,15 @@ where
         Ok((r, s, v))
     }
 
-    fn signature_into_js(big_r: C::AffinePoint, s: C::Scalar) -> JsResult<EcdsaSignature> {
+    fn signature_into_js(big_r: C::AffinePoint, s: C::Scalar, was_flipped: bool) -> JsResult<EcdsaSignature> {
         let r = Self::x_coordinate(&big_r).to_repr();
         let s = s.to_repr();
-        let v = u8::conditional_select(&0, &1, big_r.y_is_odd());
+        let mut v = u8::conditional_select(&0, &1, big_r.y_is_odd());
+
+        // Flip v if s was normalized (flipped, low-s rule)
+        if was_flipped {
+            v = 1 - v;
+        }
 
         Ok(EcdsaSignature {
             obj: into_js(&(Bytes::new(&r), Bytes::new(&s), v))?,
@@ -222,7 +228,7 @@ where
         public_key: C::ProjectivePoint,
     ) -> JsResult<EcdsaSignature> {
         let z = Self::scalar_from_hash(message_hash)?;
-        let (big_r, s) = Self::combine_inner(pre_signature, signature_shares)?;
+        let (big_r, s, was_flipped) = Self::combine_inner(pre_signature, signature_shares)?;
         let r = Self::x_coordinate(&big_r.to_affine());
 
         if z.is_zero().into() {
@@ -241,7 +247,7 @@ where
             .is_identity()
             .into()
         {
-            Self::signature_into_js(big_r.to_affine(), s)
+            Self::signature_into_js(big_r.to_affine(), s, was_flipped)
         } else {
             Err(JsError::new("invalid signature"))
         }
