Prestwich/double update sig check

agents/processor/src/processor.rs

@@ -225,7 +225,7 @@ impl Replica {
         Ok(Flow::Advance)
     }
 
-    #[instrument(err, level = "info", skip(self), fields(self = %self, domain = message.message.destination, nonce = message.message.nonce, leaf_index = message.leaf_index, leaf = ?message.message.to_leaf()))]
+    #[instrument(err, level = "info", skip(self, message), fields(self = %self, domain = message.message.destination, nonce = message.message.nonce, leaf_index = message.leaf_index, leaf = ?message.message.to_leaf()))]
     /// Dispatch a message for processing. If the message is already proven, process only.
     async fn process(&self, message: CommittedMessage, proof: NomadProof) -> Result<()> {
         use nomad_core::Replica;

agents/watcher/CHANGELOG.md

@@ -2,6 +2,9 @@
 
 ### Unreleased
 
+- update handler now errors if incoming updates have an unexpected updater
+- double-update routine now checks that both updates are signed by the same
+  updater
 - Add English description to XCM error log, change to use `Display`
 
 ### [email protected]

agents/watcher/src/watcher.rs

@@ -1,8 +1,11 @@
 use async_trait::async_trait;
-use color_eyre::{eyre::bail, Report, Result};
+use color_eyre::{
+    eyre::{bail, ensure},
+    Report, Result,
+};
 use thiserror::Error;
 
-use ethers::core::types::H256;
+use ethers::{core::types::H256, prelude::H160};
 use futures_util::future::{join, join_all, select_all};
 use prometheus::{IntGauge, IntGaugeVec};
 use std::{collections::HashMap, fmt::Display, sync::Arc, time::Duration};
@@ -12,7 +15,7 @@ use tokio::{
     task::JoinHandle,
     time::sleep,
 };
-use tracing::{error, info, info_span, instrument::Instrumented, Instrument};
+use tracing::{error, info, info_span, instrument::Instrumented, warn, Instrument};
 
 use nomad_base::{
     cancel_task, AgentCore, BaseError, CachingHome, ConnectionManagers, NomadAgent, NomadDB,
@@ -206,18 +209,21 @@ pub struct UpdateHandler {
     rx: mpsc::Receiver<SignedUpdate>,
     watcher_db: NomadDB,
     home: Arc<CachingHome>,
+    updater: H160,
 }
 
 impl UpdateHandler {
     pub fn new(
         rx: mpsc::Receiver<SignedUpdate>,
         watcher_db: NomadDB,
         home: Arc<CachingHome>,
+        updater: H160,
     ) -> Self {
         Self {
             rx,
             watcher_db,
             home,
+            updater,
         }
     }
 
@@ -231,7 +237,27 @@ impl UpdateHandler {
             .expect("!db_get")
         {
             Some(existing) => {
-                if existing.update.new_root != new_root {
+                let existing_signer = existing.recover();
+                let new_signer = update.recover();
+                // if a signature verification failed. We consider this not a
+                // double update
+                if existing_signer.is_err() || new_signer.is_err() {
+                    warn!(
+                        existing = %existing,
+                        new = %update,
+                        existing_signer = ?existing_signer,
+                        new_signer = ? new_signer,
+                        "Signature verification on update failed"
+                    );
+                    return Ok(());
+                }
+
+                let existing_signer = existing_signer.unwrap();
+                let new_signer = new_signer.unwrap();
+
+                // ensure both new roots are different, and the signer is the
+                // same. we perform this check in addition
+                if existing.update.new_root != new_root && existing_signer == new_signer {
                     error!(
                         "UpdateHandler detected double update! Existing: {:?}. Double: {:?}.",
                         &existing, &update
@@ -268,6 +294,14 @@ impl UpdateHandler {
                 let update = update.unwrap();
                 let old_root = update.update.previous_root;
 
+                // This check may appear redundant with the check in
+                // `check_double_update` that signers match, however,
+                // this is
+                ensure!(
+                    update.verify(self.updater).is_ok(),
+                    "Handling update signed by another updater. Hint: This agent may misconfigured, or the updater may have rotated while this agent was running"
+                );
+
                 if old_root == self.home.committed_root().await? {
                     // It is okay if tx reverts
                     let _ = self.home.update(&update).await;
@@ -355,9 +389,10 @@ impl Watcher {
         let updates_inspected_for_double = self.updates_inspected_for_double.clone();
 
         tokio::spawn(async move {
+            let updater = home.updater().await?;
             // Spawn update handler
             let (tx, rx) = mpsc::channel(200);
-            let handler = UpdateHandler::new(rx, watcher_db, home.clone()).spawn();
+            let handler = UpdateHandler::new(rx, watcher_db, home.clone(), updater.into()).spawn();
 
             // For each replica, spawn polling and history syncing tasks
             info!("Spawning replica watch and sync tasks...");
@@ -888,6 +923,7 @@ mod test {
                 "1111111111111111111111111111111111111111111111111111111111111111"
                     .parse()
                     .unwrap();
+            let updater = signer.address();
 
             let first_root = H256::from([1; 32]);
             let second_root = H256::from([2; 32]);
@@ -957,6 +993,7 @@ mod test {
                 rx,
                 watcher_db: nomad_db,
                 home,
+                updater,
             };
 
             handler

nomad-core/src/types/update.rs

@@ -25,7 +25,7 @@ impl std::fmt::Display for Update {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         write!(
             f,
-            "Update(domain {} moved from {} to {})",
+            "Update(domain {} moved from {:?} to {:?})",
             self.home_domain, self.previous_root, self.new_root
         )
     }