Update how signatures are handled by the Router

processor/ethereum/router/contracts/Router.sol

@@ -126,76 +126,133 @@ contract Router {
   /// @notice Escaping when escape hatch wasn't invoked.
   error EscapeHatchNotInvoked();
 
-  /**
+  /// @dev Updates the Serai key. This does not update `_nextNonce`
-   * @dev Updates the Serai key at the end of the current function. Executing at the end of the
-   *   current function allows verifying a signature with the current key. This does not update
-   *   `_nextNonce`
-   */
   /// @param nonceUpdatedWith The nonce used to update the key
   /// @param newSeraiKey The key updated to
-  modifier updateSeraiKeyAtEndOfFn(uint256 nonceUpdatedWith, bytes32 newSeraiKey) {
+  function _updateSeraiKey(uint256 nonceUpdatedWith, bytes32 newSeraiKey) private {
-    // Run the function itself
-    _;
-
-    // Update the key
     _seraiKey = newSeraiKey;
     emit SeraiKeyUpdated(nonceUpdatedWith, newSeraiKey);
   }
 
   /// @notice The constructor for the relayer
   /// @param initialSeraiKey The initial key for Serai's Ethereum validators
-  constructor(bytes32 initialSeraiKey) updateSeraiKeyAtEndOfFn(0, initialSeraiKey) {
+  constructor(bytes32 initialSeraiKey) {
     // Nonces are incremented by 1 upon account creation, prior to any code execution, per EIP-161
     // This is incompatible with any networks which don't have their nonces start at 0
     _smartContractNonce = 1;
 
-    // We consumed nonce 0 when setting the initial Serai key
+    // Set the Serai key
+    _updateSeraiKey(0, initialSeraiKey);
+
+    // We just consumed nonce 0 when setting the initial Serai key
     _nextNonce = 1;
 
     // We haven't escaped to any address yet
     _escapedTo = address(0);
   }
 
-  /// @dev Verify a signature
+  /**
-  /// @param message The message to pass to the Schnorr contract
+   * @dev
-  /// @param signature The signature by the current key for this message
+   *   Verify a signature of the calldata, placed immediately after the function selector. The calldata
-  function verifySignature(bytes32 message, Signature calldata signature) private {
+   *   should be signed with the nonce taking the place of the signature's commitment to its nonce, and
+   *   the signature solution zeroed.
+   */
+  function verifySignature()
+    private
+    returns (uint256 nonceUsed, bytes memory message, bytes32 messageHash)
+  {
     // If the escape hatch was triggered, reject further signatures
     if (_escapedTo != address(0)) {
       revert EscapeHatchInvoked();
     }
-    // Verify the signature
+
-    if (!Schnorr.verify(_seraiKey, message, signature.c, signature.s)) {
+    message = msg.data;
+    uint256 messageLen = message.length;
+    /*
+      function selector, signature
+
+      This check means we don't read memory, and as we attempt to clear portions, write past it
+      (triggering undefined behavior).
+    */
+    if (messageLen < 68) {
       revert InvalidSignature();
     }
+
+    // Read _nextNonce into memory as the nonce we'll use
+    nonceUsed = _nextNonce;
+
+    // Declare memory to copy the signature out to
+    bytes32 signatureC;
+    bytes32 signatureS;
+
+    // slither-disable-next-line assembly
+    assembly {
+      // Read the signature (placed after the function signature)
+      signatureC := mload(add(message, 36))
+      signatureS := mload(add(message, 68))
+
+      // Overwrite the signature challenge with the nonce
+      mstore(add(message, 36), nonceUsed)
+      // Overwrite the signature response with 0
+      mstore(add(message, 68), 0)
+
+      // Calculate the message hash
+      messageHash := keccak256(add(message, 32), messageLen)
+    }
+
+    // Verify the signature
+    if (!Schnorr.verify(_seraiKey, messageHash, signatureC, signatureS)) {
+      revert InvalidSignature();
+    }
+
     // Set the next nonce
     unchecked {
-      _nextNonce++;
+      _nextNonce = nonceUsed + 1;
+    }
+
+    /*
+      Advance the message past the function selector, enabling decoding the arguments. Ideally, we'd
+      also advance past the signature (to simplify decoding arguments and save some memory). This
+      would transfrom message from:
+
+        message (pointer)
+               v
+               ------------------------------------------------------------
+               | 32-byte length | 4-byte selector | Signature | Arguments |
+               ------------------------------------------------------------
+
+      to:
+
+                 message (pointer)
+                        v
+        ----------------------------------------------
+        | Junk 68 bytes | 32-byte length | Arguments |
+        ----------------------------------------------
+
+      Unfortunately, doing so corrupts the offsets defined within the ABI itself. We settle for a
+      transform to:
+
+                message (pointer)
+                       v
+        ---------------------------------------------------------
+        | Junk 4 bytes | 32-byte length | Signature | Arguments |
+        ---------------------------------------------------------
+    */
+    // slither-disable-next-line assembly
+    assembly {
+      message := add(message, 4)
+      mstore(message, sub(messageLen, 4))
     }
   }
 
   /// @notice Update the key representing Serai's Ethereum validators
   /// @dev This assumes the key is correct. No checks on it are performed
-  /// @param newSeraiKey The key to update to
+  // @param signature The signature by the current key authorizing this update
-  /// @param signature The signature by the current key authorizing this update
+  // @param newSeraiKey The key to update to
-  function updateSeraiKey(bytes32 newSeraiKey, Signature calldata signature)
+  function updateSeraiKey() external {
-    external
+    (uint256 nonceUsed, bytes memory args,) = verifySignature();
-    updateSeraiKeyAtEndOfFn(_nextNonce, newSeraiKey)
+    (,, bytes32 newSeraiKey) = abi.decode(args, (bytes32, bytes32, bytes32));
-  {
+    _updateSeraiKey(nonceUsed, newSeraiKey);
-    /*
-      This DST needs a length prefix as well to prevent DSTs potentially being substrings of each
-      other, yet this is fine for our well-defined, extremely-limited use.
-
-      We don't encode the chain ID as Serai generates independent keys for each integration. If
-      Ethereum L2s are integrated, and they reuse the Ethereum validator set, we would use the
-      existing Serai key yet we'd apply an off-chain derivation scheme to bind it to specific
-      networks. This also lets Serai identify EVMs per however it wants, solving the edge case where
-      two instances of the EVM share a chain ID for whatever horrific reason.
-
-      This uses encodePacked as all items present here are of fixed length.
-    */
-    bytes32 message = keccak256(abi.encodePacked("updateSeraiKey", _nextNonce, newSeraiKey));
-    verifySignature(message, signature);
   }
 
   /// @notice Transfer coins into Serai with an instruction
@@ -357,26 +414,19 @@ contract Router {
    * @dev All `OutInstruction`s in a batch are only for a single coin to simplify handling of the
    *  fee
    */
-  /// @param coin The coin all of these `OutInstruction`s are for
+  // @param signature The signature by the current key for Serai's Ethereum validators
-  /// @param fee The fee to pay (in coin) to the caller for their relaying of this batch
+  // @param coin The coin all of these `OutInstruction`s are for
-  /// @param outs The `OutInstruction`s to act on
+  // @param fee The fee to pay (in coin) to the caller for their relaying of this batch
-  /// @param signature The signature by the current key for Serai's Ethereum validators
+  // @param outs The `OutInstruction`s to act on
   // Each individual call is explicitly metered to ensure there isn't a DoS here
   // slither-disable-next-line calls-loop
-  function execute(
+  function execute() external {
-    address coin,
+    (uint256 nonceUsed, bytes memory args, bytes32 message) = verifySignature();
-    uint256 fee,
+    (,, address coin, uint256 fee, OutInstruction[] memory outs) =
-    OutInstruction[] calldata outs,
+      abi.decode(args, (bytes32, bytes32, address, uint256, OutInstruction[]));
-    Signature calldata signature
-  ) external {
-    // Verify the signature
-    // This uses `encode`, not `encodePacked`, as `outs` is of variable length
-    // TODO: Use a custom encode in verifySignature here with assembly (benchmarking before/after)
-    bytes32 message = keccak256(abi.encode("execute", _nextNonce, coin, fee, outs));
-    verifySignature(message, signature);
 
     // TODO: Also include a bit mask here
-    emit Executed(_nextNonce, message);
+    emit Executed(nonceUsed, message);
 
     /*
       Since we don't have a re-entrancy guard, it is possible for instructions from later batches to
@@ -439,9 +489,14 @@ contract Router {
 
   /// @notice Escapes to a new smart contract
   /// @dev This should be used upon an invariant being reached or new functionality being needed
-  /// @param escapeTo The address to escape to
+  // @param signature The signature by the current key for Serai's Ethereum validators
-  /// @param signature The signature by the current key for Serai's Ethereum validators
+  // @param escapeTo The address to escape to
-  function escapeHatch(address escapeTo, Signature calldata signature) external {
+  function escapeHatch() external {
+    // Verify the signature
+    (, bytes memory args,) = verifySignature();
+
+    (,, address escapeTo) = abi.decode(args, (bytes32, bytes32, address));
+
     if (escapeTo == address(0)) {
       revert InvalidEscapeAddress();
     }
@@ -454,10 +509,6 @@ contract Router {
       revert EscapeHatchInvoked();
     }
 
-    // Verify the signature
-    bytes32 message = keccak256(abi.encodePacked("escapeHatch", _nextNonce, escapeTo));
-    verifySignature(message, signature);
-
     _escapedTo = escapeTo;
     emit EscapeHatch(escapeTo);
   }

processor/ethereum/router/src/lib.rs

@@ -309,26 +309,36 @@ impl Router {
 
   /// Get the message to be signed in order to update the key for Serai.
   pub fn update_serai_key_message(nonce: u64, key: &PublicKey) -> Vec<u8> {
-    ("updateSeraiKey", U256::try_from(nonce).expect("couldn't convert u64 to u256"), key.eth_repr())
+    [
-      .abi_encode_packed()
+      abi::updateSeraiKeyCall::SELECTOR.as_slice(),
+      &(U256::try_from(nonce).unwrap(), U256::ZERO, key.eth_repr()).abi_encode_params(),
+    ]
+    .concat()
   }
 
   /// Construct a transaction to update the key representing Serai.
   pub fn update_serai_key(&self, public_key: &PublicKey, sig: &Signature) -> TxLegacy {
     TxLegacy {
       to: TxKind::Call(self.1),
-      input: abi::updateSeraiKeyCall::new((public_key.eth_repr().into(), sig.into()))
+      input: [
-        .abi_encode()
+        abi::updateSeraiKeyCall::SELECTOR.as_slice(),
-        .into(),
+        &(abi::Signature::from(sig), public_key.eth_repr()).abi_encode_params(),
-      gas_limit: 40748 * 120 / 100,
+      ]
+      .concat()
+      .into(),
+      gas_limit: 40927 * 120 / 100,
       ..Default::default()
     }
   }
 
   /// Get the message to be signed in order to execute a series of `OutInstruction`s.
   pub fn execute_message(nonce: u64, coin: Coin, fee: U256, outs: OutInstructions) -> Vec<u8> {
-    ("execute".to_string(), U256::try_from(nonce).unwrap(), coin.address(), fee, outs.0)
+    [
-      .abi_encode_sequence()
+      abi::executeCall::SELECTOR.as_slice(),
+      &(U256::try_from(nonce).unwrap(), U256::ZERO, coin.address(), fee, outs.0)
+        .abi_encode_params(),
+    ]
+    .concat()
   }
 
   /// Construct a transaction to execute a batch of `OutInstruction`s.
@@ -336,7 +346,12 @@ impl Router {
     let outs_len = outs.0.len();
     TxLegacy {
       to: TxKind::Call(self.1),
-      input: abi::executeCall::new((coin.address(), fee, outs.0, sig.into())).abi_encode().into(),
+      input: [
+        abi::executeCall::SELECTOR.as_slice(),
+        &(abi::Signature::from(sig), coin.address(), fee, outs.0).abi_encode_params(),
+      ]
+      .concat()
+      .into(),
       // TODO
       gas_limit: 100_000 + ((200_000 + 10_000) * u128::try_from(outs_len).unwrap()),
       ..Default::default()

processor/ethereum/router/src/tests/mod.rs

@@ -10,7 +10,7 @@ use alloy_sol_types::SolCall;
 
 use alloy_consensus::TxLegacy;
 
-use alloy_rpc_types_eth::BlockNumberOrTag;
+use alloy_rpc_types_eth::{BlockNumberOrTag, TransactionReceipt};
 use alloy_simple_request_transport::SimpleRequest;
 use alloy_rpc_client::ClientBuilder;
 use alloy_provider::RootProvider;
@@ -154,8 +154,16 @@ async fn test_erc20_in_instruction() {
   todo!("TODO")
 }
 
-async fn publish_outs(key: (Scalar, PublicKey), nonce: u64, coin: Coin, fee: U256, outs: OutInstructions) -> TransactionReceipt {
+async fn publish_outs(
-  let msg = Router::execute_message(nonce, coin, fee, instructions.clone());
+  provider: &RootProvider<SimpleRequest>,
+  router: &Router,
+  key: (Scalar, PublicKey),
+  nonce: u64,
+  coin: Coin,
+  fee: U256,
+  outs: OutInstructions,
+) -> TransactionReceipt {
+  let msg = Router::execute_message(nonce, coin, fee, outs.clone());
 
   let nonce = Scalar::random(&mut OsRng);
   let c = Signature::challenge(ProjectivePoint::GENERATOR * nonce, &key.1, &msg);
@@ -163,10 +171,10 @@ async fn publish_outs(key: (Scalar, PublicKey), nonce: u64, coin: Coin, fee: U25
 
   let sig = Signature::new(c, s).unwrap();
 
-  let mut tx = router.execute(coin, fee, instructions, &sig);
+  let mut tx = router.execute(coin, fee, outs, &sig);
   tx.gas_price = 100_000_000_000u128;
   let tx = ethereum_primitives::deterministically_sign(&tx);
-  ethereum_test_primitives::publish_tx(&provider, tx).await
+  ethereum_test_primitives::publish_tx(provider, tx).await
 }
 
 #[tokio::test]
@@ -182,7 +190,7 @@ async fn test_eth_address_out_instruction() {
   ethereum_test_primitives::fund_account(&provider, router.address(), amount).await;
 
   let instructions = OutInstructions::from([].as_slice());
-  let receipt = publish_outs(key, 1, Coin::Ether, fee, instructions);
+  let receipt = publish_outs(&provider, &router, key, 1, Coin::Ether, fee, instructions).await;
   assert!(receipt.status());
   println!("empty execute used {} gas:", receipt.gas_used);