From 88a3961e883e88878a7309738d808da4427e92bd Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Thu, 28 Sep 2023 11:00:20 +0100 Subject: [PATCH 1/7] Factor out submit_bridge_pool_tx() --- apps/src/lib/cli/client.rs | 26 ++------------------------ apps/src/lib/client/tx.rs | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/apps/src/lib/cli/client.rs b/apps/src/lib/cli/client.rs index bdb5c7e3b7..5d76a90cf5 100644 --- a/apps/src/lib/cli/client.rs +++ b/apps/src/lib/cli/client.rs @@ -1,7 +1,6 @@ use color_eyre::eyre::Result; use namada::types::io::Io; -use namada_sdk::tx::dump_tx; -use namada_sdk::{signing, Namada, NamadaImpl}; +use namada_sdk::{Namada, NamadaImpl}; use crate::cli; use crate::cli::api::{CliApi, CliClient}; @@ -220,28 +219,7 @@ impl CliApi { client.wait_until_node_is_synced(io).await?; let args = args.to_sdk(&mut ctx); let namada = ctx.to_sdk(&client, io); - let tx_args = args.tx.clone(); - let (mut tx, signing_data, _epoch) = - args.clone().build(&namada).await?; - - signing::generate_test_vector(&namada, &tx).await?; - - if args.tx.dump_tx { - dump_tx::(io, &args.tx, tx); - } else { - tx::submit_reveal_aux( - &namada, - tx_args.clone(), - &args.sender, - ) - .await?; - - namada - .sign(&mut tx, &tx_args, signing_data) - .await?; - - namada.submit(tx, &tx_args).await?; - } + tx::submit_bridge_pool_tx(&namada, args).await?; } Sub::TxUnjailValidator(TxUnjailValidator(mut args)) => { let client = client.unwrap_or_else(|| { diff --git a/apps/src/lib/client/tx.rs b/apps/src/lib/client/tx.rs index 1afefab825..8215eaa078 100644 --- a/apps/src/lib/client/tx.rs +++ b/apps/src/lib/client/tx.rs @@ -96,6 +96,26 @@ pub async fn submit_reveal_aux<'a>( Ok(()) } +pub async fn submit_bridge_pool_tx<'a, N: Namada<'a>>( + namada: &N, + args: args::EthereumBridgePool, +) -> Result<(), error::Error> { + let tx_args = args.tx.clone(); + let (mut tx, signing_data, _epoch) = args.clone().build(namada).await?; + + signing::generate_test_vector(namada, &tx).await?; + + if args.tx.dump_tx { + tx::dump_tx(namada.io(), &args.tx, tx); + } else { + submit_reveal_aux(namada, tx_args.clone(), &args.sender).await?; + namada.sign(&mut tx, &tx_args, signing_data).await?; + namada.submit(tx, &tx_args).await?; + } + + Ok(()) +} + pub async fn submit_custom<'a, N: Namada<'a>>( namada: &N, args: args::TxCustom, From 5aeabd8983e9dd235a30077de8e034d053d5ddfa Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Thu, 28 Sep 2023 13:24:02 +0100 Subject: [PATCH 2/7] Factor out validate_bridge_pool_tx() --- sdk/src/eth_bridge/bridge_pool.rs | 110 +++++++++++++++++++----------- 1 file changed, 72 insertions(+), 38 deletions(-) diff --git a/sdk/src/eth_bridge/bridge_pool.rs b/sdk/src/eth_bridge/bridge_pool.rs index 4a1664a8e8..403af5fe8c 100644 --- a/sdk/src/eth_bridge/bridge_pool.rs +++ b/sdk/src/eth_bridge/bridge_pool.rs @@ -14,6 +14,7 @@ use namada_core::types::eth_abi::Encode; use namada_core::types::eth_bridge_pool::{ GasFee, PendingTransfer, TransferToEthereum, TransferToEthereumKind, }; +use namada_core::types::ethereum_events::EthAddress; use namada_core::types::keccak::KeccakHash; use namada_core::types::storage::Epoch; use namada_core::types::token::{Amount, DenominatedAmount}; @@ -41,8 +42,8 @@ use crate::{ }; /// Craft a transaction that adds a transfer to the Ethereum bridge pool. -pub async fn build_bridge_pool_tx<'a>( - context: &impl Namada<'a>, +pub async fn build_bridge_pool_tx( + context: &impl Namada<'_>, args::EthereumBridgePool { tx: tx_args, nut, @@ -56,26 +57,75 @@ pub async fn build_bridge_pool_tx<'a>( code_path, }: args::EthereumBridgePool, ) -> Result<(Tx, SigningTxData, Option), Error> { - let default_signer = Some(sender.clone()); - let signing_data = aux_signing_data( + let sender_ = sender.clone(); + let (transfer, tx_code_hash, signing_data) = futures::try_join!( + validate_bridge_pool_tx( + context, + tx_args.force, + nut, + asset, + recipient, + sender, + amount, + fee_amount, + fee_payer, + fee_token, + ), + query_wasm_code_hash(context, code_path.to_string_lossy()), + aux_signing_data( + context, + &tx_args, + // token owner + Some(sender_.clone()), + // tx signer + Some(sender_), + ), + )?; + + let chain_id = tx_args + .chain_id + .clone() + .ok_or_else(|| Error::Other("No chain id available".into()))?; + + let mut tx = Tx::new(chain_id, tx_args.expiration); + tx.add_code_from_hash(tx_code_hash).add_data(transfer); + + let epoch = prepare_tx( context, &tx_args, - Some(sender.clone()), - default_signer, + &mut tx, + signing_data.fee_payer.clone(), + None, ) .await?; - let fee_payer = fee_payer.unwrap_or_else(|| sender.clone()); - let DenominatedAmount { amount, .. } = validate_amount( - context, - amount, - &wrapped_erc20s::token(&asset), - tx_args.force, - ) - .await - .map_err(|e| Error::Other(format!("Failed to validate amount. {}", e)))?; + + Ok((tx, signing_data, epoch)) +} + +/// Perform client validation checks on a Bridge pool transfer. +#[allow(clippy::too_many_arguments)] +async fn validate_bridge_pool_tx( + context: &impl Namada<'_>, + force: bool, + nut: bool, + asset: EthAddress, + recipient: EthAddress, + sender: Address, + amount: args::InputAmount, + fee_amount: args::InputAmount, + fee_payer: Option
, + fee_token: Address, +) -> Result { + let DenominatedAmount { amount, .. } = + validate_amount(context, amount, &wrapped_erc20s::token(&asset), force) + .await + .map_err(|e| { + Error::Other(format!("Failed to validate amount. {}", e)) + })?; + let DenominatedAmount { amount: fee_amount, .. - } = validate_amount(context, fee_amount, &fee_token, tx_args.force) + } = validate_amount(context, fee_amount, &fee_token, force) .await .map_err(|e| { Error::Other(format!( @@ -83,6 +133,8 @@ pub async fn build_bridge_pool_tx<'a>( e )) })?; + + let fee_payer = fee_payer.unwrap_or_else(|| sender.clone()); let transfer = PendingTransfer { transfer: TransferToEthereum { asset, @@ -102,28 +154,11 @@ pub async fn build_bridge_pool_tx<'a>( }, }; - let tx_code_hash = - query_wasm_code_hash(context, code_path.to_string_lossy()).await?; - - let chain_id = tx_args - .chain_id - .clone() - .ok_or_else(|| Error::Other("No chain id available".into()))?; - let mut tx = Tx::new(chain_id, tx_args.expiration); - tx.add_code_from_hash(tx_code_hash).add_data(transfer); - - // TODO(namada#1800): validate the tx on the client side - - let epoch = prepare_tx( - context, - &tx_args, - &mut tx, - signing_data.fee_payer.clone(), - None, - ) - .await?; + // if force { + // return Ok(transfer); + //} - Ok((tx, signing_data, epoch)) + Ok(transfer) } /// A json serializable representation of the Ethereum @@ -999,7 +1034,6 @@ mod recommendations { #[cfg(test)] mod test_recommendations { use namada_core::types::address::Address; - use namada_core::types::ethereum_events::EthAddress; use super::*; use crate::io::StdIo; From 601ada822894f0cced699921302be951ffab4f87 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Thu, 28 Sep 2023 13:52:14 +0100 Subject: [PATCH 3/7] Add new SDK error types --- sdk/src/error.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/sdk/src/error.rs b/sdk/src/error.rs index b34a7a5562..dba1898615 100644 --- a/sdk/src/error.rs +++ b/sdk/src/error.rs @@ -3,6 +3,7 @@ use namada_core::proto::Tx; use namada_core::types::address::Address; use namada_core::types::dec::Dec; +use namada_core::types::ethereum_events::EthAddress; use namada_core::types::storage; use namada_core::types::storage::Epoch; use prost::EncodeError; @@ -324,6 +325,18 @@ pub enum EthereumBridgeError { /// Invalid Bridge pool nonce error. #[error("The Bridge pool nonce is invalid")] InvalidBpNonce, + /// Invalid fee token error. + #[error("An invalid fee token was provided: {0}")] + InvalidFeeToken(Address), + /// Not whitelisted error. + #[error("ERC20 is not whitelisted: {0}")] + Erc20NotWhitelisted(EthAddress), + /// Exceeded token caps error. + #[error("ERC20 token caps exceeded: {0}")] + Erc20TokenCapsExceeded(EthAddress), + /// Transfer already in pool error. + #[error("An identical transfer is already present in the Bridge pool")] + TransferAlreadyInPool, } /// Checks if the given error is an invalid viewing key From a025cfcd03d073c760b97b27b26f05f53b7180e8 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Thu, 28 Sep 2023 14:36:36 +0100 Subject: [PATCH 4/7] Check ERC20 token caps --- sdk/src/queries/shell/eth_bridge.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/sdk/src/queries/shell/eth_bridge.rs b/sdk/src/queries/shell/eth_bridge.rs index 6baf649992..4c765e1144 100644 --- a/sdk/src/queries/shell/eth_bridge.rs +++ b/sdk/src/queries/shell/eth_bridge.rs @@ -57,6 +57,15 @@ pub struct Erc20FlowControl { cap: Amount, } +impl Erc20FlowControl { + /// Check if the `transferred_amount` exceeds the token caps of some ERC20 + /// asset. + #[inline] + pub fn exceeds_token_caps(&self, transferred_amount: Amount) -> bool { + self.supply + transferred_amount > self.cap + } +} + /// Request data to pass to `generate_bridge_pool_proof`. #[derive(Debug, Clone, Eq, PartialEq, BorshSerialize, BorshDeserialize)] pub struct GenBridgePoolProofReq<'transfers, 'relayer> { From 3b3e6e90fa072f602bec4b4e70c9e4aafef64d42 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Thu, 28 Sep 2023 14:39:18 +0100 Subject: [PATCH 5/7] Make ERC20 flow control fields public --- sdk/src/queries/shell/eth_bridge.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sdk/src/queries/shell/eth_bridge.rs b/sdk/src/queries/shell/eth_bridge.rs index 4c765e1144..c9c820ed5a 100644 --- a/sdk/src/queries/shell/eth_bridge.rs +++ b/sdk/src/queries/shell/eth_bridge.rs @@ -50,11 +50,11 @@ use crate::queries::{EncodedResponseQuery, RequestCtx, RequestQuery}; )] pub struct Erc20FlowControl { /// Whether the wrapped asset is whitelisted. - whitelisted: bool, + pub whitelisted: bool, /// Total minted supply of some wrapped asset. - supply: Amount, + pub supply: Amount, /// The token cap of some wrapped asset. - cap: Amount, + pub cap: Amount, } impl Erc20FlowControl { From 3525f010a57c07b9f749a976a76f71f06705abcf Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Thu, 28 Sep 2023 13:52:35 +0100 Subject: [PATCH 6/7] Validate Bridge pool client transfers --- sdk/src/eth_bridge/bridge_pool.rs | 188 ++++++++++++++++++++++++++---- 1 file changed, 165 insertions(+), 23 deletions(-) diff --git a/sdk/src/eth_bridge/bridge_pool.rs b/sdk/src/eth_bridge/bridge_pool.rs index 403af5fe8c..31d996906b 100644 --- a/sdk/src/eth_bridge/bridge_pool.rs +++ b/sdk/src/eth_bridge/bridge_pool.rs @@ -8,8 +8,10 @@ use std::sync::Arc; use borsh::BorshSerialize; use ethbridge_bridge_contract::Bridge; use ethers::providers::Middleware; +use futures::future::FutureExt; +use namada_core::ledger::eth_bridge::storage::bridge_pool::get_pending_key; use namada_core::ledger::eth_bridge::storage::wrapped_erc20s; -use namada_core::types::address::Address; +use namada_core::types::address::{Address, InternalAddress}; use namada_core::types::eth_abi::Encode; use namada_core::types::eth_bridge_pool::{ GasFee, PendingTransfer, TransferToEthereum, TransferToEthereumKind, @@ -17,7 +19,7 @@ use namada_core::types::eth_bridge_pool::{ use namada_core::types::ethereum_events::EthAddress; use namada_core::types::keccak::KeccakHash; use namada_core::types::storage::Epoch; -use namada_core::types::token::{Amount, DenominatedAmount}; +use namada_core::types::token::{balance_key, Amount, DenominatedAmount}; use namada_core::types::voting_power::FractionalVotingPower; use owo_colors::OwoColorize; use serde::Serialize; @@ -25,7 +27,9 @@ use serde::Serialize; use super::{block_on_eth_sync, eth_sync_or_exit, BlockOnEthSync}; use crate::control_flow::install_shutdown_signal; use crate::control_flow::time::{Duration, Instant}; -use crate::error::{EncodingError, Error, EthereumBridgeError, QueryError}; +use crate::error::{ + EncodingError, Error, EthereumBridgeError, QueryError, TxError, +}; use crate::eth_bridge::ethers::abi::AbiDecode; use crate::internal_macros::echo_error; use crate::io::Io; @@ -34,7 +38,7 @@ use crate::queries::{ Client, GenBridgePoolProofReq, GenBridgePoolProofRsp, TransferToErcArgs, RPC, }; -use crate::rpc::{query_wasm_code_hash, validate_amount}; +use crate::rpc::{query_storage_value, query_wasm_code_hash, validate_amount}; use crate::signing::aux_signing_data; use crate::tx::prepare_tx; use crate::{ @@ -116,24 +120,34 @@ async fn validate_bridge_pool_tx( fee_payer: Option
, fee_token: Address, ) -> Result { - let DenominatedAmount { amount, .. } = - validate_amount(context, amount, &wrapped_erc20s::token(&asset), force) - .await - .map_err(|e| { - Error::Other(format!("Failed to validate amount. {}", e)) - })?; + let token_addr = wrapped_erc20s::token(&asset); + let validate_token_amount = + validate_amount(context, amount, &token_addr, force).map(|result| { + result.map_err(|e| { + Error::Other(format!( + "Failed to validate Bridge pool transfer amount: {e}" + )) + }) + }); - let DenominatedAmount { - amount: fee_amount, .. - } = validate_amount(context, fee_amount, &fee_token, force) - .await - .map_err(|e| { - Error::Other(format!( - "Failed to validate Bridge pool fee amount. {}", - e - )) - })?; + let validate_fee_amount = + validate_amount(context, fee_amount, &fee_token, force).map(|result| { + result.map_err(|e| { + Error::Other(format!( + "Failed to validate Bridge pool fee amount: {e}", + )) + }) + }); + + // validate amounts + let ( + tok_denominated @ DenominatedAmount { amount, .. }, + fee_denominated @ DenominatedAmount { + amount: fee_amount, .. + }, + ) = futures::try_join!(validate_token_amount, validate_fee_amount)?; + // build pending Bridge pool transfer let fee_payer = fee_payer.unwrap_or_else(|| sender.clone()); let transfer = PendingTransfer { transfer: TransferToEthereum { @@ -154,9 +168,137 @@ async fn validate_bridge_pool_tx( }, }; - // if force { - // return Ok(transfer); - //} + if force { + return Ok(transfer); + } + + //====================================================== + // XXX: the following validations should be kept in sync + // with the validations performed by the Bridge pool VP! + //====================================================== + + // check if an identical transfer is already in the Bridge pool + let transfer_in_pool = RPC + .shell() + .storage_has_key(context.client(), &get_pending_key(&transfer)) + .await + .map_err(|e| Error::Query(QueryError::General(e.to_string())))?; + if transfer_in_pool { + return Err(Error::EthereumBridge( + EthereumBridgeError::TransferAlreadyInPool, + )); + } + + let wnam_addr = RPC + .shell() + .eth_bridge() + .read_native_erc20_contract(context.client()) + .await + .map_err(|e| { + Error::EthereumBridge(EthereumBridgeError::RetrieveContract( + e.to_string(), + )) + })?; + + // validate gas fee token + match &transfer.gas_fee.token { + Address::Internal(InternalAddress::Nut(_)) => { + return Err(Error::EthereumBridge( + EthereumBridgeError::InvalidFeeToken(transfer.gas_fee.token), + )); + } + fee_token if fee_token == &wrapped_erc20s::token(&wnam_addr) => { + return Err(Error::EthereumBridge( + EthereumBridgeError::InvalidFeeToken(transfer.gas_fee.token), + )); + } + _ => {} + } + + // validate wnam token caps + whitelist + if transfer.transfer.asset == wnam_addr { + let flow_control = RPC + .shell() + .eth_bridge() + .get_erc20_flow_control(context.client(), &wnam_addr) + .await + .map_err(|e| { + Error::Query(QueryError::General(format!( + "Failed to read wrapped NAM flow control data: {e}" + ))) + })?; + + if !flow_control.whitelisted { + return Err(Error::EthereumBridge( + EthereumBridgeError::Erc20NotWhitelisted(wnam_addr), + )); + } + + if flow_control.exceeds_token_caps(transfer.transfer.amount) { + return Err(Error::EthereumBridge( + EthereumBridgeError::Erc20TokenCapsExceeded(wnam_addr), + )); + } + } + + // validate balances + let maybe_balance_error = if token_addr == transfer.gas_fee.token { + let expected_debit = transfer.transfer.amount + transfer.gas_fee.amount; + let balance: Amount = query_storage_value( + context.client(), + &balance_key(&token_addr, &transfer.transfer.sender), + ) + .await?; + + balance + .checked_sub(expected_debit) + .is_none() + .then_some((token_addr, tok_denominated)) + } else { + let check_tokens = async { + let balance: Amount = query_storage_value( + context.client(), + &balance_key(&token_addr, &transfer.transfer.sender), + ) + .await?; + Result::<_, Error>::Ok( + balance + .checked_sub(transfer.transfer.amount) + .is_none() + .then_some((token_addr, tok_denominated)), + ) + }; + let check_fees = async { + let balance: Amount = query_storage_value( + context.client(), + &balance_key( + &transfer.gas_fee.token, + &transfer.transfer.sender, + ), + ) + .await?; + Result::<_, Error>::Ok( + balance + .checked_sub(transfer.gas_fee.amount) + .is_none() + .then_some(( + transfer.gas_fee.token.clone(), + fee_denominated, + )), + ) + }; + + let (err_tokens, err_fees) = + futures::try_join!(check_tokens, check_fees)?; + err_tokens.or(err_fees) + }; + if let Some((token, amount)) = maybe_balance_error { + return Err(Error::Tx(TxError::NegativeBalanceAfterTransfer( + Box::new(transfer.transfer.sender), + amount.to_string(), + Box::new(token), + ))); + } Ok(transfer) } From bbab1c48d3a03fcd0fba27261da11bb1819d7102 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Fri, 29 Sep 2023 13:20:06 +0100 Subject: [PATCH 7/7] Changelog for #1957 --- .changelog/unreleased/SDK/1957-bp-client-validation.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/SDK/1957-bp-client-validation.md diff --git a/.changelog/unreleased/SDK/1957-bp-client-validation.md b/.changelog/unreleased/SDK/1957-bp-client-validation.md new file mode 100644 index 0000000000..ad829f2c88 --- /dev/null +++ b/.changelog/unreleased/SDK/1957-bp-client-validation.md @@ -0,0 +1,2 @@ +- Validate Bridge pool transfers before submitting them to the network + ([\#1957](https://github.com/anoma/namada/pull/1957)) \ No newline at end of file