fix(sequencer)!: fix TOCTOU issues by merging check and execution (#1332

)

## Summary
Introduces `ActionHandler::check_and_execute`, replacing
`ActionHandler::check_stateful` and `ActionHandler::execute`.

## Background
Zelic found that separating execution into `check_stateful` and
`execute` lead to time-of-check-vs-time-of-use risks: while
`check_stateful` for two actions `A` and `B` might each pass, the
execution of action `A` might change the state such that a subsequent
execution of action `B` would now fail - or worse, lead to invalid
state.

This patch follows Penumbra (see issues linked at the bottom) in merging
them into one atomic operation (atomic in the sense that
`<Action>::check_and_execute` are run sequentially).

There is also a `ActionHandler::check_historic` trait method, which
however is currently not used. It is left for future work to go through
the individual checks and move them to `check_historic`, where
applicable (for example, checking address prefixes as long as changing
these is not possible after chain init).

## Changes
- change `ActionHandler` trait to merge `check_stateful` and `execute`
into `check_and_execute`.
- inject a transaction's signer into the ephemeral object store, setting
before and after a transaction is executed. Necessary because this
follows the `cnidarium_component::ActionHandler` convention, but also
allows simplifying
- remove the notion of bech32m addresses from many state writes and
reads: the prefix only matters at the boundary, not inside the system

## Testing
All tests were updated and pass.

NOTE: a useful test would be to craft a problematic transaction that
would be rejected with the newest change. However, crafting and
executing such a transaction so that it is rejected by the current
sequencer but leads to incorrect is left to a follow-up.

## Breaking Changelist
While no snapshots guarding against breaking app state were triggered,
this is still a breaking change: if supplied with a problematic payload,
a sequencer node without this patch will reach a different (and invalid)
state compared a node with the present patch.

## Related Issues

Original Penumbra issues and fix:
penumbra-zone/penumbra#3960
penumbra-zone/penumbra#3960

Closes #1318

---------

Co-authored-by: Fraser Hutchison <[email protected]>

crates/astria-sequencer/Cargo.toml

@@ -28,7 +28,6 @@ borsh = { version = "1", features = ["derive"] }
 cnidarium = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.78.0", features = [
   "metrics",
 ] }
-cnidarium-component = { git = "https://github.com/penumbra-zone/penumbra.git", tag = "v0.78.0" }
 ibc-proto = { version = "0.41.0", features = ["server"] }
 matchit = "0.7.2"
 priority-queue = "2.0.2"

crates/astria-sequencer/src/accounts/action.rs

@@ -4,31 +4,123 @@ use anyhow::{
     Result,
 };
 use astria_core::{
-    primitive::v1::Address,
+    primitive::v1::ADDRESS_LEN,
     protocol::transaction::v1alpha1::action::TransferAction,
     Protobuf,
 };
-use tracing::instrument;
+use cnidarium::{
+    StateRead,
+    StateWrite,
+};
 
+use super::AddressBytes;
 use crate::{
     accounts::{
-        self,
         StateReadExt as _,
+        StateWriteExt as _,
+    },
+    address::StateReadExt as _,
+    app::ActionHandler,
+    assets::{
+        StateReadExt as _,
+        StateWriteExt as _,
     },
-    address,
-    assets,
     bridge::StateReadExt as _,
-    transaction::action_handler::ActionHandler,
+    transaction::StateReadExt as _,
 };
 
-pub(crate) async fn transfer_check_stateful<S>(
+#[async_trait::async_trait]
+impl ActionHandler for TransferAction {
+    async fn check_stateless(&self) -> Result<()> {
+        Ok(())
+    }
+
+    async fn check_and_execute<S: StateWrite>(&self, state: S) -> Result<()> {
+        let from = state
+            .get_current_source()
+            .expect("transaction source must be present in state when executing an action")
+            .address_bytes();
+
+        ensure!(
+            state
+                .get_bridge_account_rollup_id(from)
+                .await
+                .context("failed to get bridge account rollup id")?
+                .is_none(),
+            "cannot transfer out of bridge account; BridgeUnlock must be used",
+        );
+
+        check_transfer(self, from, &state).await?;
+        execute_transfer(self, from, state).await?;
+
+        Ok(())
+    }
+}
+
+pub(crate) async fn execute_transfer<S: StateWrite>(
+    action: &TransferAction,
+    from: [u8; ADDRESS_LEN],
+    mut state: S,
+) -> anyhow::Result<()> {
+    let fee = state
+        .get_transfer_base_fee()
+        .await
+        .context("failed to get transfer base fee")?;
+    state
+        .get_and_increase_block_fees(&action.fee_asset, fee, TransferAction::full_name())
+        .await
+        .context("failed to add to block fees")?;
+
+    // if fee payment asset is same asset as transfer asset, deduct fee
+    // from same balance as asset transferred
+    if action.asset.to_ibc_prefixed() == action.fee_asset.to_ibc_prefixed() {
+        // check_stateful should have already checked this arithmetic
+        let payment_amount = action
+            .amount
+            .checked_add(fee)
+            .expect("transfer amount plus fee should not overflow");
+
+        state
+            .decrease_balance(from, &action.asset, payment_amount)
+            .await
+            .context("failed decreasing `from` account balance")?;
+        state
+            .increase_balance(action.to, &action.asset, action.amount)
+            .await
+            .context("failed increasing `to` account balance")?;
+    } else {
+        // otherwise, just transfer the transfer asset and deduct fee from fee asset balance
+        // later
+        state
+            .decrease_balance(from, &action.asset, action.amount)
+            .await
+            .context("failed decreasing `from` account balance")?;
+        state
+            .increase_balance(action.to, &action.asset, action.amount)
+            .await
+            .context("failed increasing `to` account balance")?;
+
+        // deduct fee from fee asset balance
+        state
+            .decrease_balance(from, &action.fee_asset, fee)
+            .await
+            .context("failed decreasing `from` account balance for fee payment")?;
+    }
+    Ok(())
+}
+
+pub(crate) async fn check_transfer<S, TAddress>(
     action: &TransferAction,
+    from: TAddress,
     state: &S,
-    from: Address,
 ) -> Result<()>
 where
-    S: accounts::StateReadExt + assets::StateReadExt + 'static,
+    S: StateRead,
+    TAddress: AddressBytes,
 {
+    state.ensure_base_prefix(&action.to).await.context(
+        "failed ensuring that the destination address matches the permitted base prefix",
+    )?;
     ensure!(
         state
             .is_allowed_fee_asset(&action.fee_asset)
@@ -44,7 +136,7 @@ where
     let transfer_asset = action.asset.clone();
 
     let from_fee_balance = state
-        .get_account_balance(from, &action.fee_asset)
+        .get_account_balance(&from, &action.fee_asset)
         .await
         .context("failed getting `from` account balance for fee payment")?;
 
@@ -80,84 +172,3 @@ where
 
     Ok(())
 }
-
-#[async_trait::async_trait]
-impl ActionHandler for TransferAction {
-    async fn check_stateless(&self) -> Result<()> {
-        Ok(())
-    }
-
-    async fn check_stateful<S>(&self, state: &S, from: Address) -> Result<()>
-    where
-        S: accounts::StateReadExt + address::StateReadExt + 'static,
-    {
-        state.ensure_base_prefix(&self.to).await.context(
-            "failed ensuring that the destination address matches the permitted base prefix",
-        )?;
-        ensure!(
-            state
-                .get_bridge_account_rollup_id(&from)
-                .await
-                .context("failed to get bridge account rollup id")?
-                .is_none(),
-            "cannot transfer out of bridge account; BridgeUnlock must be used",
-        );
-
-        transfer_check_stateful(self, state, from)
-            .await
-            .context("stateful transfer check failed")
-    }
-
-    #[instrument(skip_all)]
-    async fn execute<S>(&self, state: &mut S, from: Address) -> Result<()>
-    where
-        S: accounts::StateWriteExt + assets::StateWriteExt,
-    {
-        let fee = state
-            .get_transfer_base_fee()
-            .await
-            .context("failed to get transfer base fee")?;
-        state
-            .get_and_increase_block_fees(&self.fee_asset, fee, Self::full_name())
-            .await
-            .context("failed to add to block fees")?;
-
-        // if fee payment asset is same asset as transfer asset, deduct fee
-        // from same balance as asset transferred
-        if self.asset.to_ibc_prefixed() == self.fee_asset.to_ibc_prefixed() {
-            // check_stateful should have already checked this arithmetic
-            let payment_amount = self
-                .amount
-                .checked_add(fee)
-                .expect("transfer amount plus fee should not overflow");
-
-            state
-                .decrease_balance(from, &self.asset, payment_amount)
-                .await
-                .context("failed decreasing `from` account balance")?;
-            state
-                .increase_balance(self.to, &self.asset, self.amount)
-                .await
-                .context("failed increasing `to` account balance")?;
-        } else {
-            // otherwise, just transfer the transfer asset and deduct fee from fee asset balance
-            // later
-            state
-                .decrease_balance(from, &self.asset, self.amount)
-                .await
-                .context("failed decreasing `from` account balance")?;
-            state
-                .increase_balance(self.to, &self.asset, self.amount)
-                .await
-                .context("failed increasing `to` account balance")?;
-
-            // deduct fee from fee asset balance
-            state
-                .decrease_balance(from, &self.fee_asset, fee)
-                .await
-                .context("failed decreasing `from` account balance for fee payment")?;
-        }
-
-        Ok(())
-    }
-}

crates/astria-sequencer/src/accounts/mod.rs

@@ -3,7 +3,61 @@ pub(crate) mod component;
 pub(crate) mod query;
 mod state_ext;
 
+use astria_core::{
+    crypto::{
+        SigningKey,
+        VerificationKey,
+    },
+    primitive::v1::{
+        Address,
+        ADDRESS_LEN,
+    },
+    protocol::transaction::v1alpha1::SignedTransaction,
+};
 pub(crate) use state_ext::{
     StateReadExt,
     StateWriteExt,
 };
+
+pub(crate) trait AddressBytes: Send + Sync {
+    fn address_bytes(&self) -> [u8; ADDRESS_LEN];
+}
+
+impl AddressBytes for Address {
+    fn address_bytes(&self) -> [u8; ADDRESS_LEN] {
+        self.bytes()
+    }
+}
+
+impl AddressBytes for [u8; ADDRESS_LEN] {
+    fn address_bytes(&self) -> [u8; ADDRESS_LEN] {
+        *self
+    }
+}
+
+impl AddressBytes for SignedTransaction {
+    fn address_bytes(&self) -> [u8; ADDRESS_LEN] {
+        self.address_bytes()
+    }
+}
+
+impl AddressBytes for SigningKey {
+    fn address_bytes(&self) -> [u8; ADDRESS_LEN] {
+        self.address_bytes()
+    }
+}
+
+impl AddressBytes for VerificationKey {
+    fn address_bytes(&self) -> [u8; ADDRESS_LEN] {
+        self.address_bytes()
+    }
+}
+
+impl<'a, T> AddressBytes for &'a T
+where
+    T: AddressBytes,
+{
+    fn address_bytes(&self) -> [u8; ADDRESS_LEN] {
+        (*self).address_bytes()
+    }
+}