diff --git a/agents/processor/src/processor.rs b/agents/processor/src/processor.rs index 35afd071..73db8963 100644 --- a/agents/processor/src/processor.rs +++ b/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; diff --git a/agents/watcher/CHANGELOG.md b/agents/watcher/CHANGELOG.md index e7e8bbf0..3dd39a17 100644 --- a/agents/watcher/CHANGELOG.md +++ b/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` ### agents@1.1.0 diff --git a/agents/watcher/src/watcher.rs b/agents/watcher/src/watcher.rs index 3d4a58ae..001f809a 100644 --- a/agents/watcher/src/watcher.rs +++ b/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,6 +209,7 @@ pub struct UpdateHandler { rx: mpsc::Receiver, watcher_db: NomadDB, home: Arc, + updater: H160, } impl UpdateHandler { @@ -213,11 +217,13 @@ impl UpdateHandler { rx: mpsc::Receiver, watcher_db: NomadDB, home: Arc, + 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 diff --git a/nomad-core/src/types/update.rs b/nomad-core/src/types/update.rs index a76f2078..8ffe3a16 100644 --- a/nomad-core/src/types/update.rs +++ b/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 ) }