Expand tests for ethereum-schnorr-contract

networks/ethereum/schnorr/contracts/Schnorr.sol

@@ -1,7 +1,13 @@
 // SPDX-License-Identifier: AGPL-3.0-only
 pragma solidity ^0.8.26;
 
-// See https://github.com/noot/schnorr-verify for implementation details
+/// @title A library for verifying Schnorr signatures
+/// @author Luke Parker <lukeparker5132@gmail.com>
+/// @author Elizabeth Binks <elizabethjbinks@gmail.com>
+/// @notice Verifies a Schnorr signature for a specified public key
+/// @dev This contract is not complete. Only certain public keys are compatible
+/// @dev See https://github.com/serai-dex/serai/blob/next/networks/ethereum/schnorr/src/tests/premise.rs for implementation details
+// TODO: Pin to a specific branch/commit once `next` is merged into `develop`
 library Schnorr {
   // secp256k1 group order
   uint256 private constant Q = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141;
@@ -11,31 +17,39 @@ library Schnorr {
   // 2) An x-coordinate < Q
   uint8 private constant KEY_PARITY = 27;
 
-  // px := public key x-coordinate, where the public key has an even y-coordinate
-  // message := the message signed
-  // c := Schnorr signature challenge
-  // s := Schnorr signature solution
-  function verify(bytes32 px, bytes32 message, bytes32 c, bytes32 s) internal pure returns (bool) {
+  /// @notice Verifies a Schnorr signature for the specified public key
+  /// @dev The y-coordinate of the public key is assumed to be even
+  /// @dev The x-coordinate of the public key is assumed to be less than the order of secp256k1
+  /// @dev The challenge is calculated as `keccak256(abi.encodePacked(address(R), public_key, message))` where `R` is the commitment to the Schnorr signature's nonce
+  /// @param public_key The x-coordinate of the public key
+  /// @param message The (hash of the) message signed
+  /// @param c The challenge for the Schnorr signature
+  /// @param s The response to the challenge for the Schnorr signature
+  /// @return If the signature is valid
+  function verify(bytes32 public_key, bytes32 message, bytes32 c, bytes32 s)
+    internal
+    pure
+    returns (bool)
+  {
     // ecrecover = (m, v, r, s) -> key
-    // We instead pass the following to obtain the nonce (not the key)
-    // Then we hash it and verify it matches the challenge
-    bytes32 sa = bytes32(Q - mulmod(uint256(s), uint256(px), Q));
-    bytes32 ca = bytes32(Q - mulmod(uint256(c), uint256(px), Q));
+    // We instead pass the following to recover the Schnorr signature's nonce (not a public key)
+    bytes32 sa = bytes32(Q - mulmod(uint256(s), uint256(public_key), Q));
+    bytes32 ca = bytes32(Q - mulmod(uint256(c), uint256(public_key), Q));
 
     /*
-      The ecrecover precompile checks `r` and `s` (`px` and `ca`) are non-zero,
-      banning the two keys with zero for their x-coordinate and zero challenge.
-      Each has negligible probability of occuring (assuming zero x-coordinates
-      are even on-curve in the first place).
+      The ecrecover precompile checks `r` and `s` (`public_key` and `ca`) are non-zero, banning the
+      two keys with zero for their x-coordinate and zero challenges. Each already only had a
+      negligible probability of occuring (assuming zero x-coordinates are even on-curve in the first
+      place).
 
-      `sa` is not checked to be non-zero yet it does not need to be. The inverse
-      of it is never taken.
+      `sa` is not checked to be non-zero yet it does not need to be. The inverse of it is never
+      taken.
     */
-    address R = ecrecover(sa, KEY_PARITY, px, ca);
+    address R = ecrecover(sa, KEY_PARITY, public_key, ca);
     // The ecrecover failed
     if (R == address(0)) return false;
 
     // Check the signature is correct by rebuilding the challenge
-    return c == keccak256(abi.encodePacked(R, px, message));
+    return c == keccak256(abi.encodePacked(R, public_key, message));
   }
 }

networks/ethereum/schnorr/contracts/tests/Schnorr.sol

@@ -3,7 +3,19 @@ pragma solidity ^0.8.26;
 
 import "../Schnorr.sol";
 
+/// @title A thin wrapper around the library for verifying Schnorr signatures to test it with
+/// @author Luke Parker <lukeparker5132@gmail.com>
+/// @author Elizabeth Binks <elizabethjbinks@gmail.com>
 contract TestSchnorr {
+  /// @notice Verifies a Schnorr signature for the specified public key
+  /// @dev The y-coordinate of the public key is assumed to be even
+  /// @dev The x-coordinate of the public key is assumed to be less than the order of secp256k1
+  /// @dev The challenge is calculated as `keccak256(abi.encodePacked(address(R), public_key, message))` where `R` is the commitment to the Schnorr signature's nonce
+  /// @param public_key The x-coordinate of the public key
+  /// @param message The (hash of the) message signed
+  /// @param c The challenge for the Schnorr signature
+  /// @param s The response to the challenge for the Schnorr signature
+  /// @return If the signature is valid
   function verify(bytes32 public_key, bytes calldata message, bytes32 c, bytes32 s)
     external
     pure

networks/ethereum/schnorr/src/public_key.rs

@@ -31,8 +31,7 @@ impl PublicKey {
 
     let x_coordinate = affine.x();
     // Return None if the x-coordinate isn't mutual to both fields
-    // While reductions shouldn't be an issue, it's one less headache/concern to have
-    // The trivial amount of public keys this makes non-representable aren't a concern
+    // The trivial amount of public keys this makes non-representable aren't considered a concern
     if <Scalar as Reduce<KU256>>::reduce_bytes(&x_coordinate).to_repr() != x_coordinate {
       None?;
     }

networks/ethereum/schnorr/src/signature.rs

@@ -20,11 +20,17 @@ pub struct Signature {
 impl Signature {
   /// Construct a new `Signature`.
   #[must_use]
-  pub fn new(c: Scalar, s: Scalar) -> Signature {
-    Signature { c, s }
+  pub fn new(c: Scalar, s: Scalar) -> Option<Signature> {
+    if bool::from(c.is_zero()) {
+      None?;
+    }
+    Some(Signature { c, s })
   }
 
   /// The challenge for a signature.
+  ///
+  /// With negligible probability, this MAY return 0 which will create an invalid/unverifiable
+  /// signature.
   #[must_use]
   pub fn challenge(R: ProjectivePoint, key: &PublicKey, message: &[u8]) -> Scalar {
     // H(R || A || m)

networks/ethereum/schnorr/src/tests/mod.rs

@@ -17,6 +17,9 @@ use alloy_node_bindings::{Anvil, AnvilInstance};
 
 use crate::{PublicKey, Signature};
 
+mod public_key;
+pub(crate) use public_key::test_key;
+mod signature;
 mod premise;
 
 #[expect(warnings)]
@@ -88,12 +91,7 @@ async fn test_verify() {
   let (_anvil, provider, address) = setup_test().await;
 
   for _ in 0 .. 100 {
-    let (key, public_key) = loop {
-      let key = Scalar::random(&mut OsRng);
-      if let Some(public_key) = PublicKey::new(ProjectivePoint::GENERATOR * key) {
-        break (key, public_key);
-      }
-    };
+    let (key, public_key) = test_key();
 
     let nonce = Scalar::random(&mut OsRng);
     let mut message = vec![0; 1 + usize::try_from(OsRng.next_u32() % 256).unwrap()];
@@ -102,11 +100,37 @@ async fn test_verify() {
     let c = Signature::challenge(ProjectivePoint::GENERATOR * nonce, &public_key, &message);
     let s = nonce + (c * key);
 
-    let sig = Signature::new(c, s);
+    let sig = Signature::new(c, s).unwrap();
     assert!(sig.verify(&public_key, &message));
     assert!(call_verify(&provider, address, &public_key, &message, &sig).await);
+
+    // Test setting `s = 0` doesn't pass verification
+    {
+      let zero_s = Signature::new(c, Scalar::ZERO).unwrap();
+      assert!(!zero_s.verify(&public_key, &message));
+      assert!(!call_verify(&provider, address, &public_key, &message, &zero_s).await);
+    }
+
     // Mutate the message and make sure the signature now fails to verify
+    {
+      let mut message = message.clone();
       message[0] = message[0].wrapping_add(1);
+      assert!(!sig.verify(&public_key, &message));
       assert!(!call_verify(&provider, address, &public_key, &message, &sig).await);
     }
+
+    // Mutate c and make sure the signature now fails to verify
+    {
+      let mutated_c = Signature::new(c + Scalar::ONE, s).unwrap();
+      assert!(!mutated_c.verify(&public_key, &message));
+      assert!(!call_verify(&provider, address, &public_key, &message, &mutated_c).await);
+    }
+
+    // Mutate s and make sure the signature now fails to verify
+    {
+      let mutated_s = Signature::new(c, s + Scalar::ONE).unwrap();
+      assert!(!mutated_s.verify(&public_key, &message));
+      assert!(!call_verify(&provider, address, &public_key, &message, &mutated_s).await);
+    }
+  }
 }

networks/ethereum/schnorr/src/tests/premise.rs

@@ -12,7 +12,7 @@ use k256::{
 
 use alloy_core::primitives::Address;
 
-use crate::{PublicKey, Signature};
+use crate::{Signature, tests::test_key};
 
 // The ecrecover opcode, yet with if the y is odd replacing v
 fn ecrecover(message: Scalar, odd_y: bool, r: Scalar, s: Scalar) -> Option<[u8; 20]> {
@@ -64,12 +64,7 @@ fn test_ecrecover() {
 // of efficiently verifying Schnorr signatures in an Ethereum contract
 #[test]
 fn nonce_recovery_via_ecrecover() {
-  let (key, public_key) = loop {
-    let key = Scalar::random(&mut OsRng);
-    if let Some(public_key) = PublicKey::new(ProjectivePoint::GENERATOR * key) {
-      break (key, public_key);
-    }
-  };
+  let (key, public_key) = test_key();
 
   let nonce = Scalar::random(&mut OsRng);
   let R = ProjectivePoint::GENERATOR * nonce;
@@ -81,26 +76,28 @@ fn nonce_recovery_via_ecrecover() {
   let s = nonce + (c * key);
 
   /*
-    An ECDSA signature is `(r, s)` with `s = (H(m) + rx) / k`, where:
-    - `m` is the message
+    An ECDSA signature is `(r, s)` with `s = (m + (r * x)) / k`, where:
+    - `m` is the hash of the message
     - `r` is the x-coordinate of the nonce, reduced into a scalar
     - `x` is the private key
     - `k` is the nonce
 
     We fix the recovery ID to be for the even key with an x-coordinate < the order. Accordingly,
-    `kG = Point::from(Even, r)`. This enables recovering the public key via
-    `((s Point::from(Even, r)) - H(m)G) / r`.
+    `k * G = Point::from(Even, r)`. This enables recovering the public key via
+    `((s * Point::from(Even, r)) - (m * G)) / r`.
 
     We want to calculate `R` from `(c, s)` where `s = r + cx`. That means we need to calculate
-    `sG - cX`.
+    `(s * G) - (c * X)`.
 
-    We can calculate `sG - cX` with `((s Point::from(Even, r)) - H(m)G) / r` if:
-    - Latter `r` = `X.x`
-    - Latter `s` = `c`
-    - `H(m)` = former `s`
-    This gets us to `(cX - sG) / X.x`. If we additionally scale the latter's `s, H(m)` values (the
-    former's `c, s` values) by `X.x`, we get `cX - sG`. This just requires negating each to achieve
-    `sG - cX`.
+    We can calculate `(s * G) - (c * X)` with `((s * Point::from(Even, r)) - (m * G)) / r` if:
+    - ECDSA `r` = `X.x`, the x-coordinate of the Schnorr public key
+    - ECDSA `s` = `c`, the Schnorr signature's challenge
+    - ECDSA `m` = Schnorr `s`
+    This gets us to `((c * X) - (s * G)) / X.x`. If we additionally scale the ECDSA `s, m` values
+    (the Schnorr `c, s` values) by `X.x`, we get `(c * X) - (s * G)`. This just requires negating
+    to achieve `(s * G) - (c * X)`.
+
+    With `R`, we can recalculate and compare the challenges to confirm the signature is valid.
   */
   let x_scalar = <Scalar as Reduce<U256>>::reduce_bytes(&public_key.point().to_affine().x());
   let sa = -(s * x_scalar);

networks/ethereum/schnorr/src/tests/public_key.rs

@@ -0,0 +1,69 @@
+use rand_core::OsRng;
+
+use subtle::Choice;
+use group::ff::{Field, PrimeField};
+use k256::{
+  elliptic_curve::{
+    FieldBytesEncoding,
+    ops::Reduce,
+    point::{AffineCoordinates, DecompressPoint},
+  },
+  AffinePoint, ProjectivePoint, Scalar, U256 as KU256,
+};
+
+use crate::PublicKey;
+
+// Generates a key usable within tests
+pub(crate) fn test_key() -> (Scalar, PublicKey) {
+  loop {
+    let key = Scalar::random(&mut OsRng);
+    let point = ProjectivePoint::GENERATOR * key;
+    if let Some(public_key) = PublicKey::new(point) {
+      // While here, test `PublicKey::point` and its serialization functions
+      assert_eq!(point, public_key.point());
+      assert_eq!(PublicKey::from_eth_repr(public_key.eth_repr()).unwrap(), public_key);
+      return (key, public_key);
+    }
+  }
+}
+
+#[test]
+fn test_odd_key() {
+  // We generate a valid key to ensure there's not some distinct reason this key is invalid
+  let (_, key) = test_key();
+  // We then take its point and negate it so its y-coordinate is odd
+  let odd = -key.point();
+  assert!(PublicKey::new(odd).is_none());
+}
+
+#[test]
+fn test_non_mutual_key() {
+  let mut x_coordinate = KU256::from(-(Scalar::ONE)).saturating_add(&KU256::ONE);
+
+  let y_is_odd = Choice::from(0);
+  let non_mutual = loop {
+    if let Some(point) = Option::<AffinePoint>::from(AffinePoint::decompress(
+      &FieldBytesEncoding::encode_field_bytes(&x_coordinate),
+      y_is_odd,
+    )) {
+      break point;
+    }
+    x_coordinate = x_coordinate.saturating_add(&KU256::ONE);
+  };
+  let x_coordinate = non_mutual.x();
+  assert!(<Scalar as Reduce<KU256>>::reduce_bytes(&x_coordinate).to_repr() != x_coordinate);
+
+  // Even point whose x-coordinate isn't mutual to both fields (making it non-zero)
+  assert!(PublicKey::new(non_mutual.into()).is_none());
+}
+
+#[test]
+fn test_zero_key() {
+  let y_is_odd = Choice::from(0);
+  if let Some(A_affine) =
+    Option::<AffinePoint>::from(AffinePoint::decompress(&[0; 32].into(), y_is_odd))
+  {
+    let A = ProjectivePoint::from(A_affine);
+    assert!(PublicKey::new(A).is_none());
+  }
+}

networks/ethereum/schnorr/src/tests/signature.rs

@@ -0,0 +1,33 @@
+use rand_core::OsRng;
+
+use group::ff::Field;
+use k256::Scalar;
+
+use crate::Signature;
+
+#[test]
+fn test_zero_challenge() {
+  assert!(Signature::new(Scalar::ZERO, Scalar::random(&mut OsRng)).is_none());
+}
+
+#[test]
+fn test_signature_serialization() {
+  let c = Scalar::random(&mut OsRng);
+  let s = Scalar::random(&mut OsRng);
+  let sig = Signature::new(c, s).unwrap();
+  assert_eq!(sig.c(), c);
+  assert_eq!(sig.s(), s);
+
+  let sig_bytes = sig.to_bytes();
+  assert_eq!(Signature::from_bytes(sig_bytes).unwrap(), sig);
+
+  {
+    let mut sig_written_bytes = vec![];
+    sig.write(&mut sig_written_bytes).unwrap();
+    assert_eq!(sig_bytes.as_slice(), &sig_written_bytes);
+  }
+
+  let mut sig_read_slice = sig_bytes.as_slice();
+  assert_eq!(Signature::read(&mut sig_read_slice).unwrap(), sig);
+  assert!(sig_read_slice.is_empty());
+}

processor/ethereum/src/primitives/machine.rs

@@ -140,7 +140,7 @@ impl SignatureMachine<Transaction> for ActionSignatureMachine {
     self.machine.complete(shares).map(|signature| {
       let s = signature.s;
       let c = Signature::challenge(signature.R, &self.key, &self.action.message());
-      Transaction(self.action, Signature::new(c, s))
+      Transaction(self.action, Signature::new(c, s).unwrap())
     })
   }
 }