component: eliminate TOCTOU bug factory

Closes #3690

This renames the previous `check_stateful` method to `check_historical`,
renames `execute` to `check_and_execute`, and by default moves all of the
content of `check_stateful` into `check_and_execute`, unless there's an obvious
and LOCAL reason why it's safe to do so that can be written into the body of
the method.

IMO it's still worth it to have the method in the trait. We may wish to take
advantage of it later, when we can spend more time carefully reasoning about
performance.

Note: we've left behind two versions of the trait, which will be reconciled in
a later commit.

crates/cnidarium-component/src/action_handler.rs

@@ -44,33 +44,45 @@ pub trait ActionHandler {
     /// be run in parallel across all transactions in a block.
     async fn check_stateless(&self, context: Self::CheckStatelessContext) -> Result<()>;
 
-    /// Performs all of this action's stateful validity checks.
+    /// Performs those stateful validity checks that can be performed against a
+    /// historical state snapshot.
     ///
     /// This method provides read access to a snapshot of the `State` prior to
-    /// transaction execution.  Because it does not give write access, it's safe
-    /// to run the `ActionHandler` implementations for all actions in a given
-    /// transaction in parallel.
+    /// transaction execution.  It is intended to be run in parallel across all
+    /// actions within a transaction.
     ///
-    /// As much work as possible should be done in `check_stateful`, as it can
-    /// be run in parallel across all actions within a transaction.
+    /// # Warning
+    ///
+    /// Misuse of this method creates TOCTOU vulnerabilities. Checks performed
+    /// in this method must be valid if they are performed against a _prior_
+    /// state, as another action in the same transaction may execute first and
+    /// change the state.
+    ///
+    /// Checks performed in this phase should have a justification for why they
+    /// are safe to run in parallel with other actions in the same transaction,
+    /// and the default behavior should be to perform checks in
+    /// [`ActionHandler::check_and_execute`].
     ///
     /// # Invariants
     ///
     /// This method should only be called on data that has been checked
     /// with [`ActionHandler::check_stateless`].  This method can be called
     /// before [`Component::begin_block`](crate::Component::begin_block).
-    async fn check_stateful<S: StateRead + 'static>(&self, state: Arc<S>) -> Result<()>;
+    async fn check_historical<S: StateRead + 'static>(&self, _state: Arc<S>) -> Result<()> {
+        // Default behavior: no-op
+        Ok(())
+    }
 
     /// Attempts to execute this action against the provided `state`.
     ///
     /// This method provides read and write access to the `state`. It is
-    /// fallible, so it's possible to perform checks within the `execute`
+    /// fallible, so it's possible to perform checks within the `check_and_execute`
     /// implementation and abort execution on error; the [`StateTransaction`]
     /// mechanism ensures that all writes are correctly discarded.
     ///
     /// Because `execute` must run sequentially, whenever possible, checks
-    /// should be performed in [`ActionHandler::check_stateful`] or
-    /// [`ActionHandler::check_stateless`].  One example of where this is not
+    /// should be performed in [`ActionHandler::check_stateless`], or (more carefully) in
+    /// [`ActionHandler::check_historical`].  One example of where this is not
     /// possible (in fact, the motivating example) is for IBC, where a
     /// transaction may (1) submit a client update and then (2) relay messages
     /// valid relative to the newly updated state.  In this case, the checks for
@@ -81,8 +93,8 @@ pub trait ActionHandler {
     ///
     /// # Invariants
     ///
-    /// This method should only be called immediately after an invocation of
-    /// [`ActionHandler::check_stateful`] on the same transaction.  This method
+    /// This method should only be called after an invocation of
+    /// [`ActionHandler::check_historical`] on the same transaction.  This method
     /// can be called before [`Component::begin_block`](crate::Component::begin_block).
-    async fn execute<S: StateWrite>(&self, state: S) -> Result<()>;
+    async fn check_and_execute<S: StateWrite>(&self, state: S) -> Result<()>;
 }

crates/core/app/src/action_handler.rs

@@ -33,9 +33,9 @@ impl<'a, T: ComponentActionHandler + Sync> ActionHandler for crate::Compat<'a, T
         ComponentActionHandler::check_stateless(self.0, context).await
     }
     async fn check_stateful<S: StateRead + 'static>(&self, state: Arc<S>) -> Result<()> {
-        ComponentActionHandler::check_stateful(self.0, state).await
+        ComponentActionHandler::check_historical(self.0, state).await
     }
     async fn execute<S: StateWrite>(&self, state: S) -> Result<()> {
-        ComponentActionHandler::execute(self.0, state).await
+        ComponentActionHandler::check_and_execute(self.0, state).await
     }
 }

crates/core/app/src/action_handler/actions.rs

@@ -55,22 +55,22 @@ impl ActionHandler for Action {
 
     async fn check_stateful<S: StateRead + 'static>(&self, state: Arc<S>) -> Result<()> {
         match self {
-            Action::Delegate(action) => action.check_stateful(state).await,
-            Action::Undelegate(action) => action.check_stateful(state).await,
-            Action::UndelegateClaim(action) => action.check_stateful(state).await,
-            Action::ValidatorDefinition(action) => action.check_stateful(state).await,
-            Action::DelegatorVote(action) => action.check_stateful(state).await,
-            Action::ValidatorVote(action) => action.check_stateful(state).await,
-            Action::PositionClose(action) => action.check_stateful(state).await,
-            Action::PositionOpen(action) => action.check_stateful(state).await,
-            Action::PositionWithdraw(action) => action.check_stateful(state).await,
+            Action::Delegate(action) => action.check_historical(state).await,
+            Action::Undelegate(action) => action.check_historical(state).await,
+            Action::UndelegateClaim(action) => action.check_historical(state).await,
+            Action::ValidatorDefinition(action) => action.check_historical(state).await,
+            Action::DelegatorVote(action) => action.check_historical(state).await,
+            Action::ValidatorVote(action) => action.check_historical(state).await,
+            Action::PositionClose(action) => action.check_historical(state).await,
+            Action::PositionOpen(action) => action.check_historical(state).await,
+            Action::PositionWithdraw(action) => action.check_historical(state).await,
             Action::ProposalSubmit(action) => action.check_stateful(state).await,
-            Action::ProposalWithdraw(action) => action.check_stateful(state).await,
-            Action::ProposalDepositClaim(action) => action.check_stateful(state).await,
-            Action::Swap(action) => action.check_stateful(state).await,
-            Action::SwapClaim(action) => action.check_stateful(state).await,
-            Action::Spend(action) => action.check_stateful(state).await,
-            Action::Output(action) => action.check_stateful(state).await,
+            Action::ProposalWithdraw(action) => action.check_historical(state).await,
+            Action::ProposalDepositClaim(action) => action.check_historical(state).await,
+            Action::Swap(action) => action.check_historical(state).await,
+            Action::SwapClaim(action) => action.check_historical(state).await,
+            Action::Spend(action) => action.check_historical(state).await,
+            Action::Output(action) => action.check_historical(state).await,
             Action::IbcRelay(action) => {
                 if !state.get_ibc_params().await?.ibc_enabled {
                     anyhow::bail!("transaction contains IBC actions, but IBC is not enabled");
@@ -82,42 +82,42 @@ impl ActionHandler for Action {
                     .check_stateful(state)
                     .await
             }
-            Action::Ics20Withdrawal(action) => action.check_stateful(state).await,
-            Action::CommunityPoolSpend(action) => action.check_stateful(state).await,
-            Action::CommunityPoolOutput(action) => action.check_stateful(state).await,
-            Action::CommunityPoolDeposit(action) => action.check_stateful(state).await,
+            Action::Ics20Withdrawal(action) => action.check_historical(state).await,
+            Action::CommunityPoolSpend(action) => action.check_historical(state).await,
+            Action::CommunityPoolOutput(action) => action.check_historical(state).await,
+            Action::CommunityPoolDeposit(action) => action.check_historical(state).await,
         }
     }
 
     async fn execute<S: StateWrite>(&self, state: S) -> Result<()> {
         match self {
-            Action::Delegate(action) => action.execute(state).await,
-            Action::Undelegate(action) => action.execute(state).await,
-            Action::UndelegateClaim(action) => action.execute(state).await,
-            Action::ValidatorDefinition(action) => action.execute(state).await,
-            Action::DelegatorVote(action) => action.execute(state).await,
-            Action::ValidatorVote(action) => action.execute(state).await,
-            Action::PositionClose(action) => action.execute(state).await,
-            Action::PositionOpen(action) => action.execute(state).await,
-            Action::PositionWithdraw(action) => action.execute(state).await,
+            Action::Delegate(action) => action.check_and_execute(state).await,
+            Action::Undelegate(action) => action.check_and_execute(state).await,
+            Action::UndelegateClaim(action) => action.check_and_execute(state).await,
+            Action::ValidatorDefinition(action) => action.check_and_execute(state).await,
+            Action::DelegatorVote(action) => action.check_and_execute(state).await,
+            Action::ValidatorVote(action) => action.check_and_execute(state).await,
+            Action::PositionClose(action) => action.check_and_execute(state).await,
+            Action::PositionOpen(action) => action.check_and_execute(state).await,
+            Action::PositionWithdraw(action) => action.check_and_execute(state).await,
             Action::ProposalSubmit(action) => action.execute(state).await,
-            Action::ProposalWithdraw(action) => action.execute(state).await,
-            Action::ProposalDepositClaim(action) => action.execute(state).await,
-            Action::Swap(action) => action.execute(state).await,
-            Action::SwapClaim(action) => action.execute(state).await,
-            Action::Spend(action) => action.execute(state).await,
-            Action::Output(action) => action.execute(state).await,
+            Action::ProposalWithdraw(action) => action.check_and_execute(state).await,
+            Action::ProposalDepositClaim(action) => action.check_and_execute(state).await,
+            Action::Swap(action) => action.check_and_execute(state).await,
+            Action::SwapClaim(action) => action.check_and_execute(state).await,
+            Action::Spend(action) => action.check_and_execute(state).await,
+            Action::Output(action) => action.check_and_execute(state).await,
             Action::IbcRelay(action) => {
                 action
                     .clone()
                     .with_handler::<Ics20Transfer, PenumbraHost>()
                     .execute(state)
                     .await
             }
-            Action::Ics20Withdrawal(action) => action.execute(state).await,
-            Action::CommunityPoolSpend(action) => action.execute(state).await,
-            Action::CommunityPoolOutput(action) => action.execute(state).await,
-            Action::CommunityPoolDeposit(action) => action.execute(state).await,
+            Action::Ics20Withdrawal(action) => action.check_and_execute(state).await,
+            Action::CommunityPoolSpend(action) => action.check_and_execute(state).await,
+            Action::CommunityPoolOutput(action) => action.check_and_execute(state).await,
+            Action::CommunityPoolDeposit(action) => action.check_and_execute(state).await,
         }
     }
 }

crates/core/app/tests/spend.rs

@@ -69,10 +69,10 @@ async fn spend_happy_path() -> anyhow::Result<()> {
 
     // 3. Simulate execution of the Spend action
     spend.check_stateless(transaction_context).await?;
-    spend.check_stateful(state.clone()).await?;
+    spend.check_historical(state.clone()).await?;
     let mut state_tx = state.try_begin_transaction().unwrap();
     state_tx.put_mock_source(1u8);
-    spend.execute(&mut state_tx).await?;
+    spend.check_and_execute(&mut state_tx).await?;
     state_tx.apply();
 
     // 4. Execute EndBlock
@@ -253,13 +253,13 @@ async fn spend_duplicate_nullifier_previous_transaction() {
         .await
         .expect("can apply first spend");
     spend
-        .check_stateful(state.clone())
+        .check_historical(state.clone())
         .await
         .expect("can apply first spend");
     let mut state_tx = state.try_begin_transaction().unwrap();
     state_tx.put_mock_source(1u8);
     spend
-        .execute(&mut state_tx)
+        .check_and_execute(&mut state_tx)
         .await
         .expect("should be able to apply first spend");
     state_tx.apply();
@@ -280,10 +280,10 @@ async fn spend_duplicate_nullifier_previous_transaction() {
         .check_stateless(transaction_context)
         .await
         .expect("check stateless should succeed");
-    spend.check_stateful(state.clone()).await.unwrap();
+    spend.check_historical(state.clone()).await.unwrap();
     let mut state_tx = state.try_begin_transaction().unwrap();
     state_tx.put_mock_source(2u8);
-    spend.execute(&mut state_tx).await.unwrap();
+    spend.check_and_execute(&mut state_tx).await.unwrap();
     state_tx.apply();
 }
 

crates/core/app/tests/swap_and_swap_claim.rs

@@ -69,10 +69,10 @@ async fn swap_and_swap_claim() -> anyhow::Result<()> {
     // 3. Simulate execution of the Swap action
 
     swap.check_stateless(()).await?;
-    swap.check_stateful(state.clone()).await?;
+    swap.check_historical(state.clone()).await?;
     let mut state_tx = state.try_begin_transaction().unwrap();
     state_tx.put_mock_source(1u8);
-    swap.execute(&mut state_tx).await?;
+    swap.check_and_execute(&mut state_tx).await?;
     state_tx.apply();
 
     // 4. Execute EndBlock (where the swap is actually executed)
@@ -127,10 +127,10 @@ async fn swap_and_swap_claim() -> anyhow::Result<()> {
     .context();
 
     claim.check_stateless(context.clone()).await?;
-    claim.check_stateful(state.clone()).await?;
+    claim.check_historical(state.clone()).await?;
     let mut state_tx = state.try_begin_transaction().unwrap();
     state_tx.put_mock_source(2u8);
-    claim.execute(&mut state_tx).await?;
+    claim.check_and_execute(&mut state_tx).await?;
     state_tx.apply();
 
     Ok(())
@@ -184,10 +184,10 @@ async fn swap_claim_duplicate_nullifier_previous_transaction() {
     // 3. Simulate execution of the Swap action
 
     swap.check_stateless(()).await.unwrap();
-    swap.check_stateful(state.clone()).await.unwrap();
+    swap.check_historical(state.clone()).await.unwrap();
     let mut state_tx = state.try_begin_transaction().unwrap();
     state_tx.put_mock_source(1u8);
-    swap.execute(&mut state_tx).await.unwrap();
+    swap.check_and_execute(&mut state_tx).await.unwrap();
     state_tx.apply();
 
     // 4. Execute EndBlock (where the swap is actually executed)
@@ -241,10 +241,10 @@ async fn swap_claim_duplicate_nullifier_previous_transaction() {
     .context();
 
     claim.check_stateless(context.clone()).await.unwrap();
-    claim.check_stateful(state.clone()).await.unwrap();
+    claim.check_historical(state.clone()).await.unwrap();
     let mut state_tx = state.try_begin_transaction().unwrap();
     state_tx.put_mock_source(2u8);
-    claim.execute(&mut state_tx).await.unwrap();
+    claim.check_and_execute(&mut state_tx).await.unwrap();
     state_tx.apply();
 
     // 8. Now form a second SwapClaim action attempting to claim the outputs again.
@@ -259,7 +259,7 @@ async fn swap_claim_duplicate_nullifier_previous_transaction() {
     let claim = claim_plan.swap_claim(&test_keys::FULL_VIEWING_KEY, &swap_auth_path);
 
     // 9. Execute the second SwapClaim action - the test should panic here
-    claim.check_stateful(state.clone()).await.unwrap();
+    claim.check_historical(state.clone()).await.unwrap();
 }
 
 #[tokio::test]
@@ -304,10 +304,10 @@ async fn swap_with_nonzero_fee() -> anyhow::Result<()> {
     // 3. Simulate execution of the Swap action
 
     swap.check_stateless(()).await?;
-    swap.check_stateful(state.clone()).await?;
+    swap.check_historical(state.clone()).await?;
     let mut state_tx = state.try_begin_transaction().unwrap();
     state_tx.put_mock_source(1u8);
-    swap.execute(&mut state_tx).await?;
+    swap.check_and_execute(&mut state_tx).await?;
     state_tx.apply();
 
     // 4. Execute EndBlock (where the swap is actually executed)

crates/core/component/community-pool/src/component/action_handler/community_pool_deposit.rs

@@ -1,8 +1,6 @@
-use std::sync::Arc;
-
 use anyhow::Result;
 use async_trait::async_trait;
-use cnidarium::{StateRead, StateWrite};
+use cnidarium::StateWrite;
 use cnidarium_component::ActionHandler;
 
 use crate::{component::StateWriteExt as _, CommunityPoolDeposit};
@@ -15,12 +13,7 @@ impl ActionHandler for CommunityPoolDeposit {
         Ok(())
     }
 
-    async fn check_stateful<S: StateRead + 'static>(&self, _state: Arc<S>) -> Result<()> {
-        // Any deposit into the Community Pool is valid.
-        Ok(())
-    }
-
-    async fn execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
+    async fn check_and_execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
         state.community_pool_deposit(self.value).await
     }
 }

crates/core/component/community-pool/src/component/action_handler/community_pool_output.rs

@@ -1,8 +1,6 @@
-use std::sync::Arc;
-
 use anyhow::Result;
 use async_trait::async_trait;
-use cnidarium::{StateRead, StateWrite};
+use cnidarium::StateWrite;
 use cnidarium_component::ActionHandler;
 use penumbra_sct::CommitmentSource;
 use penumbra_shielded_pool::component::NoteManager;
@@ -17,12 +15,7 @@ impl ActionHandler for CommunityPoolOutput {
         Ok(())
     }
 
-    async fn check_stateful<S: StateRead + 'static>(&self, _state: Arc<S>) -> Result<()> {
-        // Any output from the Community Pool is valid (it's just a transparent output).
-        Ok(())
-    }
-
-    async fn execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
+    async fn check_and_execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
         // Executing a Community Pool output is just minting a note to the recipient of the output.
         state
             .mint_note(

crates/core/component/community-pool/src/component/action_handler/community_pool_spend.rs

@@ -1,8 +1,6 @@
-use std::sync::Arc;
-
 use anyhow::Result;
 use async_trait::async_trait;
-use cnidarium::{StateRead, StateWrite};
+use cnidarium::StateWrite;
 use cnidarium_component::ActionHandler;
 
 use crate::{component::StateWriteExt as _, CommunityPoolSpend};
@@ -16,13 +14,7 @@ impl ActionHandler for CommunityPoolSpend {
         Ok(())
     }
 
-    async fn check_stateful<S: StateRead + 'static>(&self, _state: Arc<S>) -> Result<()> {
-        // Instead of checking here, we just check during execution, which will fail if we try to
-        // overdraw the Community Pool.
-        Ok(())
-    }
-
-    async fn execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
+    async fn check_and_execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
         // This will fail if we try to overdraw the Community Pool, so we can never spend more than we have.
         state.community_pool_withdraw(self.value).await
     }

crates/core/component/dex/src/component/action_handler/position/close.rs

@@ -1,8 +1,6 @@
-use std::sync::Arc;
-
 use anyhow::Result;
 use async_trait::async_trait;
-use cnidarium::{StateRead, StateWrite};
+use cnidarium::StateWrite;
 use cnidarium_component::ActionHandler;
 use penumbra_proto::StateWriteProto as _;
 
@@ -18,11 +16,7 @@ impl ActionHandler for PositionClose {
         Ok(())
     }
 
-    async fn check_stateful<S: StateRead + 'static>(&self, _state: Arc<S>) -> Result<()> {
-        Ok(())
-    }
-
-    async fn execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
+    async fn check_and_execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
         // We don't want to actually close the position here, because otherwise
         // the economic effects could depend on intra-block ordering, and we'd
         // lose the ability to do block-scoped JIT liquidity, where a single