Skip to content

fix(sequencer)!: take funds from bridge in ics20 withdrawals #1344

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
SuperFluffy merged 2 commits into from
Aug 20, 2024
Merged

fix(sequencer)!: take funds from bridge in ics20 withdrawals #1344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
44f9197
fix(sequencer)!: take funds from bridge address if ics20 withdrawal i…
SuperFluffy Aug 2, 2024
0c40bfa
remove no longer necessary comment
SuperFluffy Aug 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions crates/astria-sequencer/src/bridge/state_ext.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,12 @@ fn last_transaction_hash_for_bridge_account_storage_key<T: AddressBytes>(address

#[async_trait]
pub(crate) trait StateReadExt: StateRead + address::StateReadExt {
#[instrument(skip_all)]
async fn is_a_bridge_account<T: AddressBytes>(&self, address: T) -> anyhow::Result<bool> {
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<T: AddressBytes>(
&self,
Expand Down
258 changes: 107 additions & 151 deletions crates/astria-sequencer/src/ibc/ics20_withdrawal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use anyhow::{
anyhow,
bail,
ensure,
Context as _,
Expand Down Expand Up @@ -30,7 +29,6 @@ use penumbra_ibc::component::packet::{
use crate::{
accounts::{
AddressBytes,
StateReadExt as _,
StateWriteExt as _,
},
address::StateReadExt as _,
Expand Down Expand Up @@ -59,30 +57,29 @@ fn withdrawal_to_unchecked_ibc_packet(
)
}

async fn ics20_withdrawal_check_stateful_bridge_account<S: StateRead>(
/// 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<S: StateRead>(
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);

Expand All @@ -99,7 +96,7 @@ async fn ics20_withdrawal_check_stateful_bridge_account<S: StateRead>(
"sender does not match bridge withdrawer address; unauthorized"
);

Ok(())
Ok(bridge_address)
}

#[async_trait::async_trait]
Expand Down Expand Up @@ -130,7 +127,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()
Expand All @@ -145,47 +144,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)
Expand Down Expand Up @@ -235,13 +197,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);
Expand All @@ -261,13 +224,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);
Expand Down Expand Up @@ -296,97 +262,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::<Denom>().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::<Denom>().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"),
Expand All @@ -407,18 +367,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);
Expand All @@ -440,13 +398,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",
);
}
}