diff --git a/.changelog/unreleased/bug-fixes/2443-fix-multitoken-vp.md b/.changelog/unreleased/bug-fixes/2443-fix-multitoken-vp.md new file mode 100644 index 0000000000..e9ec9aab31 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/2443-fix-multitoken-vp.md @@ -0,0 +1,2 @@ +- Avoid diff overflow in Multitoken VP + ([\#2443](https://github.com/anoma/namada/issues/2443)) \ No newline at end of file diff --git a/crates/namada/src/ledger/native_vp/multitoken.rs b/crates/namada/src/ledger/native_vp/multitoken.rs index 3f9ea1cdf8..8999855ece 100644 --- a/crates/namada/src/ledger/native_vp/multitoken.rs +++ b/crates/namada/src/ledger/native_vp/multitoken.rs @@ -13,7 +13,7 @@ use crate::token::storage_key::{ is_any_minted_balance_key, is_any_minter_key, is_any_token_balance_key, minter_key, }; -use crate::token::{Amount, Change}; +use crate::token::Amount; use crate::types::address::{Address, InternalAddress}; use crate::types::storage::{Key, KeySeg}; use crate::vm::WasmCacheAccess; @@ -53,26 +53,71 @@ where keys_changed: &BTreeSet, verifiers: &BTreeSet
, ) -> Result { - let mut changes = HashMap::new(); - let mut mints = HashMap::new(); + let mut inc_changes: HashMap = HashMap::new(); + let mut dec_changes: HashMap = HashMap::new(); + let mut inc_mints: HashMap = HashMap::new(); + let mut dec_mints: HashMap = HashMap::new(); for key in keys_changed { if let Some([token, _]) = is_any_token_balance_key(key) { let pre: Amount = self.ctx.read_pre(key)?.unwrap_or_default(); let post: Amount = self.ctx.read_post(key)?.unwrap_or_default(); - let diff = post.change() - pre.change(); - match changes.get_mut(token) { - Some(change) => *change += diff, - None => _ = changes.insert(token, diff), + match post.checked_sub(pre) { + Some(diff) => { + let change = + inc_changes.entry(token.clone()).or_default(); + *change = + change.checked_add(diff).ok_or_else(|| { + Error::NativeVpError( + native_vp::Error::SimpleMessage( + "Overflowed in balance check", + ), + ) + })?; + } + None => { + let diff = pre + .checked_sub(post) + .expect("Underflow shouldn't happen here"); + let change = + dec_changes.entry(token.clone()).or_default(); + *change = + change.checked_add(diff).ok_or_else(|| { + Error::NativeVpError( + native_vp::Error::SimpleMessage( + "Overflowed in balance check", + ), + ) + })?; + } } } else if let Some(token) = is_any_minted_balance_key(key) { let pre: Amount = self.ctx.read_pre(key)?.unwrap_or_default(); let post: Amount = self.ctx.read_post(key)?.unwrap_or_default(); - let diff = post.change() - pre.change(); - match mints.get_mut(token) { - Some(mint) => *mint += diff, - None => _ = mints.insert(token, diff), + match post.checked_sub(pre) { + Some(diff) => { + let mint = inc_mints.entry(token.clone()).or_default(); + *mint = mint.checked_add(diff).ok_or_else(|| { + Error::NativeVpError( + native_vp::Error::SimpleMessage( + "Overflowed in balance check", + ), + ) + })?; + } + None => { + let diff = pre + .checked_sub(post) + .expect("Underflow shouldn't happen here"); + let mint = dec_mints.entry(token.clone()).or_default(); + *mint = mint.checked_add(diff).ok_or_else(|| { + Error::NativeVpError( + native_vp::Error::SimpleMessage( + "Overflowed in balance check", + ), + ) + })?; + } } - // Check if the minter is set if !self.is_valid_minter(token, verifiers)? { return Ok(false); @@ -94,12 +139,31 @@ where } } - Ok(changes.iter().all(|(token, change)| { - let mint = match mints.get(token) { - Some(mint) => *mint, - None => Change::zero(), - }; - *change == mint + let mut all_tokens = BTreeSet::new(); + all_tokens.extend(inc_changes.keys().cloned()); + all_tokens.extend(dec_changes.keys().cloned()); + all_tokens.extend(inc_mints.keys().cloned()); + all_tokens.extend(dec_mints.keys().cloned()); + + Ok(all_tokens.iter().all(|token| { + let inc_change = + inc_changes.get(token).cloned().unwrap_or_default(); + let dec_change = + dec_changes.get(token).cloned().unwrap_or_default(); + let inc_mint = inc_mints.get(token).cloned().unwrap_or_default(); + let dec_mint = dec_mints.get(token).cloned().unwrap_or_default(); + + if inc_change >= dec_change && inc_mint >= dec_mint { + inc_change.checked_sub(dec_change) + == inc_mint.checked_sub(dec_mint) + } else if (inc_change < dec_change && inc_mint >= dec_mint) + || (inc_change >= dec_change && inc_mint < dec_mint) + { + false + } else { + dec_change.checked_sub(inc_change) + == dec_mint.checked_sub(inc_mint) + } })) } } diff --git a/crates/namada/src/ledger/native_vp/slash_fund.rs b/crates/namada/src/ledger/native_vp/slash_fund.rs deleted file mode 100644 index 6cd463ea1c..0000000000 --- a/crates/namada/src/ledger/native_vp/slash_fund.rs +++ /dev/null @@ -1,103 +0,0 @@ -//! SlashFund VP - -use std::collections::BTreeSet; - -use namada_core::ledger::slash_fund; -/// SlashFund storage -pub use namada_core::ledger::slash_fund::storage; -use namada_state::StorageRead; -use namada_tx::Tx; -use thiserror::Error; - -use crate::ledger::native_vp::{self, governance, Ctx, NativeVp}; -use crate::state::{self as ledger_storage, StorageHasher}; -use crate::token; -use crate::types::address::Address; -use crate::types::storage::Key; -use crate::vm::WasmCacheAccess; - -#[allow(missing_docs)] -#[derive(Error, Debug)] -pub enum Error { - #[error("Native VP error: {0}")] - NativeVpError(#[from] native_vp::Error), -} - -/// SlashFund functions result -pub type Result = std::result::Result; - -/// SlashFund VP -pub struct SlashFundVp<'a, DB, H, CA> -where - DB: namada_state::DB + for<'iter> namada_state::DBIter<'iter>, - H: StorageHasher, - CA: WasmCacheAccess, -{ - /// Context to interact with the host structures. - pub ctx: Ctx<'a, DB, H, CA>, -} - -impl<'a, DB, H, CA> NativeVp for SlashFundVp<'a, DB, H, CA> -where - DB: 'static + namada_state::DB + for<'iter> namada_state::DBIter<'iter>, - H: 'static + StorageHasher, - CA: 'static + WasmCacheAccess, -{ - type Error = Error; - - fn validate_tx( - &self, - tx_data: &Tx, - keys_changed: &BTreeSet, - _verifiers: &BTreeSet
, - ) -> Result { - let native_token = self.ctx.pre().get_native_token()?; - let result = keys_changed.iter().all(|key| { - let key_type: KeyType = get_key_type(key, &native_token); - match key_type { - KeyType::BALANCE(addr) => { - if addr.ne(&slash_fund::ADDRESS) { - return true; - } - let data = if let Some(data) = tx_data.data() { - data - } else { - return false; - }; - governance::utils::is_proposal_accepted( - &self.ctx.pre(), - &data, - ) - .unwrap_or(false) - } - KeyType::UNKNOWN_SLASH_FUND => false, - KeyType::UNKNOWN => true, - } - }); - Ok(result) - } -} - -#[allow(clippy::upper_case_acronyms)] -enum KeyType { - #[allow(clippy::upper_case_acronyms)] - BALANCE(Address), - #[allow(clippy::upper_case_acronyms)] - #[allow(non_camel_case_types)] - UNKNOWN_SLASH_FUND, - #[allow(clippy::upper_case_acronyms)] - UNKNOWN, -} - -fn get_key_type(value: &Key, native_token: &Address) -> KeyType { - if storage::is_slash_fund_key(value) { - KeyType::UNKNOWN_SLASH_FUND - } else if token::is_any_token_balance_key(value).is_some() { - match token::is_balance_key(native_token, value) { - Some(addr) => KeyType::BALANCE(addr.clone()), - None => KeyType::UNKNOWN, - } - } else { - KeyType::UNKNOWN - } -}