Skip to content

Commit

Permalink
fix(sequencer)!: take funds from bridge in ics20 withdrawals (#1344)
Browse files Browse the repository at this point in the history 
## Summary
Fixes ics20 withdrawals by actually taking funds from the provided
bridge address.

## Background
Prior to this change, funds were always taken from the signer of the
`astria.protocol.v1alpha1.Ics20Withdrawal` action, even if its
`bridge_address` field was set.

## Changes
- Uses `astria.protocol.v1alpha1.Ics20Withdrawal.bridge_address` to take
funds in an ics20 withdrawal if set (and if it passes bridge checks)
- Remove unnecessary state accesses to check if enough funds are
present: just try to decrease them. If they fail, then the entire action
fails.

## Testing
All present tests have been updated to check if the correct withdrawal
target is found.

There should be tests for the entire
`Ics20Withdrawal::check_and_execute` flow. However, adding an ibc
channel and port does not seem to be currently possible because
`penumbra_ibc` does not export the channel `StateWriteExt` trait. This
is left to a followup.

## Breaking Changelist
This is a breaking change because two different versions of sequencer
will come to different conclusions on which account to take funds from.

## Related Issues
Closes #1308
  • Loading branch information
@SuperFluffy
SuperFluffy authored Aug 20, 2024
1 parent 9f959f4 commit d47a374
Show file tree
Hide file tree
Showing 2 changed files with 113 additions and 151 deletions.
6 changes: 6 additions & 0 deletions crates/astria-sequencer/src/bridge/state_ext.rs
View file
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
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",
);
}
}

0 comments on commit d47a374

Please sign in to comment.