From f353ad667e8d8f080811cc6b1ff1ffe2ffed73bb Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Wed, 30 Oct 2024 11:28:34 -0500 Subject: [PATCH 1/2] move checks to parsing --- Cargo.lock | 1 + crates/astria-core/Cargo.toml | 1 + .../src/protocol/transaction/v1/action/mod.rs | 189 +++++++++++++++--- .../src/bridge/bridge_unlock_action.rs | 15 -- .../src/ibc/ics20_withdrawal.rs | 29 --- 5 files changed, 167 insertions(+), 68 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index aef7fd15f5..b542bfaaea 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -721,6 +721,7 @@ dependencies = [ "prost", "rand 0.8.5", "serde", + "serde_json", "sha2 0.10.8", "tempfile", "tendermint", diff --git a/crates/astria-core/Cargo.toml b/crates/astria-core/Cargo.toml index a7a45d5ba0..1590488984 100644 --- a/crates/astria-core/Cargo.toml +++ b/crates/astria-core/Cargo.toml @@ -37,6 +37,7 @@ penumbra-proto = { workspace = true } prost = { workspace = true } rand = { workspace = true } serde = { workspace = true, features = ["derive"], optional = true } +serde_json = { workspace = true } sha2 = { workspace = true } tendermint = { workspace = true } tendermint-proto = { workspace = true } diff --git a/crates/astria-core/src/protocol/transaction/v1/action/mod.rs b/crates/astria-core/src/protocol/transaction/v1/action/mod.rs index c4f8320f9c..a8852d486e 100644 --- a/crates/astria-core/src/protocol/transaction/v1/action/mod.rs +++ b/crates/astria-core/src/protocol/transaction/v1/action/mod.rs @@ -21,22 +21,25 @@ use crate::{ IncorrectRollupIdLength, RollupId, }, - protocol::fees::v1::{ - BridgeLockFeeComponents, - BridgeSudoChangeFeeComponents, - BridgeUnlockFeeComponents, - FeeAssetChangeFeeComponents, - FeeChangeFeeComponents, - FeeComponentError, - IbcRelayFeeComponents, - IbcRelayerChangeFeeComponents, - IbcSudoChangeFeeComponents, - Ics20WithdrawalFeeComponents, - InitBridgeAccountFeeComponents, - RollupDataSubmissionFeeComponents, - SudoAddressChangeFeeComponents, - TransferFeeComponents, - ValidatorUpdateFeeComponents, + protocol::{ + fees::v1::{ + BridgeLockFeeComponents, + BridgeSudoChangeFeeComponents, + BridgeUnlockFeeComponents, + FeeAssetChangeFeeComponents, + FeeChangeFeeComponents, + FeeComponentError, + IbcRelayFeeComponents, + IbcRelayerChangeFeeComponents, + IbcSudoChangeFeeComponents, + Ics20WithdrawalFeeComponents, + InitBridgeAccountFeeComponents, + RollupDataSubmissionFeeComponents, + SudoAddressChangeFeeComponents, + TransferFeeComponents, + ValidatorUpdateFeeComponents, + }, + memos::v1::Ics20WithdrawalFromRollup, }, Protobuf, }; @@ -887,7 +890,7 @@ enum IbcSudoChangeErrorKind { /// /// It also contains a `return_address` field which may or may not be the same as the signer /// of the packet. The funds will be returned to the `return_address` in the case of a timeout. -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone)] pub struct Ics20Withdrawal { // a transparent value consisting of an amount and a denom. pub amount: u128, @@ -1032,12 +1035,19 @@ impl Protobuf for Ics20Withdrawal { bridge_address, use_compat_address, } = proto; - let amount = amount.ok_or(Ics20WithdrawalError::field_not_set("amount"))?; + let amount = u128::from(amount.ok_or(Ics20WithdrawalError::field_not_set("amount"))?); let return_address = Address::try_from_raw( &return_address.ok_or(Ics20WithdrawalError::field_not_set("return_address"))?, ) .map_err(Ics20WithdrawalError::return_address)?; + if timeout_time == 0 { + return Err(Ics20WithdrawalError::invalid_timeout_time()); + } + if amount == 0 { + return Err(Ics20WithdrawalError::invalid_amount()); + } + let timeout_height = timeout_height .ok_or(Ics20WithdrawalError::field_not_set("timeout_height"))? .into(); @@ -1047,8 +1057,20 @@ impl Protobuf for Ics20Withdrawal { .transpose() .map_err(Ics20WithdrawalError::invalid_bridge_address)?; + if bridge_address.is_some() { + let parsed_withdrawal: Ics20WithdrawalFromRollup = + serde_json::from_str(&memo).map_err(Ics20WithdrawalError::parse_memo)?; + validate_rollup_return_address(&parsed_withdrawal.rollup_return_address) + .map_err(Ics20WithdrawalError::rollup_withdrawal)?; + validate_withdrawal_event_id_and_block_number( + &parsed_withdrawal.rollup_withdrawal_event_id, + parsed_withdrawal.rollup_block_number, + ) + .map_err(Ics20WithdrawalError::rollup_withdrawal)?; + } + Ok(Self { - amount: amount.into(), + amount, denom: denom.parse().map_err(Ics20WithdrawalError::invalid_denom)?, destination_chain_address, return_address, @@ -1089,7 +1111,7 @@ impl Protobuf for Ics20Withdrawal { bridge_address, use_compat_address, } = proto; - let amount = amount.ok_or(Ics20WithdrawalError::field_not_set("amount"))?; + let amount = u128::from(amount.ok_or(Ics20WithdrawalError::field_not_set("amount"))?); let return_address = Address::try_from_raw( return_address .as_ref() @@ -1097,6 +1119,13 @@ impl Protobuf for Ics20Withdrawal { ) .map_err(Ics20WithdrawalError::return_address)?; + if *timeout_time == 0 { + return Err(Ics20WithdrawalError::invalid_timeout_time()); + } + if amount == 0 { + return Err(Ics20WithdrawalError::invalid_amount()); + } + let timeout_height = timeout_height .clone() .ok_or(Ics20WithdrawalError::field_not_set("timeout_height"))? @@ -1107,8 +1136,20 @@ impl Protobuf for Ics20Withdrawal { .transpose() .map_err(Ics20WithdrawalError::invalid_bridge_address)?; + if bridge_address.is_some() { + let parsed_withdrawal: Ics20WithdrawalFromRollup = + serde_json::from_str(memo).map_err(Ics20WithdrawalError::parse_memo)?; + validate_rollup_return_address(&parsed_withdrawal.rollup_return_address) + .map_err(Ics20WithdrawalError::rollup_withdrawal)?; + validate_withdrawal_event_id_and_block_number( + &parsed_withdrawal.rollup_withdrawal_event_id, + parsed_withdrawal.rollup_block_number, + ) + .map_err(Ics20WithdrawalError::rollup_withdrawal)?; + } + Ok(Self { - amount: amount.into(), + amount, denom: denom.parse().map_err(Ics20WithdrawalError::invalid_denom)?, destination_chain_address: destination_chain_address.clone(), return_address, @@ -1189,11 +1230,32 @@ impl Ics20WithdrawalError { Self(Ics20WithdrawalErrorKind::InvalidBridgeAddress(err)) } + #[must_use] fn invalid_denom(source: asset::ParseDenomError) -> Self { Self(Ics20WithdrawalErrorKind::InvalidDenom { source, }) } + + #[must_use] + fn invalid_timeout_time() -> Self { + Self(Ics20WithdrawalErrorKind::InvalidTimeoutTime) + } + + #[must_use] + fn invalid_amount() -> Self { + Self(Ics20WithdrawalErrorKind::InvalidAmount) + } + + #[must_use] + fn parse_memo(err: serde_json::Error) -> Self { + Self(Ics20WithdrawalErrorKind::ParseMemo(err)) + } + + #[must_use] + fn rollup_withdrawal(err: RollupWithdrawalError) -> Self { + Self(Ics20WithdrawalErrorKind::RollupWithdrawal(err)) + } } #[derive(Debug, thiserror::Error)] @@ -1210,6 +1272,14 @@ enum Ics20WithdrawalErrorKind { InvalidBridgeAddress(#[source] AddressError), #[error("`denom` field was invalid")] InvalidDenom { source: asset::ParseDenomError }, + #[error("`timeout_time` must be non-zero")] + InvalidTimeoutTime, + #[error("`amount` must be greater than zero")] + InvalidAmount, + #[error("failed to parse memo for ICS bound bridge withdrawal")] + ParseMemo(#[source] serde_json::Error), + #[error("rollup withdrawal information was invalid")] + RollupWithdrawal(#[source] RollupWithdrawalError), } #[derive(Debug, Clone)] @@ -1705,18 +1775,30 @@ impl Protobuf for BridgeUnlock { let to = to .ok_or_else(|| BridgeUnlockError::field_not_set("to")) .and_then(|to| Address::try_from_raw(&to).map_err(BridgeUnlockError::address))?; - let amount = amount.ok_or_else(|| BridgeUnlockError::field_not_set("amount"))?; + let amount = u128::from(amount.ok_or_else(|| BridgeUnlockError::field_not_set("amount"))?); + if amount == 0 { + return Err(BridgeUnlockError::invalid_amount()); + } + if memo.len() > 64 { + return Err(BridgeUnlockError::invalid_memo()); + } let fee_asset = fee_asset.parse().map_err(BridgeUnlockError::fee_asset)?; + validate_withdrawal_event_id_and_block_number( + &rollup_withdrawal_event_id, + rollup_block_number, + ) + .map_err(BridgeUnlockError::rollup_withdrawal)?; + let bridge_address = bridge_address .ok_or_else(|| BridgeUnlockError::field_not_set("bridge_address")) .and_then(|to| Address::try_from_raw(&to).map_err(BridgeUnlockError::bridge_address))?; Ok(Self { to, - amount: amount.into(), + amount, fee_asset, - memo, bridge_address, + memo, rollup_block_number, rollup_withdrawal_event_id, }) @@ -1766,6 +1848,21 @@ impl BridgeUnlockError { source, }) } + + #[must_use] + fn invalid_amount() -> Self { + Self(BridgeUnlockErrorKind::InvalidAmount) + } + + #[must_use] + fn invalid_memo() -> Self { + Self(BridgeUnlockErrorKind::InvalidMemo) + } + + #[must_use] + fn rollup_withdrawal(err: RollupWithdrawalError) -> Self { + Self(BridgeUnlockErrorKind::RollupWithdrawal(err)) + } } #[derive(Debug, thiserror::Error)] @@ -1778,6 +1875,12 @@ enum BridgeUnlockErrorKind { FeeAsset { source: asset::ParseDenomError }, #[error("the `bridge_address` field was invalid")] BridgeAddress { source: AddressError }, + #[error("`amount` must be greater than zero")] + InvalidAmount, + #[error("`memo` muse be no longer than 64 bytes")] + InvalidMemo, + #[error("rollup withdrawal information was invalid")] + RollupWithdrawal(#[source] RollupWithdrawalError), } #[derive(Debug, Clone)] @@ -2076,3 +2179,41 @@ impl Protobuf for FeeChange { }) } } + +#[derive(Debug, thiserror::Error)] +enum RollupWithdrawalError { + #[error("rollup return address must be non-empty")] + EmptyRollupReturnAddress, + #[error("rollup return address must be no more than 256 bytes")] + InvalidRollupReturnAddress, + #[error("rollup withdrawal event id must be non-empty")] + EmptyRollupEventId, + #[error("rollup withdrawal event id must be no more than 256 bytes")] + InvalidRollupEventId, + #[error("rollup block number must be non-zero")] + InvalidRollupBlockNumber, +} + +fn validate_rollup_return_address(address: &str) -> Result<(), RollupWithdrawalError> { + if address.is_empty() { + return Err(RollupWithdrawalError::EmptyRollupReturnAddress); + } else if address.len() > 256 { + return Err(RollupWithdrawalError::InvalidRollupReturnAddress); + } + Ok(()) +} + +fn validate_withdrawal_event_id_and_block_number( + id: &str, + block: u64, +) -> Result<(), RollupWithdrawalError> { + if id.is_empty() { + return Err(RollupWithdrawalError::EmptyRollupEventId); + } else if id.len() > 256 { + return Err(RollupWithdrawalError::InvalidRollupEventId); + } + if block == 0 { + return Err(RollupWithdrawalError::InvalidRollupBlockNumber); + } + Ok(()) +} diff --git a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs index 52fcb2b407..21c5f4e12a 100644 --- a/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_unlock_action.rs @@ -26,22 +26,7 @@ use crate::{ #[async_trait::async_trait] impl ActionHandler for BridgeUnlock { - // TODO(https://github.com/astriaorg/astria/issues/1430): move checks to the `BridgeUnlock` parsing. async fn check_stateless(&self) -> Result<()> { - ensure!(self.amount > 0, "amount must be greater than zero",); - ensure!(self.memo.len() <= 64, "memo must not be more than 64 bytes"); - ensure!( - !self.rollup_withdrawal_event_id.is_empty(), - "rollup withdrawal event id must be non-empty", - ); - ensure!( - self.rollup_withdrawal_event_id.len() <= 256, - "rollup withdrawal event id must not be more than 256 bytes", - ); - ensure!( - self.rollup_block_number > 0, - "rollup block number must be greater than zero", - ); Ok(()) } diff --git a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs index 12975e4abf..d1db245153 100644 --- a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs +++ b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs @@ -143,36 +143,7 @@ async fn establish_withdrawal_target<'a, S: StateRead>( #[async_trait::async_trait] impl ActionHandler for action::Ics20Withdrawal { - // TODO(https://github.com/astriaorg/astria/issues/1430): move checks to the `Ics20Withdrawal` parsing. async fn check_stateless(&self) -> Result<()> { - ensure!(self.timeout_time() != 0, "timeout time must be non-zero",); - ensure!(self.amount() > 0, "amount must be greater than zero",); - if self.bridge_address.is_some() { - let parsed_bridge_memo: Ics20WithdrawalFromRollup = serde_json::from_str(&self.memo) - .context("failed to parse memo for ICS bound bridge withdrawal")?; - - ensure!( - !parsed_bridge_memo.rollup_return_address.is_empty(), - "rollup return address must be non-empty", - ); - ensure!( - parsed_bridge_memo.rollup_return_address.len() <= 256, - "rollup return address must be no more than 256 bytes", - ); - ensure!( - !parsed_bridge_memo.rollup_withdrawal_event_id.is_empty(), - "rollup withdrawal event id must be non-empty", - ); - ensure!( - parsed_bridge_memo.rollup_withdrawal_event_id.len() <= 256, - "rollup withdrawal event id must be no more than 256 bytes", - ); - ensure!( - parsed_bridge_memo.rollup_block_number != 0, - "rollup block number must be non-zero", - ); - } - // NOTE (from penumbra): we could validate the destination chain address as bech32 to // prevent mistyped addresses, but this would preclude sending to chains that don't // use bech32 addresses. From 4aa74868cec063f41cf884929316b00d59365b2c Mon Sep 17 00:00:00 2001 From: ethanoroshiba Date: Wed, 30 Oct 2024 12:11:20 -0500 Subject: [PATCH 2/2] minor updates --- crates/astria-core/src/protocol/transaction/v1/action/mod.rs | 2 +- crates/astria-sequencer-client/Cargo.toml | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/astria-core/src/protocol/transaction/v1/action/mod.rs b/crates/astria-core/src/protocol/transaction/v1/action/mod.rs index a8852d486e..8609bf42ea 100644 --- a/crates/astria-core/src/protocol/transaction/v1/action/mod.rs +++ b/crates/astria-core/src/protocol/transaction/v1/action/mod.rs @@ -890,7 +890,7 @@ enum IbcSudoChangeErrorKind { /// /// It also contains a `return_address` field which may or may not be the same as the signer /// of the packet. The funds will be returned to the `return_address` in the case of a timeout. -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct Ics20Withdrawal { // a transparent value consisting of an amount and a denom. pub amount: u128, diff --git a/crates/astria-sequencer-client/Cargo.toml b/crates/astria-sequencer-client/Cargo.toml index 1fa879ff12..d913dc8e94 100644 --- a/crates/astria-sequencer-client/Cargo.toml +++ b/crates/astria-sequencer-client/Cargo.toml @@ -39,3 +39,4 @@ serde_json = { workspace = true } tokio = { workspace = true } tokio-test = { workspace = true } wiremock = { workspace = true } +astria-core = { path = "../astria-core", features = ["serde"] }