diff --git a/crates/astria-sequencer/src/bridge/state_ext.rs b/crates/astria-sequencer/src/bridge/state_ext.rs index 0316603c09..8e12ba6949 100644 --- a/crates/astria-sequencer/src/bridge/state_ext.rs +++ b/crates/astria-sequencer/src/bridge/state_ext.rs @@ -149,6 +149,12 @@ fn last_transaction_hash_for_bridge_account_storage_key(address #[async_trait] pub(crate) trait StateReadExt: StateRead + address::StateReadExt { + #[instrument(skip_all)] + async fn is_a_bridge_account(&self, address: T) -> anyhow::Result { + let maybe_id = self.get_bridge_account_rollup_id(address).await?; + Ok(maybe_id.is_some()) + } + #[instrument(skip_all)] async fn get_bridge_account_rollup_id( &self, diff --git a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs index 34ca3fee80..97adaf25af 100644 --- a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs +++ b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs @@ -1,5 +1,4 @@ use anyhow::{ - anyhow, bail, ensure, Context as _, @@ -30,7 +29,6 @@ use penumbra_ibc::component::packet::{ use crate::{ accounts::{ AddressBytes, - StateReadExt as _, StateWriteExt as _, }, address::StateReadExt as _, @@ -59,30 +57,36 @@ fn withdrawal_to_unchecked_ibc_packet( ) } -async fn ics20_withdrawal_check_stateful_bridge_account( +// bridge address checks: +// - if the sender of this transaction is not a bridge account, and the tx `bridge_address` field is +// None, don't need to do any bridge related checks as it's a normal user withdrawal. +// - if the sender of this transaction is a bridge account, and the tx `bridge_address` field is +// None, check that the withdrawer address is the same as the transaction sender. +// - if the tx `bridge_address` field is Some, check that the `bridge_address` is a valid bridge, +// and check that the withdrawer address is the same as the transaction sender. +/// Establishes the withdrawal target. +/// +/// The function returns the following addresses under the following conditions: +/// 1. `from` if `action.bridge_address` is unset and `from` is *not* a bridge account; +/// 2. `from` if `action.bridge_address` is unset and `from` is a bridge account and `from` is its +/// stored withdrawer address. +/// 3. `action.bridge_address` if `action.bridge_address` is set and a bridge account and `from` is +/// its stored withdrawer address. +async fn establish_withdrawal_target( action: &action::Ics20Withdrawal, state: &S, from: [u8; 20], -) -> Result<()> { - // bridge address checks: - // - if the sender of this transaction is not a bridge account, and the tx `bridge_address` - // field is None, don't need to do any bridge related checks as it's a normal user withdrawal. - // - if the sender of this transaction is a bridge account, and the tx `bridge_address` field is - // None, check that the withdrawer address is the same as the transaction sender. - // - if the tx `bridge_address` field is Some, check that the `bridge_address` is a valid - // bridge, and check that the withdrawer address is the same as the transaction sender. - - let is_sender_bridge = state - .get_bridge_account_rollup_id(from) - .await - .context("failed to get bridge account rollup id")? - .is_some(); - - if !is_sender_bridge && action.bridge_address.is_none() { - return Ok(()); +) -> Result<[u8; 20]> { + if action.bridge_address.is_none() + && !state + .is_a_bridge_account(from) + .await + .context("failed to get bridge account rollup id")? + { + return Ok(from); } - // if `action.bridge_address` is Some, but it's not a valid bridge account, + // if `action.bridge_address` is set, but it's not a valid bridge account, // the `get_bridge_account_withdrawer_address` step will fail. let bridge_address = action.bridge_address.map_or(from, Address::bytes); @@ -99,7 +103,7 @@ async fn ics20_withdrawal_check_stateful_bridge_account( "sender does not match bridge withdrawer address; unauthorized" ); - Ok(()) + Ok(bridge_address) } #[async_trait::async_trait] @@ -132,7 +136,9 @@ impl ActionHandler for action::Ics20Withdrawal { )?; } - ics20_withdrawal_check_stateful_bridge_account(self, &state, from).await?; + let withdrawal_target = establish_withdrawal_target(self, &state, from) + .await + .context("failed establishing which account to withdraw funds from")?; let fee = state .get_ics20_withdrawal_base_fee() @@ -147,47 +153,10 @@ impl ActionHandler for action::Ics20Withdrawal { .context("packet failed send check")? }; - let transfer_asset = self.denom(); - - let from_fee_balance = state - .get_account_balance(from, self.fee_asset()) - .await - .context("failed getting `from` account balance for fee payment")?; - - // if fee asset is same as transfer asset, ensure accounts has enough funds - // to cover both the fee and the amount transferred - if self.fee_asset().to_ibc_prefixed() == transfer_asset.to_ibc_prefixed() { - let payment_amount = self - .amount() - .checked_add(fee) - .ok_or(anyhow!("transfer amount plus fee overflowed"))?; - - ensure!( - from_fee_balance >= payment_amount, - "insufficient funds for transfer and fee payment" - ); - } else { - // otherwise, check the fee asset account has enough to cover the fees, - // and the transfer asset account has enough to cover the transfer - ensure!( - from_fee_balance >= fee, - "insufficient funds for fee payment" - ); - - let from_transfer_balance = state - .get_account_balance(from, transfer_asset) - .await - .context("failed to get account balance in transfer check")?; - ensure!( - from_transfer_balance >= self.amount(), - "insufficient funds for transfer" - ); - } - state - .decrease_balance(from, self.denom(), self.amount()) + .decrease_balance(withdrawal_target, self.denom(), self.amount()) .await - .context("failed to decrease sender balance")?; + .context("failed to decrease sender or bridge balance")?; state .decrease_balance(from, self.fee_asset(), fee) @@ -237,13 +206,14 @@ mod tests { address::StateWriteExt as _, bridge::StateWriteExt as _, test_utils::{ + assert_anyhow_error, astria_address, ASTRIA_PREFIX, }, }; #[tokio::test] - async fn check_stateful_bridge_account_not_bridge() { + async fn sender_is_withdrawal_target_if_bridge_is_not_set_and_sender_is_not_bridge() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let state = StateDelta::new(snapshot); @@ -263,13 +233,16 @@ mod tests { memo: String::new(), }; - ics20_withdrawal_check_stateful_bridge_account(&action, &state, from) - .await - .unwrap(); + assert_eq!( + establish_withdrawal_target(&action, &state, from) + .await + .unwrap(), + from + ); } #[tokio::test] - async fn check_stateful_bridge_account_sender_is_bridge_bridge_address_none_ok() { + async fn sender_is_withdrawal_target_if_bridge_is_unset_but_sender_is_bridge() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); @@ -298,97 +271,91 @@ mod tests { memo: String::new(), }; - ics20_withdrawal_check_stateful_bridge_account(&action, &state, bridge_address) - .await - .unwrap(); + assert_eq!( + establish_withdrawal_target(&action, &state, bridge_address) + .await + .unwrap(), + bridge_address, + ); } - #[tokio::test] - async fn check_stateful_bridge_account_sender_is_bridge_bridge_address_none_invalid() { - let storage = cnidarium::TempStorage::new().await.unwrap(); - let snapshot = storage.latest_snapshot(); - let mut state = StateDelta::new(snapshot); + mod bridge_sender_is_rejected_because_it_is_not_a_withdrawer { + use super::*; - state.put_base_prefix(ASTRIA_PREFIX).unwrap(); + fn bridge_address() -> [u8; 20] { + [1; 20] + } - // withdraw is *not* the bridge address, Ics20Withdrawal must be sent by the withdrawer - let bridge_address = [1u8; 20]; - state.put_bridge_account_rollup_id( - bridge_address, - &RollupId::from_unhashed_bytes("testrollupid"), - ); - state.put_bridge_account_withdrawer_address(bridge_address, astria_address(&[2u8; 20])); + fn denom() -> Denom { + "test".parse().unwrap() + } - let denom = "test".parse::().unwrap(); - let action = action::Ics20Withdrawal { - amount: 1, - denom: denom.clone(), - bridge_address: None, - destination_chain_address: "test".to_string(), - return_address: astria_address(&bridge_address), - timeout_height: Height::new(1, 1).unwrap(), - timeout_time: 1, - source_channel: "channel-0".to_string().parse().unwrap(), - fee_asset: denom.clone(), - memo: String::new(), - }; + fn action() -> action::Ics20Withdrawal { + action::Ics20Withdrawal { + amount: 1, + denom: denom(), + bridge_address: None, + destination_chain_address: "test".to_string(), + return_address: astria_address(&[1; 20]), + timeout_height: Height::new(1, 1).unwrap(), + timeout_time: 1, + source_channel: "channel-0".to_string().parse().unwrap(), + fee_asset: denom(), + memo: String::new(), + } + } - let err = ics20_withdrawal_check_stateful_bridge_account(&action, &state, bridge_address) - .await - .unwrap_err(); - assert!( - err.to_string() - .contains("sender does not match bridge withdrawer address; unauthorized") - ); - } + async fn run_test(action: action::Ics20Withdrawal) { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); - #[tokio::test] - async fn check_stateful_bridge_account_bridge_address_some_ok() { - let storage = cnidarium::TempStorage::new().await.unwrap(); - let snapshot = storage.latest_snapshot(); - let mut state = StateDelta::new(snapshot); + state.put_base_prefix(ASTRIA_PREFIX).unwrap(); - state.put_base_prefix(ASTRIA_PREFIX).unwrap(); + // withdraw is *not* the bridge address, Ics20Withdrawal must be sent by the withdrawer + state.put_bridge_account_rollup_id( + bridge_address(), + &RollupId::from_unhashed_bytes("testrollupid"), + ); + state.put_bridge_account_withdrawer_address( + bridge_address(), + astria_address(&[2u8; 20]), + ); - // sender the withdrawer address, so it's ok - let bridge_address = [1u8; 20]; - let withdrawer_address = [2u8; 20]; - state.put_bridge_account_rollup_id( - bridge_address, - &RollupId::from_unhashed_bytes("testrollupid"), - ); - state.put_bridge_account_withdrawer_address(bridge_address, withdrawer_address); + assert_anyhow_error( + &establish_withdrawal_target(&action, &state, bridge_address()) + .await + .unwrap_err(), + "sender does not match bridge withdrawer address; unauthorized", + ); + } - let denom = "test".parse::().unwrap(); - let action = action::Ics20Withdrawal { - amount: 1, - denom: denom.clone(), - bridge_address: Some(astria_address(&bridge_address)), - destination_chain_address: "test".to_string(), - return_address: astria_address(&bridge_address), - timeout_height: Height::new(1, 1).unwrap(), - timeout_time: 1, - source_channel: "channel-0".to_string().parse().unwrap(), - fee_asset: denom.clone(), - memo: String::new(), - }; + #[tokio::test] + async fn bridge_unset() { + let mut action = action(); + action.bridge_address = None; + run_test(action).await; + } - ics20_withdrawal_check_stateful_bridge_account(&action, &state, withdrawer_address) - .await - .unwrap(); + #[tokio::test] + async fn bridge_set() { + let mut action = action(); + action.bridge_address = Some(astria_address(&bridge_address())); + run_test(action).await; + } } #[tokio::test] - async fn check_stateful_bridge_account_bridge_address_some_invalid_sender() { + async fn bridge_sender_is_withdrawal_target() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); state.put_base_prefix(ASTRIA_PREFIX).unwrap(); - // sender is not the withdrawer address, so must fail + // sender the withdrawer address, so it's ok let bridge_address = [1u8; 20]; - let withdrawer_address = astria_address(&[2u8; 20]); + let withdrawer_address = [2u8; 20]; state.put_bridge_account_rollup_id( bridge_address, &RollupId::from_unhashed_bytes("testrollupid"), @@ -409,18 +376,16 @@ mod tests { memo: String::new(), }; - let err = ics20_withdrawal_check_stateful_bridge_account(&action, &state, bridge_address) - .await - .unwrap_err(); - assert!( - err.to_string() - .contains("sender does not match bridge withdrawer address; unauthorized") + assert_eq!( + establish_withdrawal_target(&action, &state, withdrawer_address) + .await + .unwrap(), + bridge_address, ); } #[tokio::test] - async fn ics20_withdrawal_check_stateful_bridge_account_bridge_address_some_invalid_bridge_account() - { + async fn bridge_is_rejected_as_withdrawal_target_because_it_has_no_withdrawer_address_set() { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let state = StateDelta::new(snapshot); @@ -442,13 +407,11 @@ mod tests { memo: String::new(), }; - let err = - ics20_withdrawal_check_stateful_bridge_account(&action, &state, not_bridge_address) + assert_anyhow_error( + &establish_withdrawal_target(&action, &state, not_bridge_address) .await - .unwrap_err(); - assert!( - err.to_string() - .contains("bridge address must have a withdrawer address set") + .unwrap_err(), + "bridge address must have a withdrawer address set", ); } }