From 602978311c34e7734b518da375c56bff90e452a9 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Thu, 24 Aug 2023 11:35:50 +0100 Subject: [PATCH 1/4] Introduce `has_eth_addr_segment()` storage key predicate --- core/src/ledger/eth_bridge/storage/mod.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/core/src/ledger/eth_bridge/storage/mod.rs b/core/src/ledger/eth_bridge/storage/mod.rs index b777caa2654..e728603fb7f 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) From 4c76977ea96d87742d6d5bf878492e9f698664e6 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Thu, 24 Aug 2023 11:37:39 +0100 Subject: [PATCH 2/4] Refactor the Ethereum bridge native VP Remove ERC20 transfer checks from the Ethereum bridge native VP. Now, the only checks performed by this VP are that NAM was escrowed correctly under the Ethereum bridge address, and that no keys other than balance keys under this address can be modified. --- .../ledger/native_vp/ethereum_bridge/vp.rs | 257 ++++++------------ 1 file changed, 80 insertions(+), 177 deletions(-) diff --git a/shared/src/ledger/native_vp/ethereum_bridge/vp.rs b/shared/src/ledger/native_vp/ethereum_bridge/vp.rs index 6a18554692b..af965da715c 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,11 @@ 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.len() == 2 + && keys_changed + .iter() + .all(|key| is_balance_key(nam_addr, key).is_some()) + && keys_changed.contains(&escrow_key(nam_addr))) } #[cfg(test)] @@ -232,6 +168,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::storage_api::StorageWrite; use namada_ethereum_bridge::parameters::{ Contracts, EthereumBridgeConfig, UpgradeableContract, @@ -245,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; @@ -256,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; @@ -332,11 +268,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()); } @@ -346,9 +294,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![ @@ -357,9 +305,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)); } } @@ -369,9 +317,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)); } { @@ -382,9 +330,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)); } { @@ -399,54 +347,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)); } } From b348bdeb44f67b61f8096bca9c30ee48bc7e8728 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Thu, 24 Aug 2023 15:46:17 +0100 Subject: [PATCH 3/4] Allow an arbitrary nr of accounts to transfer to the Eth bridge addr --- shared/src/ledger/native_vp/ethereum_bridge/vp.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/shared/src/ledger/native_vp/ethereum_bridge/vp.rs b/shared/src/ledger/native_vp/ethereum_bridge/vp.rs index af965da715c..87be6d15106 100644 --- a/shared/src/ledger/native_vp/ethereum_bridge/vp.rs +++ b/shared/src/ledger/native_vp/ethereum_bridge/vp.rs @@ -153,11 +153,10 @@ fn validate_changed_keys( relevant_keys.len = keys_changed.len(), "Found keys changed under our account" ); - Ok(keys_changed.len() == 2 + Ok(keys_changed.contains(&escrow_key(nam_addr)) && keys_changed .iter() - .all(|key| is_balance_key(nam_addr, key).is_some()) - && keys_changed.contains(&escrow_key(nam_addr))) + .all(|key| is_balance_key(nam_addr, key).is_some())) } #[cfg(test)] From f63d364e0fe2eec555d758c32ecb77999c8bf316 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Tue, 29 Aug 2023 13:46:24 +0100 Subject: [PATCH 4/4] Changelog for #1855 --- .changelog/unreleased/bug-fixes/1855-fix-ethbridge-vp.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/bug-fixes/1855-fix-ethbridge-vp.md 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 00000000000..a3947e1f519 --- /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