diff --git a/.changelog/unreleased/bug-fixes/1855-fix-ethbridge-vp.md b/.changelog/unreleased/bug-fixes/1855-fix-ethbridge-vp.md new file mode 100644 index 0000000000..a3947e1f51 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/1855-fix-ethbridge-vp.md @@ -0,0 +1,2 @@ +- Fix the Ethereum Bridge VP + ([\#1855](https://github.com/anoma/namada/pull/1855)) \ No newline at end of file diff --git a/core/src/ledger/eth_bridge/storage/mod.rs b/core/src/ledger/eth_bridge/storage/mod.rs index b777caa265..e728603fb7 100644 --- a/core/src/ledger/eth_bridge/storage/mod.rs +++ b/core/src/ledger/eth_bridge/storage/mod.rs @@ -7,7 +7,7 @@ use super::ADDRESS; use crate::ledger::parameters::storage::*; use crate::ledger::parameters::ADDRESS as PARAM_ADDRESS; use crate::types::address::Address; -use crate::types::storage::{Key, KeySeg}; +use crate::types::storage::{DbKeySeg, Key, KeySeg}; use crate::types::token::balance_key; /// Key prefix for the storage subspace @@ -26,6 +26,15 @@ pub fn escrow_key(nam_addr: &Address) -> Key { balance_key(nam_addr, &ADDRESS) } +/// Check if the given `key` contains an Ethereum +/// bridge address segment. +#[inline] +pub fn has_eth_addr_segment(key: &Key) -> bool { + key.segments + .iter() + .any(|s| matches!(s, DbKeySeg::AddressSeg(ADDRESS))) +} + /// Returns whether a key belongs to this account or not pub fn is_eth_bridge_key(nam_addr: &Address, key: &Key) -> bool { key == &escrow_key(nam_addr) diff --git a/shared/src/ledger/native_vp/ethereum_bridge/vp.rs b/shared/src/ledger/native_vp/ethereum_bridge/vp.rs index 00ca7d933f..650c065ab7 100644 --- a/shared/src/ledger/native_vp/ethereum_bridge/vp.rs +++ b/shared/src/ledger/native_vp/ethereum_bridge/vp.rs @@ -1,22 +1,23 @@ //! Validity predicate for the Ethereum bridge use std::collections::{BTreeSet, HashSet}; -use borsh::BorshDeserialize; use eyre::{eyre, Result}; -use itertools::Itertools; -use namada_core::ledger::eth_bridge::storage::{ - self, escrow_key, wrapped_erc20s, -}; +use namada_core::ledger::eth_bridge::storage::{self, escrow_key}; use namada_core::ledger::storage::traits::StorageHasher; use namada_core::ledger::{eth_bridge, storage as ledger_storage}; use namada_core::types::address::Address; use namada_core::types::storage::Key; -use namada_core::types::token::{balance_key, Amount}; +use namada_core::types::token::{balance_key, is_balance_key, Amount}; -use crate::ledger::native_vp::{Ctx, NativeVp, VpEnv}; +use crate::ledger::native_vp::{Ctx, NativeVp, StorageReader}; use crate::proto::Tx; use crate::vm::WasmCacheAccess; +/// Generic error that may be returned by the validity predicate +#[derive(thiserror::Error, Debug)] +#[error(transparent)] +pub struct Error(#[from] eyre::Error); + /// Validity predicate for the Ethereum bridge pub struct EthBridge<'ctx, DB, H, CA> where @@ -34,39 +35,29 @@ where H: 'static + StorageHasher, CA: 'static + WasmCacheAccess, { - /// If the bridge's escrow key was changed, we check - /// that the balance increased and that the bridge pool - /// VP has been triggered. The bridge pool VP will carry - /// out the rest of the checks. + /// If the Ethereum bridge's escrow key was written to, we check + /// that the NAM balance increased and that the Bridge pool VP has + /// been triggered. fn check_escrow( &self, verifiers: &BTreeSet
, ) -> Result { let escrow_key = balance_key(&self.ctx.storage.native_token, ð_bridge::ADDRESS); - let escrow_pre: Amount = if let Ok(Some(bytes)) = - self.ctx.read_bytes_pre(&escrow_key) - { - BorshDeserialize::try_from_slice(bytes.as_slice()).map_err( - |_| Error(eyre!("Couldn't deserialize a balance from storage")), - )? - } else { - tracing::debug!( - "Could not retrieve the Ethereum bridge VP's balance from \ - storage" - ); - return Ok(false); - }; + + let escrow_pre: Amount = + if let Ok(Some(value)) = (&self.ctx).read_pre_value(&escrow_key) { + value + } else { + tracing::debug!( + "Could not retrieve the Ethereum bridge VP's balance from \ + storage" + ); + return Ok(false); + }; let escrow_post: Amount = - if let Ok(Some(bytes)) = self.ctx.read_bytes_post(&escrow_key) { - BorshDeserialize::try_from_slice(bytes.as_slice()).map_err( - |_| { - Error(eyre!( - "Couldn't deserialize the balance of the Ethereum \ - bridge VP from storage." - )) - }, - )? + if let Ok(Some(value)) = (&self.ctx).read_post_value(&escrow_key) { + value } else { tracing::debug!( "Could not retrieve the modified Ethereum bridge VP's \ @@ -77,6 +68,8 @@ where // The amount escrowed should increase. if escrow_pre < escrow_post { + // NB: normally, we only escrow NAM under the Ethereum bridge + // addresss in the context of a Bridge pool transfer Ok(verifiers.contains(&storage::bridge_pool::BRIDGE_POOL_ADDRESS)) } else { tracing::info!( @@ -88,19 +81,6 @@ where } } -/// One of the the two types of checks -/// this VP must perform. -#[derive(Debug)] -enum CheckType { - Escrow, - Erc20Transfer, -} - -#[derive(thiserror::Error, Debug)] -#[error(transparent)] -/// Generic error that may be returned by the validity predicate -pub struct Error(#[from] eyre::Error); - impl<'a, DB, H, CA> NativeVp for EthBridge<'a, DB, H, CA> where DB: 'static + ledger_storage::DB + for<'iter> ledger_storage::DBIter<'iter>, @@ -112,12 +92,8 @@ where /// Validate that a wasm transaction is permitted to change keys under this /// account. /// - /// We permit only the following changes via wasm for the time being: - /// - a wrapped ERC20's supply key to decrease iff one of its balance keys - /// decreased by the same amount - /// - a wrapped ERC20's balance key to decrease iff another one of its - /// balance keys increased by the same amount - /// - Escrowing Nam in order to mint wrapped Nam on Ethereum + /// We only permit increasing the escrowed balance of NAM under the Ethereum + /// bridge address, when writing to storage from wasm transactions. /// /// Some other changes to the storage subspace of this account are expected /// to happen natively i.e. bypassing this validity predicate. For example, @@ -135,33 +111,37 @@ where "Ethereum Bridge VP triggered", ); - match determine_check_type( - &self.ctx.storage.native_token, - keys_changed, - )? { - // Multitoken VP checks the balance changes for the ERC20 transfer - Some(CheckType::Erc20Transfer) => Ok(true), - Some(CheckType::Escrow) => self.check_escrow(verifiers), - None => Ok(false), + if !validate_changed_keys(&self.ctx.storage.native_token, keys_changed)? + { + return Ok(false); } + + self.check_escrow(verifiers) } } /// Checks if `keys_changed` represents a valid set of changed keys. -/// Depending on which keys get changed, chooses which type of -/// check to perform in the `validate_tx` function. -/// 1. If the Ethereum bridge escrow key was changed, we need to check -/// that escrow was performed correctly. -/// 2. If two erc20 keys where changed, this is a transfer that needs -/// to be checked. -fn determine_check_type( +/// +/// This implies cheking if two distinct keys were changed: +/// +/// 1. The Ethereum bridge escrow account's NAM balance key. +/// 2. Another account's NAM balance key. +/// +/// Any other keys changed under the Ethereum bridge account +/// are rejected. +fn validate_changed_keys( nam_addr: &Address, keys_changed: &BTreeSet, -) -> Result, Error> { - // we aren't concerned with keys that changed outside of our account +) -> Result { + // acquire all keys that either changed our account, or that touched + // nam balances let keys_changed: HashSet<_> = keys_changed .iter() - .filter(|key| storage::is_eth_bridge_key(nam_addr, key)) + .filter(|&key| { + let changes_eth_storage = storage::has_eth_addr_segment(key); + let changes_nam_balance = is_balance_key(nam_addr, key).is_some(); + changes_nam_balance || changes_eth_storage + }) .collect(); if keys_changed.is_empty() { return Err(Error(eyre!( @@ -173,55 +153,10 @@ fn determine_check_type( relevant_keys.len = keys_changed.len(), "Found keys changed under our account" ); - if keys_changed.len() == 1 && keys_changed.contains(&escrow_key(nam_addr)) { - return Ok(Some(CheckType::Escrow)); - } else if keys_changed.len() != 2 { - tracing::debug!( - relevant_keys.len = keys_changed.len(), - "Rejecting transaction as only two keys should have changed" - ); - return Ok(None); - } - - let mut keys = HashSet::<_>::default(); - for key in keys_changed.into_iter() { - let key = match wrapped_erc20s::Key::try_from((nam_addr, key)) { - Ok(key) => { - // Disallow changes to any supply keys via wasm transactions, - // since these should only ever be changed via FinalizeBlock - // after a successful transfer to or from Ethereum - if matches!(key.suffix, wrapped_erc20s::KeyType::Supply) { - tracing::debug!( - ?key, - "Rejecting transaction as key is a supply key" - ); - return Ok(None); - } - key - } - Err(error) => { - tracing::debug!( - %key, - ?error, - "Rejecting transaction as key is not a wrapped ERC20 key" - ); - return Ok(None); - } - }; - keys.insert(key); - } - - // We can .unwrap() here as we know for sure that this set has len=2 - let (key_a, key_b) = keys.into_iter().collect_tuple().unwrap(); - if key_a.asset != key_b.asset { - tracing::debug!( - ?key_a, - ?key_b, - "Rejecting transaction as keys are for different assets" - ); - return Ok(None); - } - Ok(Some(CheckType::Erc20Transfer)) + Ok(keys_changed.contains(&escrow_key(nam_addr)) + && keys_changed + .iter() + .all(|key| is_balance_key(nam_addr, key).is_some())) } #[cfg(test)] @@ -232,6 +167,7 @@ mod tests { use borsh::BorshSerialize; use namada_core::ledger::eth_bridge; use namada_core::ledger::eth_bridge::storage::bridge_pool::BRIDGE_POOL_ADDRESS; + use namada_core::ledger::eth_bridge::storage::wrapped_erc20s; use namada_core::ledger::gas::TxGasMeter; use namada_core::ledger::storage_api::StorageWrite; use namada_ethereum_bridge::parameters::{ @@ -246,6 +182,7 @@ mod tests { use crate::ledger::storage::write_log::WriteLog; use crate::ledger::storage::{Storage, WlStorage}; use crate::proto::Tx; + use crate::types::address::testing::established_address_1; use crate::types::address::{nam, wnam}; use crate::types::ethereum_events; use crate::types::ethereum_events::EthAddress; @@ -257,8 +194,6 @@ mod tests { const ARBITRARY_OWNER_A_ADDRESS: &str = "atest1d9khqw36x9zyxwfhgfpygv2pgc65gse4gy6rjs34gfzr2v69gy6y23zpggurjv2yx5m52sesu6r4y4"; - const ARBITRARY_OWNER_B_ADDRESS: &str = - "atest1v4ehgw36xuunwd6989prwdfkxqmnvsfjxs6nvv6xxucrs3f3xcmns3fcxdzrvvz9xverzvzr56le8f"; const ARBITRARY_OWNER_A_INITIAL_BALANCE: u64 = 100; const ESCROW_AMOUNT: u64 = 100; const BRIDGE_POOL_ESCROW_INITIAL_BALANCE: u64 = 0; @@ -335,11 +270,23 @@ mod tests { ) } + #[test] + fn test_accepts_expected_keys_changed() { + let keys_changed = BTreeSet::from([ + balance_key(&nam(), &established_address_1()), + balance_key(&nam(), ð_bridge::ADDRESS), + ]); + + let result = validate_changed_keys(&nam(), &keys_changed); + + assert_matches!(result, Ok(true)); + } + #[test] fn test_error_if_triggered_without_keys_changed() { let keys_changed = BTreeSet::new(); - let result = determine_check_type(&nam(), &keys_changed); + let result = validate_changed_keys(&nam(), &keys_changed); assert!(result.is_err()); } @@ -349,9 +296,9 @@ mod tests { { let keys_changed = BTreeSet::from_iter(vec![arbitrary_key(); 3]); - let result = determine_check_type(&nam(), &keys_changed); + let result = validate_changed_keys(&nam(), &keys_changed); - assert_matches!(result, Ok(None)); + assert_matches!(result, Ok(false)); } { let keys_changed = BTreeSet::from_iter(vec![ @@ -360,9 +307,9 @@ mod tests { arbitrary_key(), ]); - let result = determine_check_type(&nam(), &keys_changed); + let result = validate_changed_keys(&nam(), &keys_changed); - assert_matches!(result, Ok(None)); + assert_matches!(result, Ok(false)); } } @@ -372,9 +319,9 @@ mod tests { let keys_changed = BTreeSet::from_iter(vec![arbitrary_key(), arbitrary_key()]); - let result = determine_check_type(&nam(), &keys_changed); + let result = validate_changed_keys(&nam(), &keys_changed); - assert_matches!(result, Ok(None)); + assert_matches!(result, Ok(false)); } { @@ -385,9 +332,9 @@ mod tests { )), ]); - let result = determine_check_type(&nam(), &keys_changed); + let result = validate_changed_keys(&nam(), &keys_changed); - assert_matches!(result, Ok(None)); + assert_matches!(result, Ok(false)); } { @@ -402,54 +349,9 @@ mod tests { ), ]); - let result = determine_check_type(&nam(), &keys_changed); - - assert_matches!(result, Ok(None)); - } - } - - #[test] - fn test_rejects_if_multitoken_keys_for_different_assets() { - { - let keys_changed = BTreeSet::from_iter(vec![ - balance_key( - &wrapped_erc20s::token( - ðereum_events::testing::DAI_ERC20_ETH_ADDRESS, - ), - &Address::decode(ARBITRARY_OWNER_A_ADDRESS) - .expect("Couldn't set up test"), - ), - balance_key( - &wrapped_erc20s::token( - ðereum_events::testing::USDC_ERC20_ETH_ADDRESS, - ), - &Address::decode(ARBITRARY_OWNER_B_ADDRESS) - .expect("Couldn't set up test"), - ), - ]); - - let result = determine_check_type(&nam(), &keys_changed); - - assert_matches!(result, Ok(None)); - } - } - - #[test] - fn test_rejects_if_supply_key_changed() { - let asset = ðereum_events::testing::DAI_ERC20_ETH_ADDRESS; - { - let keys_changed = BTreeSet::from_iter(vec![ - minted_balance_key(&wrapped_erc20s::token(asset)), - balance_key( - &wrapped_erc20s::token(asset), - &Address::decode(ARBITRARY_OWNER_B_ADDRESS) - .expect("Couldn't set up test"), - ), - ]); - - let result = determine_check_type(&nam(), &keys_changed); + let result = validate_changed_keys(&nam(), &keys_changed); - assert_matches!(result, Ok(None)); + assert_matches!(result, Ok(false)); } }