From d1fba2f5c4dd7e89eaec4c5a8608847eb002d694 Mon Sep 17 00:00:00 2001 From: Lily Johnson Date: Wed, 30 Oct 2024 14:05:11 +0700 Subject: [PATCH 1/2] rework with AuthenticationHandler instead of ActionHandler --- crates/astria-core/src/protocol/abci.rs | 2 + .../astria-sequencer/src/authority/action.rs | 32 -- .../astria-sequencer/src/authorization/mod.rs | 390 ++++++++++++++++++ .../src/authorization/tests.rs | 341 +++++++++++++++ .../src/bridge/bridge_sudo_change_action.rs | 68 +-- .../src/bridge/bridge_unlock_action.rs | 102 ----- crates/astria-sequencer/src/bridge/mod.rs | 2 +- crates/astria-sequencer/src/fees/action.rs | 25 -- .../src/ibc/ibc_relayer_change.rs | 20 +- crates/astria-sequencer/src/ibc/mod.rs | 2 +- crates/astria-sequencer/src/lib.rs | 1 + crates/astria-sequencer/src/metrics.rs | 15 + .../src/service/mempool/mod.rs | 25 +- .../src/service/mempool/tests.rs | 46 ++- .../astria-sequencer/src/transaction/mod.rs | 25 +- 15 files changed, 832 insertions(+), 264 deletions(-) create mode 100644 crates/astria-sequencer/src/authorization/mod.rs create mode 100644 crates/astria-sequencer/src/authorization/tests.rs diff --git a/crates/astria-core/src/protocol/abci.rs b/crates/astria-core/src/protocol/abci.rs index 4047ae679f..2a4f23284e 100644 --- a/crates/astria-core/src/protocol/abci.rs +++ b/crates/astria-core/src/protocol/abci.rs @@ -26,6 +26,7 @@ impl AbciErrorCode { pub const NONCE_TAKEN: Self = Self(unsafe { NonZeroU32::new_unchecked(15) }); pub const ACCOUNT_SIZE_LIMIT: Self = Self(unsafe { NonZeroU32::new_unchecked(16) }); pub const PARKED_FULL: Self = Self(unsafe { NonZeroU32::new_unchecked(17) }); + pub const AUTHORIZATION_FAILED: Self = Self(unsafe { NonZeroU32::new_unchecked(18) }); } impl AbciErrorCode { @@ -64,6 +65,7 @@ impl AbciErrorCode { "the account has reached the maximum number of parked transactions".into() } Self::PARKED_FULL => "the mempool is out of space for more parked transactions".into(), + Self::AUTHORIZATION_FAILED => "the transaction failed authorization checks".into(), Self(other) => { format!("invalid error code {other}: should be unreachable (this is a bug)") } diff --git a/crates/astria-sequencer/src/authority/action.rs b/crates/astria-sequencer/src/authority/action.rs index 78d69733eb..029dad77a1 100644 --- a/crates/astria-sequencer/src/authority/action.rs +++ b/crates/astria-sequencer/src/authority/action.rs @@ -19,7 +19,6 @@ use crate::{ StateWriteExt as _, }, ibc::StateWriteExt as _, - transaction::StateReadExt as _, }; #[async_trait::async_trait] @@ -29,17 +28,6 @@ impl ActionHandler for ValidatorUpdate { } async fn check_and_execute(&self, mut state: S) -> Result<()> { - let from = state - .get_transaction_context() - .expect("transaction source must be present in state when executing an action") - .address_bytes(); - // ensure signer is the valid `sudo` key in state - let sudo_address = state - .get_sudo_address() - .await - .wrap_err("failed to get sudo address from state")?; - ensure!(sudo_address == from, "signer is not the sudo key"); - // ensure that we're not removing the last validator or a validator // that doesn't exist, these both cause issues in cometBFT if self.power == 0 { @@ -80,20 +68,10 @@ impl ActionHandler for SudoAddressChange { /// check that the signer of the transaction is the current sudo address, /// as only that address can change the sudo address async fn check_and_execute(&self, mut state: S) -> Result<()> { - let from = state - .get_transaction_context() - .expect("transaction source must be present in state when executing an action") - .address_bytes(); state .ensure_base_prefix(&self.new_address) .await .wrap_err("desired new sudo address has an unsupported prefix")?; - // ensure signer is the valid `sudo` key in state - let sudo_address = state - .get_sudo_address() - .await - .wrap_err("failed to get sudo address from state")?; - ensure!(sudo_address == from, "signer is not the sudo key"); state .put_sudo_address(self.new_address) .wrap_err("failed to put sudo address in state")?; @@ -108,20 +86,10 @@ impl ActionHandler for IbcSudoChange { } async fn check_and_execute(&self, mut state: S) -> Result<()> { - let from = state - .get_transaction_context() - .expect("transaction source must be present in state when executing an action") - .address_bytes(); state .ensure_base_prefix(&self.new_address) .await .wrap_err("desired new ibc sudo address has an unsupported prefix")?; - // ensure signer is the valid `sudo` key in state - let sudo_address = state - .get_sudo_address() - .await - .wrap_err("failed to get sudo address from state")?; - ensure!(sudo_address == from, "signer is not the sudo key"); state .put_ibc_sudo_address(self.new_address) .wrap_err("failed to put ibc sudo address in state")?; diff --git a/crates/astria-sequencer/src/authorization/mod.rs b/crates/astria-sequencer/src/authorization/mod.rs new file mode 100644 index 0000000000..f9952c24bd --- /dev/null +++ b/crates/astria-sequencer/src/authorization/mod.rs @@ -0,0 +1,390 @@ +use astria_core::{ + protocol::transaction::v1::{ + action::{ + BridgeLock, + BridgeSudoChange, + BridgeUnlock, + FeeAssetChange, + FeeChange, + IbcRelayerChange, + IbcSudoChange, + Ics20Withdrawal, + InitBridgeAccount, + RollupDataSubmission, + SudoAddressChange, + Transfer, + ValidatorUpdate, + }, + Action, + Transaction, + }, + Protobuf, +}; +use astria_eyre::eyre::{ + self, + bail, + ensure, + WrapErr as _, +}; +use cnidarium::StateRead; +use penumbra_ibc::IbcRelayWithHandlers; +use tracing::{ + instrument, + Level, +}; + +use crate::{ + accounts::AddressBytes, + authority::StateReadExt as _, + bridge::state_ext::StateReadExt as _, + ibc::{ + host_interface::AstriaHost, + state_ext::StateReadExt as _, + }, +}; + +#[cfg(test)] +mod tests; + +#[async_trait::async_trait] +pub(crate) trait AuthorizationHandler { + async fn check_authorization( + &self, + state: &S, + from: &T, + ) -> eyre::Result<()>; +} + +#[async_trait::async_trait] +impl AuthorizationHandler for Transfer { + #[instrument(skip_all, err)] + async fn check_authorization( + &self, + _state: &S, + _from: &T, + ) -> eyre::Result<()> { + Ok(()) + } +} + +#[async_trait::async_trait] +impl AuthorizationHandler for RollupDataSubmission { + #[instrument(skip_all, err)] + async fn check_authorization( + &self, + _state: &S, + _from: &T, + ) -> eyre::Result<()> { + Ok(()) + } +} + +#[async_trait::async_trait] +impl AuthorizationHandler for BridgeLock { + #[instrument(skip_all, err)] + async fn check_authorization( + &self, + _state: &S, + _from: &T, + ) -> eyre::Result<()> { + Ok(()) + } +} + +#[async_trait::async_trait] +impl AuthorizationHandler for BridgeUnlock { + #[instrument(skip_all, err)] + async fn check_authorization( + &self, + state: &S, + from: &T, + ) -> eyre::Result<()> { + // check that the sender of this tx is the authorized withdrawer for the bridge account + let Some(withdrawer_address) = state + .get_bridge_account_withdrawer_address(&self.bridge_address) + .await + .wrap_err("failed to get bridge account withdrawer address")? + else { + bail!("bridge account does not have an associated withdrawer address"); + }; + + ensure!( + withdrawer_address == *from.address_bytes(), + "unauthorized to unlock bridge account", + ); + Ok(()) + } +} + +#[async_trait::async_trait] +impl AuthorizationHandler for InitBridgeAccount { + #[instrument(skip_all, err)] + async fn check_authorization( + &self, + _state: &S, + _from: &T, + ) -> eyre::Result<()> { + Ok(()) + } +} + +#[async_trait::async_trait] +impl AuthorizationHandler + for IbcRelayWithHandlers +{ + #[instrument(skip_all, err)] + async fn check_authorization( + &self, + state: &S, + from: &T, + ) -> eyre::Result<()> { + check_ibc_authorization(state, from).await + } +} + +async fn check_ibc_authorization( + state: &S, + from: &T, +) -> eyre::Result<()> { + ensure!( + state + .is_ibc_relayer(*from.address_bytes()) + .await + .wrap_err("failed to check if address is IBC relayer")?, + "only IBC sudo address can execute IBC actions" + ); + Ok(()) +} + +#[async_trait::async_trait] +impl AuthorizationHandler for IbcRelayerChange { + #[instrument(skip_all, err)] + async fn check_authorization( + &self, + state: &S, + from: &T, + ) -> eyre::Result<()> { + let ibc_sudo_address = state + .get_ibc_sudo_address() + .await + .wrap_err("failed to get IBC sudo address")?; + ensure!( + ibc_sudo_address == *from.address_bytes(), + "unauthorized address for IBC relayer change" + ); + Ok(()) + } +} + +#[async_trait::async_trait] +impl AuthorizationHandler for IbcSudoChange { + #[instrument(skip_all, err)] + async fn check_authorization( + &self, + state: &S, + from: &T, + ) -> eyre::Result<()> { + // ensure signer is the valid `sudo` key in state + let sudo_address = state + .get_sudo_address() + .await + .wrap_err("failed to get sudo address from state")?; + ensure!( + sudo_address == *from.address_bytes(), + "signer is not the sudo key" + ); + Ok(()) + } +} + +#[async_trait::async_trait] +impl AuthorizationHandler for SudoAddressChange { + #[instrument(skip_all, err)] + async fn check_authorization( + &self, + state: &S, + from: &T, + ) -> eyre::Result<()> { + // ensure signer is the valid `sudo` key in state + let sudo_address = state + .get_sudo_address() + .await + .wrap_err("failed to get sudo address from state")?; + ensure!( + sudo_address == *from.address_bytes(), + "signer is not the sudo key" + ); + Ok(()) + } +} + +#[async_trait::async_trait] +impl AuthorizationHandler for ValidatorUpdate { + #[instrument(skip_all, err)] + async fn check_authorization( + &self, + state: &S, + from: &T, + ) -> eyre::Result<()> { + // ensure signer is the valid `sudo` key in state + let sudo_address = state + .get_sudo_address() + .await + .wrap_err("failed to get sudo address from state")?; + ensure!( + sudo_address == *from.address_bytes(), + "signer is not the sudo key" + ); + Ok(()) + } +} + +#[async_trait::async_trait] +impl AuthorizationHandler for FeeChange { + #[instrument(skip_all, err)] + async fn check_authorization( + &self, + state: &S, + from: &T, + ) -> eyre::Result<()> { + // ensure signer is the valid `sudo` key in state + let sudo_address = state + .get_sudo_address() + .await + .wrap_err("failed to get sudo address from state")?; + ensure!( + sudo_address == *from.address_bytes(), + "signer is not the sudo key" + ); + Ok(()) + } +} + +#[async_trait::async_trait] +impl AuthorizationHandler for FeeAssetChange { + #[instrument(skip_all, err)] + async fn check_authorization( + &self, + state: &S, + from: &T, + ) -> eyre::Result<()> { + // ensure signer is the valid `sudo` key in state + let sudo_address = state + .get_sudo_address() + .await + .wrap_err("failed to get sudo address from state")?; + ensure!( + sudo_address == *from.address_bytes(), + "signer is not the sudo key" + ); + Ok(()) + } +} + +#[async_trait::async_trait] +impl AuthorizationHandler for BridgeSudoChange { + #[instrument(skip_all, err)] + async fn check_authorization( + &self, + state: &S, + from: &T, + ) -> eyre::Result<()> { + // check that the sender of this tx is the authorized sudo address for the bridge account + let Some(sudo_address) = state + .get_bridge_account_sudo_address(&self.bridge_address) + .await + .wrap_err("failed to get bridge account sudo address")? + else { + // TODO: if the sudo address is unset, should we still allow this action + // if the sender if the bridge address itself? + bail!("bridge account does not have an associated sudo address"); + }; + + ensure!( + sudo_address == *from.address_bytes(), + "unauthorized for bridge sudo change action", + ); + Ok(()) + } +} + +#[async_trait::async_trait] +impl AuthorizationHandler for Ics20Withdrawal { + #[instrument(skip_all, err)] + async fn check_authorization( + &self, + _state: &S, + _from: &T, + ) -> eyre::Result<()> { + Ok(()) + } +} + +#[instrument(skip_all, err(level = Level::WARN))] +async fn check_authorization( + act: &T, + state: &S, + from: &F, +) -> eyre::Result<()> { + act.check_authorization(state, from).await?; + Ok(()) +} + +#[async_trait::async_trait] +impl AuthorizationHandler for Transaction { + async fn check_authorization( + &self, + state: &S, + from: &T, + ) -> eyre::Result<()> { + for action in self.actions() { + match action { + Action::Transfer(transfer) => { + transfer.check_authorization(state, from).await?; + } + Action::RollupDataSubmission(rollup_data_submission) => { + rollup_data_submission + .check_authorization(state, from) + .await?; + } + Action::BridgeLock(bridge_lock) => { + bridge_lock.check_authorization(state, from).await?; + } + Action::BridgeUnlock(bridge_unlock) => { + bridge_unlock.check_authorization(state, from).await?; + } + Action::InitBridgeAccount(init_bridge_account) => { + init_bridge_account.check_authorization(state, from).await?; + } + Action::BridgeSudoChange(bridge_sudo_change) => { + bridge_sudo_change.check_authorization(state, from).await?; + } + Action::Ics20Withdrawal(ics20_withdrawal) => { + ics20_withdrawal.check_authorization(state, from).await?; + } + Action::Ibc(_) => { + check_ibc_authorization(state, from).await?; + } + Action::IbcRelayerChange(ibc_relayer_change) => { + ibc_relayer_change.check_authorization(state, from).await?; + } + Action::IbcSudoChange(ibc_sudo_change) => { + ibc_sudo_change.check_authorization(state, from).await?; + } + Action::SudoAddressChange(sudo_address_change) => { + sudo_address_change.check_authorization(state, from).await?; + } + Action::ValidatorUpdate(validator_update) => { + validator_update.check_authorization(state, from).await?; + } + Action::FeeChange(fee_change) => { + fee_change.check_authorization(state, from).await?; + } + Action::FeeAssetChange(fee_asset_change) => { + fee_asset_change.check_authorization(state, from).await?; + } + } + } + Ok(()) + } +} diff --git a/crates/astria-sequencer/src/authorization/tests.rs b/crates/astria-sequencer/src/authorization/tests.rs new file mode 100644 index 0000000000..e86d7697a8 --- /dev/null +++ b/crates/astria-sequencer/src/authorization/tests.rs @@ -0,0 +1,341 @@ +use astria_core::{ + primitive::v1::asset, + protocol::{ + fees::v1::TransferFeeComponents, + transaction::v1::action::{ + BridgeSudoChange, + BridgeUnlock, + FeeAssetChange, + FeeChange, + IbcRelayerChange, + IbcSudoChange, + SudoAddressChange, + ValidatorUpdate, + }, + }, +}; +use cnidarium::StateDelta; + +use crate::{ + address::StateWriteExt as _, + app::{ + benchmark_and_test_utils::{ + initialize_app_with_storage, + ALICE_ADDRESS, + BOB_ADDRESS, + }, + test_utils::get_alice_signing_key, + }, + authority::StateWriteExt as _, + authorization::AuthorizationHandler, + benchmark_and_test_utils::{ + assert_eyre_error, + astria_address, + astria_address_from_hex_string, + ASTRIA_PREFIX, + }, + bridge::StateWriteExt as _, + ibc::StateWriteExt as _, +}; + +fn test_asset() -> asset::Denom { + "test".parse().unwrap() +} + +#[tokio::test] +async fn ensure_sudo_change_action_is_authorized() { + let (_, storage) = initialize_app_with_storage(None, vec![]).await; + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + state + .put_sudo_address(astria_address_from_hex_string(ALICE_ADDRESS)) + .expect("failed to write sudo address"); + + let sudo_change = SudoAddressChange { + new_address: astria_address_from_hex_string(BOB_ADDRESS), + }; + + assert_eyre_error( + &sudo_change + .check_authorization(&state, &astria_address_from_hex_string(BOB_ADDRESS)) + .await + .unwrap_err(), + "signer is not the sudo key", + ); + assert!( + sudo_change + .check_authorization(&state, &astria_address_from_hex_string(ALICE_ADDRESS)) + .await + .is_ok(), + "correct signer should be ok" + ); +} + +#[tokio::test] +async fn ensure_ibc_sudo_change_action_is_authorized() { + let (_, storage) = initialize_app_with_storage(None, vec![]).await; + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + state + .put_sudo_address(astria_address_from_hex_string(ALICE_ADDRESS)) + .expect("failed to write sudo address"); + + let ibc_sudo_change = IbcSudoChange { + new_address: astria_address_from_hex_string(BOB_ADDRESS), + }; + + assert_eyre_error( + &ibc_sudo_change + .check_authorization(&state, &astria_address_from_hex_string(BOB_ADDRESS)) + .await + .unwrap_err(), + "signer is not the sudo key", + ); + assert!( + ibc_sudo_change + .check_authorization(&state, &astria_address_from_hex_string(ALICE_ADDRESS)) + .await + .is_ok(), + "correct signer should be ok" + ); +} + +#[tokio::test] +async fn ensure_validator_update_action_is_authorized() { + let (_, storage) = initialize_app_with_storage(None, vec![]).await; + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + state + .put_sudo_address(astria_address_from_hex_string(ALICE_ADDRESS)) + .expect("failed to write sudo address"); + + let validator_update = ValidatorUpdate { + verification_key: get_alice_signing_key().verification_key(), + power: 1, + }; + + assert_eyre_error( + &validator_update + .check_authorization(&state, &astria_address_from_hex_string(BOB_ADDRESS)) + .await + .unwrap_err(), + "signer is not the sudo key", + ); + assert!( + validator_update + .check_authorization(&state, &astria_address_from_hex_string(ALICE_ADDRESS)) + .await + .is_ok(), + "correct signer should be ok" + ); +} + +#[tokio::test] +async fn ensure_fee_asset_change_action_is_authorized() { + let (_, storage) = initialize_app_with_storage(None, vec![]).await; + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + state + .put_sudo_address(astria_address_from_hex_string(ALICE_ADDRESS)) + .expect("failed to write sudo address"); + + let fee_asset_change = FeeAssetChange::Addition(test_asset()); + + assert_eyre_error( + &fee_asset_change + .check_authorization(&state, &astria_address_from_hex_string(BOB_ADDRESS)) + .await + .unwrap_err(), + "signer is not the sudo key", + ); + assert!( + fee_asset_change + .check_authorization(&state, &astria_address_from_hex_string(ALICE_ADDRESS)) + .await + .is_ok(), + "correct signer should be ok" + ); +} + +#[tokio::test] +async fn ensure_fee_change_action_is_authorized() { + let (_, storage) = initialize_app_with_storage(None, vec![]).await; + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + state + .put_sudo_address(astria_address_from_hex_string(ALICE_ADDRESS)) + .expect("failed to write sudo address"); + + let fee_change = FeeChange::Transfer(TransferFeeComponents { + base: 10, + multiplier: 0, + }); + + assert_eyre_error( + &fee_change + .check_authorization(&state, &astria_address_from_hex_string(BOB_ADDRESS)) + .await + .unwrap_err(), + "signer is not the sudo key", + ); + assert!( + fee_change + .check_authorization(&state, &astria_address_from_hex_string(ALICE_ADDRESS)) + .await + .is_ok(), + "correct signer should be ok" + ); +} + +#[tokio::test] +async fn ensure_ibc_relayer_change_action_is_authorized() { + let (_, storage) = initialize_app_with_storage(None, vec![]).await; + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + state + .put_ibc_sudo_address(astria_address_from_hex_string(ALICE_ADDRESS)) + .expect("failed to write IBC relayer address"); + + let ibc_relayer_change = + IbcRelayerChange::Addition(astria_address_from_hex_string(BOB_ADDRESS)); + + assert_eyre_error( + &ibc_relayer_change + .check_authorization(&state, &astria_address_from_hex_string(BOB_ADDRESS)) + .await + .unwrap_err(), + "unauthorized address for IBC relayer change", + ); + assert!( + ibc_relayer_change + .check_authorization(&state, &astria_address_from_hex_string(ALICE_ADDRESS)) + .await + .is_ok(), + "correct signer should be ok" + ); +} + +#[tokio::test] +async fn bridge_unlock_no_withdrawer_address_fails() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + state.put_base_prefix(ASTRIA_PREFIX.to_string()).unwrap(); + + let asset = test_asset(); + let transfer_amount = 100; + + let to_address = astria_address(&[2; 20]); + let bridge_address = astria_address(&[3; 20]); + state + .put_bridge_account_ibc_asset(&bridge_address, &asset) + .unwrap(); + + let bridge_unlock: BridgeUnlock = BridgeUnlock { + to: to_address, + amount: transfer_amount, + fee_asset: asset.clone(), + memo: String::new(), + bridge_address, + rollup_block_number: 1, + rollup_withdrawal_event_id: "a-rollup-defined-hash".to_string(), + }; + + // missing withdrawer address should cause failure + assert_eyre_error( + &bridge_unlock + .check_authorization(&state, &astria_address_from_hex_string(BOB_ADDRESS)) + .await + .unwrap_err(), + "bridge account does not have an associated withdrawer address", + ); +} + +#[tokio::test] +async fn ensure_bridge_unlock_withdrawer_address_is_authorized() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + state.put_base_prefix(ASTRIA_PREFIX.to_string()).unwrap(); + + let asset = test_asset(); + let transfer_amount = 100; + + let bridge_address = astria_address(&[3; 20]); + let withdrawer_address = astria_address_from_hex_string(ALICE_ADDRESS); + state + .put_bridge_account_withdrawer_address(&bridge_address, withdrawer_address) + .unwrap(); + + let bridge_unlock = BridgeUnlock { + to: astria_address(&[2; 20]), + amount: transfer_amount, + fee_asset: asset, + memo: String::new(), + bridge_address, + rollup_block_number: 1, + rollup_withdrawal_event_id: "a-rollup-defined-hash".to_string(), + }; + + // invalid sender, doesn't match action's bridge account's withdrawer, should fail + assert_eyre_error( + &bridge_unlock + .check_authorization(&state, &astria_address_from_hex_string(BOB_ADDRESS)) + .await + .unwrap_err(), + "unauthorized to unlock bridge account", + ); + + assert!( + bridge_unlock + .check_authorization(&state, &withdrawer_address) + .await + .is_ok(), + "correct withdrawer should be ok" + ); +} + +#[tokio::test] +async fn ensure_bridge_account_sudo_change_is_authorized() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + let asset = test_asset(); + + let bridge_address = astria_address(&[99; 20]); + let sudo_address = astria_address_from_hex_string(ALICE_ADDRESS); + state + .put_bridge_account_sudo_address(&bridge_address, sudo_address) + .unwrap(); + + let action = BridgeSudoChange { + bridge_address, + new_sudo_address: None, + new_withdrawer_address: None, + fee_asset: asset.clone(), + }; + + assert_eyre_error( + &action + .check_authorization(&state, &astria_address_from_hex_string(BOB_ADDRESS)) + .await + .unwrap_err(), + "unauthorized for bridge sudo change action", + ); + assert!( + action + .check_authorization(&state, &sudo_address) + .await + .is_ok(), + "correct signer should be ok" + ); +} diff --git a/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs index 655694abfc..feb6ac0e65 100644 --- a/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs @@ -1,7 +1,5 @@ use astria_core::protocol::transaction::v1::action::BridgeSudoChange; use astria_eyre::eyre::{ - bail, - ensure, Result, WrapErr as _, }; @@ -10,11 +8,7 @@ use cnidarium::StateWrite; use crate::{ address::StateReadExt as _, app::ActionHandler, - bridge::state_ext::{ - StateReadExt as _, - StateWriteExt as _, - }, - transaction::StateReadExt as _, + bridge::state_ext::StateWriteExt as _, }; #[async_trait::async_trait] impl ActionHandler for BridgeSudoChange { @@ -23,10 +17,6 @@ impl ActionHandler for BridgeSudoChange { } async fn check_and_execute(&self, mut state: S) -> Result<()> { - let from = state - .get_transaction_context() - .expect("transaction source must be present in state when executing an action") - .address_bytes(); state .ensure_base_prefix(&self.bridge_address) .await @@ -44,22 +34,6 @@ impl ActionHandler for BridgeSudoChange { .wrap_err("failed check for base prefix of new withdrawer address")?; } - // check that the sender of this tx is the authorized sudo address for the bridge account - let Some(sudo_address) = state - .get_bridge_account_sudo_address(&self.bridge_address) - .await - .wrap_err("failed to get bridge account sudo address")? - else { - // TODO: if the sudo address is unset, should we still allow this action - // if the sender if the bridge address itself? - bail!("bridge account does not have an associated sudo address"); - }; - - ensure!( - sudo_address == from, - "unauthorized for bridge sudo change action", - ); - if let Some(sudo_address) = self.new_sudo_address { state .put_bridge_account_sudo_address(&self.bridge_address, sudo_address) @@ -95,6 +69,7 @@ mod tests { astria_address, ASTRIA_PREFIX, }, + bridge::StateReadExt as _, fees::StateWriteExt as _, transaction::{ StateWriteExt as _, @@ -106,45 +81,6 @@ mod tests { "test".parse().unwrap() } - #[tokio::test] - async fn fails_with_unauthorized_if_signer_is_not_sudo_address() { - let storage = cnidarium::TempStorage::new().await.unwrap(); - let snapshot = storage.latest_snapshot(); - let mut state = StateDelta::new(snapshot); - - state.put_transaction_context(TransactionContext { - address_bytes: [1; 20], - transaction_id: TransactionId::new([0; 32]), - source_action_index: 0, - }); - state.put_base_prefix(ASTRIA_PREFIX.to_string()).unwrap(); - - let asset = test_asset(); - state.put_allowed_fee_asset(&asset).unwrap(); - - let bridge_address = astria_address(&[99; 20]); - let sudo_address = astria_address(&[98; 20]); - state - .put_bridge_account_sudo_address(&bridge_address, sudo_address) - .unwrap(); - - let action = BridgeSudoChange { - bridge_address, - new_sudo_address: None, - new_withdrawer_address: None, - fee_asset: asset.clone(), - }; - - assert!( - action - .check_and_execute(state) - .await - .unwrap_err() - .to_string() - .contains("unauthorized for bridge sudo change action") - ); - } - #[tokio::test] async fn executes() { let storage = cnidarium::TempStorage::new().await.unwrap(); diff --git a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs index 52fcb2b407..8163c66dce 100644 --- a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs @@ -3,7 +3,6 @@ use astria_core::protocol::transaction::v1::action::{ Transfer, }; use astria_eyre::eyre::{ - bail, ensure, Result, WrapErr as _, @@ -21,7 +20,6 @@ use crate::{ StateReadExt as _, StateWriteExt as _, }, - transaction::StateReadExt as _, }; #[async_trait::async_trait] @@ -46,10 +44,6 @@ impl ActionHandler for BridgeUnlock { } async fn check_and_execute(&self, mut state: S) -> Result<()> { - let from = state - .get_transaction_context() - .expect("transaction source must be present in state when executing an action") - .address_bytes(); state .ensure_base_prefix(&self.to) .await @@ -64,20 +58,6 @@ impl ActionHandler for BridgeUnlock { .await .wrap_err("failed to get bridge's asset id, must be a bridge account")?; - // check that the sender of this tx is the authorized withdrawer for the bridge account - let Some(withdrawer_address) = state - .get_bridge_account_withdrawer_address(&self.bridge_address) - .await - .wrap_err("failed to get bridge account withdrawer address")? - else { - bail!("bridge account does not have an associated withdrawer address"); - }; - - ensure!( - withdrawer_address == from, - "unauthorized to unlock bridge account", - ); - let transfer_action = Transfer { to: self.to, asset: asset.into(), @@ -136,88 +116,6 @@ mod tests { "test".parse().unwrap() } - #[tokio::test] - async fn fails_if_bridge_account_has_no_withdrawer_address() { - let storage = cnidarium::TempStorage::new().await.unwrap(); - let snapshot = storage.latest_snapshot(); - let mut state = StateDelta::new(snapshot); - - state.put_transaction_context(TransactionContext { - address_bytes: [1; 20], - transaction_id: TransactionId::new([0; 32]), - source_action_index: 0, - }); - state.put_base_prefix(ASTRIA_PREFIX.to_string()).unwrap(); - - let asset = test_asset(); - let transfer_amount = 100; - - let to_address = astria_address(&[2; 20]); - let bridge_address = astria_address(&[3; 20]); - state - .put_bridge_account_ibc_asset(&bridge_address, &asset) - .unwrap(); - - let bridge_unlock = BridgeUnlock { - to: to_address, - amount: transfer_amount, - fee_asset: asset.clone(), - memo: String::new(), - bridge_address, - rollup_block_number: 1, - rollup_withdrawal_event_id: "a-rollup-defined-hash".to_string(), - }; - - // invalid sender, doesn't match action's `from`, should fail - assert_eyre_error( - &bridge_unlock.check_and_execute(state).await.unwrap_err(), - "bridge account does not have an associated withdrawer address", - ); - } - - #[tokio::test] - async fn fails_if_withdrawer_is_not_signer() { - let storage = cnidarium::TempStorage::new().await.unwrap(); - let snapshot = storage.latest_snapshot(); - let mut state = StateDelta::new(snapshot); - - state.put_transaction_context(TransactionContext { - address_bytes: [1; 20], - transaction_id: TransactionId::new([0; 32]), - source_action_index: 0, - }); - state.put_base_prefix(ASTRIA_PREFIX.to_string()).unwrap(); - - let asset = test_asset(); - let transfer_amount = 100; - - let to_address = astria_address(&[2; 20]); - let bridge_address = astria_address(&[3; 20]); - let withdrawer_address = astria_address(&[4; 20]); - state - .put_bridge_account_withdrawer_address(&bridge_address, withdrawer_address) - .unwrap(); - state - .put_bridge_account_ibc_asset(&bridge_address, &asset) - .unwrap(); - - let bridge_unlock = BridgeUnlock { - to: to_address, - amount: transfer_amount, - fee_asset: asset, - memo: String::new(), - bridge_address, - rollup_block_number: 1, - rollup_withdrawal_event_id: "a-rollup-defined-hash".to_string(), - }; - - // invalid sender, doesn't match action's bridge account's withdrawer, should fail - assert_eyre_error( - &bridge_unlock.check_and_execute(state).await.unwrap_err(), - "unauthorized to unlock bridge account", - ); - } - #[tokio::test] async fn execute_with_duplicated_withdrawal_event_id() { let storage = cnidarium::TempStorage::new().await.unwrap(); diff --git a/crates/astria-sequencer/src/bridge/mod.rs b/crates/astria-sequencer/src/bridge/mod.rs index 5aec65349a..5725ef8ccd 100644 --- a/crates/astria-sequencer/src/bridge/mod.rs +++ b/crates/astria-sequencer/src/bridge/mod.rs @@ -3,7 +3,7 @@ mod bridge_sudo_change_action; mod bridge_unlock_action; pub(crate) mod init_bridge_account_action; pub(crate) mod query; -mod state_ext; +pub(crate) mod state_ext; pub(crate) mod storage; pub(crate) use state_ext::{ diff --git a/crates/astria-sequencer/src/fees/action.rs b/crates/astria-sequencer/src/fees/action.rs index 927eea5ec9..12b15a3128 100644 --- a/crates/astria-sequencer/src/fees/action.rs +++ b/crates/astria-sequencer/src/fees/action.rs @@ -13,12 +13,10 @@ use tokio::pin; use crate::{ app::ActionHandler, - authority::StateReadExt as _, fees::{ StateReadExt as _, StateWriteExt as _, }, - transaction::StateReadExt as _, }; #[async_trait::async_trait] @@ -30,17 +28,6 @@ impl ActionHandler for FeeChange { /// check that the signer of the transaction is the current sudo address, /// as only that address can change the fee async fn check_and_execute(&self, mut state: S) -> eyre::Result<()> { - let from = state - .get_transaction_context() - .expect("transaction source must be present in state when executing an action") - .address_bytes(); - // ensure signer is the valid `sudo` key in state - let sudo_address = state - .get_sudo_address() - .await - .wrap_err("failed to get sudo address from state")?; - ensure!(sudo_address == from, "signer is not the sudo key"); - match self { Self::Transfer(fees) => state .put_transfer_fees(*fees) @@ -95,18 +82,6 @@ impl ActionHandler for FeeAssetChange { } async fn check_and_execute(&self, mut state: S) -> eyre::Result<()> { - let from = state - .get_transaction_context() - .expect("transaction source must be present in state when executing an action") - .address_bytes(); - let authority_sudo_address = state - .get_sudo_address() - .await - .wrap_err("failed to get authority sudo address")?; - ensure!( - authority_sudo_address == from, - "unauthorized address for fee asset change" - ); match self { FeeAssetChange::Addition(asset) => { state diff --git a/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs b/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs index 0c277b412e..e084a6b58b 100644 --- a/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs +++ b/crates/astria-sequencer/src/ibc/ibc_relayer_change.rs @@ -1,6 +1,5 @@ use astria_core::protocol::transaction::v1::action::IbcRelayerChange; use astria_eyre::eyre::{ - ensure, Result, WrapErr as _, }; @@ -10,11 +9,7 @@ use cnidarium::StateWrite; use crate::{ address::StateReadExt as _, app::ActionHandler, - ibc::{ - StateReadExt as _, - StateWriteExt as _, - }, - transaction::StateReadExt as _, + ibc::StateWriteExt as _, }; #[async_trait] @@ -24,10 +19,6 @@ impl ActionHandler for IbcRelayerChange { } async fn check_and_execute(&self, mut state: S) -> Result<()> { - let from = state - .get_transaction_context() - .expect("transaction source must be present in state when executing an action") - .address_bytes(); match self { IbcRelayerChange::Addition(addr) | IbcRelayerChange::Removal(addr) => { state.ensure_base_prefix(addr).await.wrap_err( @@ -36,15 +27,6 @@ impl ActionHandler for IbcRelayerChange { } } - let ibc_sudo_address = state - .get_ibc_sudo_address() - .await - .wrap_err("failed to get IBC sudo address")?; - ensure!( - ibc_sudo_address == from, - "unauthorized address for IBC relayer change" - ); - match self { IbcRelayerChange::Addition(address) => { state diff --git a/crates/astria-sequencer/src/ibc/mod.rs b/crates/astria-sequencer/src/ibc/mod.rs index 5a94d8b536..6cdc9fc968 100644 --- a/crates/astria-sequencer/src/ibc/mod.rs +++ b/crates/astria-sequencer/src/ibc/mod.rs @@ -3,7 +3,7 @@ pub(crate) mod host_interface; pub(crate) mod ibc_relayer_change; pub(crate) mod ics20_transfer; pub(crate) mod ics20_withdrawal; -mod state_ext; +pub(crate) mod state_ext; pub(crate) mod storage; pub(crate) use state_ext::{ diff --git a/crates/astria-sequencer/src/lib.rs b/crates/astria-sequencer/src/lib.rs index 8701ee6a72..c3e81777b9 100644 --- a/crates/astria-sequencer/src/lib.rs +++ b/crates/astria-sequencer/src/lib.rs @@ -3,6 +3,7 @@ pub(crate) mod address; pub(crate) mod app; pub(crate) mod assets; pub(crate) mod authority; +pub(crate) mod authorization; #[cfg(any(test, feature = "benchmark"))] pub(crate) mod benchmark_and_test_utils; #[cfg(feature = "benchmark")] diff --git a/crates/astria-sequencer/src/metrics.rs b/crates/astria-sequencer/src/metrics.rs index a59f4d33bf..bfa0a4df23 100644 --- a/crates/astria-sequencer/src/metrics.rs +++ b/crates/astria-sequencer/src/metrics.rs @@ -34,6 +34,7 @@ pub struct Metrics { check_tx_duration_seconds_fetch_balances: Histogram, check_tx_duration_seconds_fetch_tx_cost: Histogram, check_tx_duration_seconds_insert_to_app_mempool: Histogram, + check_tx_duration_seconds_check_authorization: Histogram, actions_per_transaction_in_mempool: Histogram, transaction_in_mempool_size_bytes: Histogram, transactions_in_mempool_total: Gauge, @@ -141,6 +142,11 @@ impl Metrics { .record(duration); } + pub(crate) fn record_check_tx_duration_seconds_check_authorization(&self, duration: Duration) { + self.check_tx_duration_seconds_check_authorization + .record(duration); + } + pub(crate) fn record_actions_per_transaction_in_mempool(&self, count: usize) { self.actions_per_transaction_in_mempool.record(count); } @@ -311,6 +317,8 @@ impl telemetry::Metrics for Metrics { .register_with_labels(&[(CHECK_TX_STAGE, "check for removal".to_string())])?; let check_tx_duration_seconds_insert_to_app_mempool = check_tx_duration_factory .register_with_labels(&[(CHECK_TX_STAGE, "insert to app mempool".to_string())])?; + let check_tx_duration_seconds_check_authorization = check_tx_duration_factory + .register_with_labels(&[(CHECK_TX_STAGE, "authorization check".to_string())])?; let actions_per_transaction_in_mempool = builder .new_histogram_factory( @@ -377,6 +385,7 @@ impl telemetry::Metrics for Metrics { check_tx_duration_seconds_fetch_balances, check_tx_duration_seconds_fetch_tx_cost, check_tx_duration_seconds_insert_to_app_mempool, + check_tx_duration_seconds_check_authorization, actions_per_transaction_in_mempool, transaction_in_mempool_size_bytes, transactions_in_mempool_total, @@ -406,6 +415,7 @@ metric_names!(const METRICS_NAMES: CHECK_TX_DURATION_SECONDS_FETCH_NONCE, CHECK_TX_DURATION_SECONDS_FETCH_TX_COST, CHECK_TX_DURATION_SECONDS_CHECK_TRACKED, + CHECK_TX_DURATION_SECONDS_CHECK_AUTHORIZATION, ACTIONS_PER_TRANSACTION_IN_MEMPOOL, TRANSACTION_IN_MEMPOOL_SIZE_BYTES, TRANSACTIONS_IN_MEMPOOL_TOTAL, @@ -419,6 +429,7 @@ mod tests { use super::{ ACTIONS_PER_TRANSACTION_IN_MEMPOOL, CHECK_TX_DURATION_SECONDS, + CHECK_TX_DURATION_SECONDS_CHECK_AUTHORIZATION, CHECK_TX_REMOVED_ACCOUNT_BALANCE, CHECK_TX_REMOVED_EXPIRED, CHECK_TX_REMOVED_FAILED_EXECUTION, @@ -501,6 +512,10 @@ mod tests { TRANSACTIONS_IN_MEMPOOL_PARKED, "transactions_in_mempool_parked", ); + assert_const( + CHECK_TX_DURATION_SECONDS_CHECK_AUTHORIZATION, + "check_tx_duration_seconds_check_authorization", + ); assert_const(MEMPOOL_RECOSTED, "mempool_recosted"); assert_const(INTERNAL_LOGIC_ERROR, "internal_logic_error"); } diff --git a/crates/astria-sequencer/src/service/mempool/mod.rs b/crates/astria-sequencer/src/service/mempool/mod.rs index 88b59a5b77..e61ed8ed20 100644 --- a/crates/astria-sequencer/src/service/mempool/mod.rs +++ b/crates/astria-sequencer/src/service/mempool/mod.rs @@ -52,6 +52,7 @@ use crate::{ accounts::StateReadExt as _, address::StateReadExt as _, app::ActionHandler as _, + authorization, mempool::{ get_account_balances, InsertionError, @@ -245,7 +246,7 @@ async fn handle_check_tx( } // perform stateless checks - let signed_tx = match stateless_checks(tx, &state, metrics).await { + let signed_tx = match transaction_checks(tx, &state, metrics).await { Ok(signed_tx) => signed_tx, Err(rsp) => return rsp, }; @@ -308,7 +309,7 @@ async fn check_removed_comet_bft( /// /// Returns an `Err(response::CheckTx)` if the transaction fails any of the checks. /// Otherwise, it returns the [`Transaction`] to be inserted into the mempool. -async fn stateless_checks( +async fn transaction_checks( tx: Bytes, state: &S, metrics: &'static Metrics, @@ -375,7 +376,25 @@ async fn stateless_checks( )); } - metrics.record_check_tx_duration_seconds_check_chain_id(finished_check_stateless.elapsed()); + let finished_check_chain_id = Instant::now(); + metrics.record_check_tx_duration_seconds_check_chain_id( + finished_check_chain_id.saturating_duration_since(finished_check_stateless), + ); + + if let Err(e) = authorization::AuthorizationHandler::check_authorization( + &signed_tx, + &state, + signed_tx.verification_key().address_bytes(), + ) + .await + { + return Err(error_response( + AbciErrorCode::AUTHORIZATION_FAILED, + format!("transaction failed authorization checks: {e:#}"), + )); + } + + metrics.record_check_tx_duration_seconds_check_authorization(finished_check_chain_id.elapsed()); // NOTE: decide if worth moving to post-insertion, would have to recalculate cost metrics.record_transaction_in_mempool_size_bytes(tx_len); diff --git a/crates/astria-sequencer/src/service/mempool/tests.rs b/crates/astria-sequencer/src/service/mempool/tests.rs index 2baa7bda6b..b96821937c 100644 --- a/crates/astria-sequencer/src/service/mempool/tests.rs +++ b/crates/astria-sequencer/src/service/mempool/tests.rs @@ -1,6 +1,9 @@ use std::num::NonZeroU32; -use astria_core::Protobuf as _; +use astria_core::{ + protocol::transaction::v1::Group, + Protobuf as _, +}; use prost::Message as _; use telemetry::Metrics; use tendermint::{ @@ -14,7 +17,10 @@ use tendermint::{ use crate::{ app::{ benchmark_and_test_utils::genesis_state, - test_utils::MockTxBuilder, + test_utils::{ + get_bob_signing_key, + MockTxBuilder, + }, App, }, mempool::{ @@ -168,3 +174,39 @@ async fn recheck_adds_non_tracked_tx() { // mempool should contain single transaction still assert_eq!(mempool.len().await, 1); } + +#[tokio::test] +async fn unauthorized_sudo_signers_rejected() { + // The mempool should not allow incorrect sudo signers. + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + + let metrics = Box::leak(Box::new(Metrics::noop_metrics(&()).unwrap())); + let mut mempool = Mempool::new(metrics, 100); + let mut app = App::new(snapshot, mempool.clone(), metrics).await.unwrap(); + let genesis_state = genesis_state(); + + app.init_chain(storage.clone(), genesis_state, vec![], "test".to_string()) + .await + .unwrap(); + app.commit(storage.clone()).await; + + let tx = MockTxBuilder::new() + .group(Group::UnbundleableSudo) + .signer(get_bob_signing_key()) + .build(); + let req = CheckTx { + tx: tx.to_raw().encode_to_vec().into(), + kind: CheckTxKind::New, + }; + + let rsp = super::handle_check_tx(req, storage.latest_snapshot(), &mut mempool, metrics).await; + assert_eq!( + rsp.code, + Code::Err(NonZeroU32::new(18).unwrap()), + "{rsp:#?}" + ); + + // mempool should not contain any transactions + assert_eq!(mempool.len().await, 0); +} diff --git a/crates/astria-sequencer/src/transaction/mod.rs b/crates/astria-sequencer/src/transaction/mod.rs index d430cca111..5a74b00366 100644 --- a/crates/astria-sequencer/src/transaction/mod.rs +++ b/crates/astria-sequencer/src/transaction/mod.rs @@ -40,15 +40,13 @@ use crate::{ ActionHandler, StateReadExt as _, }, + authorization::AuthorizationHandler, bridge::{ StateReadExt as _, StateWriteExt as _, }, fees::FeeHandler, - ibc::{ - host_interface::AstriaHost, - StateReadExt as _, - }, + ibc::host_interface::AstriaHost, }; #[derive(Debug)] @@ -158,7 +156,6 @@ impl ActionHandler for Transaction { // FIXME (https://github.com/astriaorg/astria/issues/1584): because most lines come from delegating (and error wrapping) to the // individual actions. This could be tidied up by implementing `ActionHandler for Action` // and letting it delegate. - #[expect(clippy::too_many_lines, reason = "should be refactored")] async fn check_and_execute(&self, mut state: S) -> Result<()> { // Add the current signed transaction into the ephemeral state in case // downstream actions require access to it. @@ -239,16 +236,10 @@ impl ActionHandler for Transaction { // access to the the signer through state. However, what's the correct // ibc AppHandler call to do it? Can we just update one of the trait methods // of crate::ibc::ics20_transfer::Ics20Transfer? - ensure!( - state - .is_ibc_relayer(self) - .await - .wrap_err("failed to check if address is IBC relayer")?, - "only IBC sudo address can execute IBC actions" - ); let action = act .clone() .with_handler::(); + action.check_authorization(&state, &self).await?; action .check_and_execute(&mut state) .await @@ -285,10 +276,18 @@ impl ActionHandler for Transaction { } } -async fn check_execute_and_pay_fees( +async fn check_execute_and_pay_fees< + T: ActionHandler + FeeHandler + AuthorizationHandler + Sync, + S: StateWrite, +>( action: &T, mut state: S, ) -> Result<()> { + let from = state + .get_transaction_context() + .expect("transaction source must be present in state when executing an action") + .address_bytes(); + action.check_authorization(&state, &from).await?; action.check_and_execute(&mut state).await?; action.check_and_pay_fees(&mut state).await?; Ok(()) From 50cddebf0e2d406a65820f1eed27eb9a4f03cd75 Mon Sep 17 00:00:00 2001 From: Lily Johnson Date: Wed, 30 Oct 2024 14:25:51 +0700 Subject: [PATCH 2/2] update changelog --- crates/astria-sequencer/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/astria-sequencer/CHANGELOG.md b/crates/astria-sequencer/CHANGELOG.md index 9e18802d96..33da83f9b9 100644 --- a/crates/astria-sequencer/CHANGELOG.md +++ b/crates/astria-sequencer/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Bump penumbra dependencies [#1740](https://github.com/astriaorg/astria/pull/1740). - Move fee event recording to transaction from block [#1718](https://github.com/astriaorg/astria/pull/1718). +- Mempool now rejects transactions with incorrect authorizations with a new ABCI error code [#1713](https://github.com/astriaorg/astria/pull/1713). ## [1.0.0-rc.2] - 2024-10-23