Merge branch 'grarco/replay-protection-improvements' (#1905)

.changelog/unreleased/improvements/1905-replay-protection-improvements.md

@@ -0,0 +1,2 @@
+- Improved replay protection for invalid transactions.
+  ([\#1905](https://github.com/anoma/namada/pull/1905))

core/src/ledger/tx_env.rs

@@ -64,4 +64,7 @@ pub trait TxEnv: StorageRead + StorageWrite {
         &self,
         event_type: impl AsRef<str>,
     ) -> Result<Vec<IbcEvent>, storage_api::Error>;
+
+    /// Set the sentinel for an invalid section commitment
+    fn set_commitment_sentinel(&mut self);
 }

core/src/proto/types.rs

@@ -26,7 +26,9 @@ use sha2::{Digest, Sha256};
 use thiserror::Error;
 
 use super::generated::types;
-use crate::ledger::gas::{GasMetering, VpGasMeter, VERIFY_TX_SIG_GAS_COST};
+use crate::ledger::gas::{
+    self, GasMetering, VpGasMeter, VERIFY_TX_SIG_GAS_COST,
+};
 use crate::ledger::storage::{KeccakHasher, Sha256Hasher, StorageHasher};
 #[cfg(any(feature = "tendermint", feature = "tendermint-abcipp"))]
 use crate::tendermint_proto::abci::ResponseDeliverTx;
@@ -74,8 +76,8 @@ pub enum Error {
     InvalidJSONDeserialization(String),
     #[error("The wrapper signature is invalid.")]
     InvalidWrapperSignature,
-    #[error("Signature verification went out of gas")]
-    OutOfGas,
+    #[error("Signature verification went out of gas: {0}")]
+    OutOfGas(gas::Error),
 }
 
 pub type Result<T> = std::result::Result<T, Error>;
@@ -593,7 +595,7 @@ impl Signature {
                         if let Some(meter) = gas_meter {
                             meter
                                 .consume(VERIFY_TX_SIG_GAS_COST)
-                                .map_err(|_| VerifySigError::OutOfGas)?;
+                                .map_err(VerifySigError::OutOfGas)?;
                         }
                         common::SigScheme::verify_signature(
                             &pk,
@@ -618,7 +620,7 @@ impl Signature {
                         if let Some(meter) = gas_meter {
                             meter
                                 .consume(VERIFY_TX_SIG_GAS_COST)
-                                .map_err(|_| VerifySigError::OutOfGas)?;
+                                .map_err(VerifySigError::OutOfGas)?;
                         }
                         common::SigScheme::verify_signature(
                             pk,
@@ -1448,8 +1450,8 @@ impl Tx {
                             gas_meter,
                         )
                         .map_err(|e| {
-                            if let VerifySigError::OutOfGas = e {
-                                Error::OutOfGas
+                            if let VerifySigError::OutOfGas(inner) = e {
+                                Error::OutOfGas(inner)
                             } else {
                                 Error::InvalidSectionSignature(
                                     "found invalid signature.".to_string(),

core/src/types/key/mod.rs

@@ -124,8 +124,8 @@ pub enum VerifySigError {
     MissingData,
     #[error("Signature belongs to a different scheme from the public key.")]
     MismatchedScheme,
-    #[error("Signature verification went out of gas")]
-    OutOfGas,
+    #[error("Signature verification went out of gas: {0}")]
+    OutOfGas(crate::ledger::gas::Error),
 }
 
 #[allow(missing_docs)]

core/src/types/transaction/mod.rs

@@ -80,6 +80,8 @@ pub struct VpsResult {
     pub gas_used: VpsGas,
     /// Errors occurred in any of the VPs, if any
     pub errors: Vec<(Address, String)>,
+    /// Sentinel to signal an invalid transaction signature
+    pub invalid_sig: bool,
 }
 
 impl fmt::Display for TxResult {
@@ -166,6 +168,31 @@ impl TxType {
     }
 }
 
+/// Sentinel used in transactions to signal events that require special
+/// replay protection handling back to the protocol.
+#[derive(Debug, Default)]
+pub enum TxSentinel {
+    /// No action required
+    #[default]
+    None,
+    /// Exceeded gas limit
+    OutOfGas,
+    /// Found invalid commtiment to one of the transaction's sections
+    InvalidCommitment,
+}
+
+impl TxSentinel {
+    /// Set the sentinel for an out of gas error
+    pub fn set_out_of_gas(&mut self) {
+        *self = Self::OutOfGas
+    }
+
+    /// Set the sentinel for an invalid section commitment error
+    pub fn set_invalid_commitment(&mut self) {
+        *self = Self::InvalidCommitment
+    }
+}
+
 #[cfg(test)]
 mod test_process_tx {
     use super::*;

core/src/types/validity_predicate.rs

@@ -17,3 +17,38 @@ pub struct EvalVp {
     /// The input for the `eval`ed VP
     pub input: Tx,
 }
+
+/// Sentinel used in validity predicates to signal events that require special
+/// replay protection handling back to the protocol.
+#[derive(Debug, Default)]
+pub enum VpSentinel {
+    /// No action required
+    #[default]
+    None,
+    /// Exceeded gas limit
+    OutOfGas,
+    /// Found invalid transaction signature
+    InvalidSignature,
+}
+
+impl VpSentinel {
+    /// Check if the Vp ran out of gas
+    pub fn is_out_of_gas(&self) -> bool {
+        matches!(self, Self::OutOfGas)
+    }
+
+    /// Check if the Vp found an invalid signature
+    pub fn is_invalid_signature(&self) -> bool {
+        matches!(self, Self::InvalidSignature)
+    }
+
+    /// Set the sentinel for an out of gas error
+    pub fn set_out_of_gas(&mut self) {
+        *self = Self::OutOfGas
+    }
+
+    /// Set the sentinel for an invalid signature error
+    pub fn set_invalid_signature(&mut self) {
+        *self = Self::InvalidSignature
+    }
+}

shared/src/ledger/native_vp/mod.rs

@@ -12,6 +12,7 @@ use std::collections::BTreeSet;
 use borsh::BorshDeserialize;
 use eyre::WrapErr;
 pub use namada_core::ledger::vp_env::VpEnv;
+use namada_core::types::validity_predicate::VpSentinel;
 
 use super::storage_api::{self, ResultExt, StorageRead};
 use super::vp_host_fns;
@@ -66,6 +67,8 @@ where
     pub iterators: RefCell<PrefixIterators<'a, DB>>,
     /// VP gas meter.
     pub gas_meter: RefCell<VpGasMeter>,
+    /// Errors sentinel
+    pub sentinel: RefCell<VpSentinel>,
     /// Read-only access to the storage.
     pub storage: &'a Storage<DB, H>,
     /// Read-only access to the write log.
@@ -136,6 +139,7 @@ where
             address,
             iterators: RefCell::new(PrefixIterators::default()),
             gas_meter: RefCell::new(gas_meter),
+            sentinel: RefCell::new(VpSentinel::default()),
             storage,
             write_log,
             tx,
@@ -182,6 +186,7 @@ where
             self.ctx.storage,
             self.ctx.write_log,
             key,
+            &mut self.ctx.sentinel.borrow_mut(),
         )
         .into_storage_result()
     }
@@ -195,6 +200,7 @@ where
             self.ctx.storage,
             self.ctx.write_log,
             key,
+            &mut self.ctx.sentinel.borrow_mut(),
         )
         .into_storage_result()
     }
@@ -208,6 +214,7 @@ where
             self.ctx.write_log,
             self.ctx.storage,
             prefix,
+            &mut self.ctx.sentinel.borrow_mut(),
         )
         .into_storage_result()
     }
@@ -219,8 +226,12 @@ where
         &'iter self,
         iter: &mut Self::PrefixIter<'iter>,
     ) -> Result<Option<(String, Vec<u8>)>, storage_api::Error> {
-        vp_host_fns::iter_next::<DB>(&mut self.ctx.gas_meter.borrow_mut(), iter)
-            .into_storage_result()
+        vp_host_fns::iter_next::<DB>(
+            &mut self.ctx.gas_meter.borrow_mut(),
+            iter,
+            &mut self.ctx.sentinel.borrow_mut(),
+        )
+        .into_storage_result()
     }
 
     fn get_chain_id(&self) -> Result<String, storage_api::Error> {
@@ -273,6 +284,7 @@ where
             self.ctx.storage,
             self.ctx.write_log,
             key,
+            &mut self.ctx.sentinel.borrow_mut(),
         )
         .into_storage_result()
     }
@@ -286,6 +298,7 @@ where
             self.ctx.storage,
             self.ctx.write_log,
             key,
+            &mut self.ctx.sentinel.borrow_mut(),
         )
         .into_storage_result()
     }
@@ -299,6 +312,7 @@ where
             self.ctx.write_log,
             self.ctx.storage,
             prefix,
+            &mut self.ctx.sentinel.borrow_mut(),
         )
         .into_storage_result()
     }
@@ -310,8 +324,12 @@ where
         &'iter self,
         iter: &mut Self::PrefixIter<'iter>,
     ) -> Result<Option<(String, Vec<u8>)>, storage_api::Error> {
-        vp_host_fns::iter_next::<DB>(&mut self.ctx.gas_meter.borrow_mut(), iter)
-            .into_storage_result()
+        vp_host_fns::iter_next::<DB>(
+            &mut self.ctx.gas_meter.borrow_mut(),
+            iter,
+            &mut self.ctx.sentinel.borrow_mut(),
+        )
+        .into_storage_result()
     }
 
     fn get_chain_id(&self) -> Result<String, storage_api::Error> {
@@ -372,6 +390,7 @@ where
             &mut self.gas_meter.borrow_mut(),
             self.write_log,
             key,
+            &mut self.sentinel.borrow_mut(),
         )
         .map(|data| data.and_then(|t| T::try_from_slice(&t[..]).ok()))
         .into_storage_result()
@@ -385,6 +404,7 @@ where
             &mut self.gas_meter.borrow_mut(),
             self.write_log,
             key,
+            &mut self.sentinel.borrow_mut(),
         )
         .into_storage_result()
     }
@@ -393,6 +413,7 @@ where
         vp_host_fns::get_chain_id(
             &mut self.gas_meter.borrow_mut(),
             self.storage,
+            &mut self.sentinel.borrow_mut(),
         )
         .into_storage_result()
     }
@@ -401,6 +422,7 @@ where
         vp_host_fns::get_block_height(
             &mut self.gas_meter.borrow_mut(),
             self.storage,
+            &mut self.sentinel.borrow_mut(),
         )
         .into_storage_result()
     }
@@ -413,6 +435,7 @@ where
             &mut self.gas_meter.borrow_mut(),
             self.storage,
             height,
+            &mut self.sentinel.borrow_mut(),
         )
         .into_storage_result()
     }
@@ -421,6 +444,7 @@ where
         vp_host_fns::get_block_hash(
             &mut self.gas_meter.borrow_mut(),
             self.storage,
+            &mut self.sentinel.borrow_mut(),
         )
         .into_storage_result()
     }
@@ -429,6 +453,7 @@ where
         vp_host_fns::get_block_epoch(
             &mut self.gas_meter.borrow_mut(),
             self.storage,
+            &mut self.sentinel.borrow_mut(),
         )
         .into_storage_result()
     }
@@ -437,6 +462,7 @@ where
         vp_host_fns::get_tx_index(
             &mut self.gas_meter.borrow_mut(),
             self.tx_index,
+            &mut self.sentinel.borrow_mut(),
         )
         .into_storage_result()
     }
@@ -445,6 +471,7 @@ where
         vp_host_fns::get_native_token(
             &mut self.gas_meter.borrow_mut(),
             self.storage,
+            &mut self.sentinel.borrow_mut(),
         )
         .into_storage_result()
     }
@@ -470,6 +497,7 @@ where
             self.write_log,
             self.storage,
             prefix,
+            &mut self.sentinel.borrow_mut(),
         )
         .into_storage_result()
     }
@@ -501,6 +529,7 @@ where
                 self.storage,
                 self.write_log,
                 &mut self.gas_meter.borrow_mut(),
+                &mut self.sentinel.borrow_mut(),
                 self.tx,
                 self.tx_index,
                 &mut iterators,
@@ -543,8 +572,12 @@ where
     }
 
     fn get_tx_code_hash(&self) -> Result<Option<Hash>, storage_api::Error> {
-        vp_host_fns::get_tx_code_hash(&mut self.gas_meter.borrow_mut(), self.tx)
-            .into_storage_result()
+        vp_host_fns::get_tx_code_hash(
+            &mut self.gas_meter.borrow_mut(),
+            self.tx,
+            &mut self.sentinel.borrow_mut(),
+        )
+        .into_storage_result()
     }
 
     fn read_pre<T: borsh::BorshDeserialize>(