MASP Bug Fixes

.changelog/unreleased/improvements/2371-masp-fixes.md

@@ -0,0 +1,2 @@
+- Strengthened the checks in the MASP VP. Allow viewing and spending of
+  unconvertible MASP notes ([\#2371](https://github.com/anoma/namada/pull/2371))

core/src/ledger/masp_conversions.rs

@@ -3,7 +3,6 @@
 use std::collections::BTreeMap;
 
 use borsh::{BorshDeserialize, BorshSerialize};
-use borsh_ext::BorshSerializeExt;
 use masp_primitives::asset_type::AssetType;
 use masp_primitives::convert::AllowedConversion;
 use masp_primitives::merkle_tree::FrozenCommitmentTree;
@@ -192,6 +191,12 @@ where
     Ok((noterized_inflation, precision))
 }
 
+// Safely sum a pair of amounts
+#[cfg(any(feature = "wasm-runtime", test))]
+fn sum_pair(pair: (token::Amount, token::Amount)) -> Option<token::Amount> {
+    pair.0.checked_add(pair.1)
+}
+
 // This is only enabled when "wasm-runtime" is on, because we're using rayon
 #[cfg(any(feature = "wasm-runtime", test))]
 /// Update the MASP's allowed conversions
@@ -212,6 +217,8 @@ where
     };
     use rayon::prelude::ParallelSlice;
 
+    use crate::ledger::storage_api::ResultExt;
+    use crate::types::masp::encode_asset_type;
     use crate::types::storage::{Key, KeySeg};
     use crate::types::token::MASP_CONVERT_ANCHOR_KEY;
 
@@ -245,10 +252,14 @@ where
     // notes clients have to use. This trick works under the assumption that
     // reward tokens will then be reinflated back to the current epoch.
     let reward_assets = [
-        encode_asset_type(native_token.clone(), MaspDenom::Zero, Epoch(0)),
-        encode_asset_type(native_token.clone(), MaspDenom::One, Epoch(0)),
-        encode_asset_type(native_token.clone(), MaspDenom::Two, Epoch(0)),
-        encode_asset_type(native_token.clone(), MaspDenom::Three, Epoch(0)),
+        encode_asset_type(Some(Epoch(0)), &native_token, MaspDenom::Zero)
+            .into_storage_result()?,
+        encode_asset_type(Some(Epoch(0)), &native_token, MaspDenom::One)
+            .into_storage_result()?,
+        encode_asset_type(Some(Epoch(0)), &native_token, MaspDenom::Two)
+            .into_storage_result()?,
+        encode_asset_type(Some(Epoch(0)), &native_token, MaspDenom::Three)
+            .into_storage_result()?,
     ];
     // Conversions from the previous to current asset for each address
     let mut current_convs =
@@ -272,15 +283,17 @@ where
             // negative sign allows each instance of the old asset to be
             // cancelled out/replaced with the new asset
             let old_asset = encode_asset_type(
-                addr.clone(),
+                Some(wl_storage.storage.last_epoch),
+                addr,
                 denom,
-                wl_storage.storage.last_epoch,
-            );
+            )
+            .into_storage_result()?;
             let new_asset = encode_asset_type(
-                addr.clone(),
+                Some(wl_storage.storage.block.epoch),
+                addr,
                 denom,
-                wl_storage.storage.block.epoch,
-            );
+            )
+            .into_storage_result()?;
             // Get the last rewarded amount of the native token
             let normed_inflation = wl_storage
                 .storage
@@ -328,11 +341,12 @@ where
                 if denom == MaspDenom::Three {
                     // The reward for each reward.1 units of the current asset
                     // is reward.0 units of the reward token
-                    total_reward += (addr_bal
-                        * (new_normed_inflation, *normed_inflation))
-                        .0
-                        .checked_sub(addr_bal)
-                        .unwrap_or_default();
+                    total_reward += sum_pair(
+                        addr_bal * (new_normed_inflation, *normed_inflation),
+                    )
+                    .unwrap_or(token::Amount::max())
+                    .checked_sub(addr_bal)
+                    .unwrap_or_default();
                     // Save the new normed inflation
                     *normed_inflation = new_normed_inflation;
                 }
@@ -373,9 +387,7 @@ where
                 if denom == MaspDenom::Three {
                     // The reward for each reward.1 units of the current asset
                     // is reward.0 units of the reward token
-                    total_reward += ((addr_bal * (real_reward, reward.1)).0
-                        * (*normed_inflation, ref_inflation))
-                        .0;
+                    total_reward += (addr_bal * (reward.0, reward.1)).0;
                 }
             }
             // Add a conversion from the previous asset type
@@ -467,10 +479,11 @@ where
             // Add the decoding entry for the new asset type. An uncommitted
             // node position is used since this is not a conversion.
             let new_asset = encode_asset_type(
-                addr.clone(),
+                Some(wl_storage.storage.block.epoch),
+                &addr,
                 denom,
-                wl_storage.storage.block.epoch,
-            );
+            )
+            .into_storage_result()?;
             wl_storage.storage.conversion_state.assets.insert(
                 new_asset,
                 (
@@ -486,17 +499,6 @@ where
     Ok(())
 }
 
-/// Construct MASP asset type with given epoch for given token
-pub fn encode_asset_type(
-    addr: Address,
-    denom: MaspDenom,
-    epoch: Epoch,
-) -> AssetType {
-    let new_asset_bytes = (addr, denom, epoch.0).serialize_to_vec();
-    AssetType::new(new_asset_bytes.as_ref())
-        .expect("unable to derive asset identifier")
-}
-
 #[cfg(test)]
 mod tests {
     use std::collections::HashMap;

core/src/ledger/storage/mod.rs

@@ -29,7 +29,7 @@ use crate::ledger::gas::{
     STORAGE_ACCESS_GAS_PER_BYTE, STORAGE_WRITE_GAS_PER_BYTE,
 };
 pub use crate::ledger::masp_conversions::{
-    calculate_masp_rewards, encode_asset_type, ConversionState,
+    calculate_masp_rewards, ConversionState,
 };
 use crate::ledger::parameters::{self, EpochDuration, Parameters};
 use crate::ledger::storage::merkle_tree::{

core/src/proto/types.rs

@@ -767,7 +767,7 @@ pub struct MaspBuilder {
     pub target: crate::types::hash::Hash,
     /// The decoded set of asset types used by the transaction. Useful for
     /// offline wallets trying to display AssetTypes.
-    pub asset_types: HashSet<(Address, MaspDenom, Epoch)>,
+    pub asset_types: HashSet<(Address, MaspDenom, Option<Epoch>)>,
     /// Track how Info objects map to descriptors and outputs
     #[serde(
         serialize_with = "borsh_serde::<SaplingMetadataSerde, _>",

core/src/types/dec.rs

@@ -69,7 +69,8 @@ impl Dec {
     ///
     /// Example:
     /// ```
-    /// 
+    /// use namada_core::types::dec::Dec;
+    ///
     /// let x = Dec::new(3, 1).unwrap(); // Represents 0.3
     /// let y = Dec::new(2, 1).unwrap(); // Represents 0.2
     /// let result = x.trunc_div(&y).unwrap();

core/src/types/masp.rs

@@ -5,14 +5,34 @@ use std::str::FromStr;
 
 use borsh::{BorshDeserialize, BorshSerialize};
 use borsh_ext::BorshSerializeExt;
+use masp_primitives::asset_type::AssetType;
 use sha2::{Digest, Sha256};
 
 use crate::impl_display_and_from_str_via_format;
 use crate::types::address::{Address, DecodeError, HASH_HEX_LEN, MASP};
+use crate::types::storage::Epoch;
 use crate::types::string_encoding::{
     self, MASP_EXT_FULL_VIEWING_KEY_HRP, MASP_EXT_SPENDING_KEY_HRP,
     MASP_PAYMENT_ADDRESS_HRP,
 };
+use crate::types::token::MaspDenom;
+
+/// Make asset type corresponding to given address and epoch
+pub fn encode_asset_type(
+    epoch: Option<Epoch>,
+    token: &Address,
+    denom: MaspDenom,
+) -> Result<AssetType, std::io::Error> {
+    // Timestamp the chosen token with the current epoch
+    let token_bytes = (token, denom, epoch).serialize_to_vec();
+    // Generate the unique asset identifier from the unique token address
+    AssetType::new(token_bytes.as_ref()).map_err(|_| {
+        std::io::Error::new(
+            std::io::ErrorKind::Other,
+            "unable to create asset type".to_string(),
+        )
+    })
+}
 
 // enough capacity to store the payment address
 // plus the pinned/unpinned discriminant

core/src/types/token.rs

@@ -67,6 +67,11 @@ impl Amount {
         }
     }
 
+    /// Convert a [`u128`] to an [`Amount`].
+    pub fn from_u128(x: u128) -> Self {
+        Self { raw: Uint::from(x) }
+    }
+
     /// Get the amount as a [`Change`]
     pub fn change(&self) -> Change {
         self.raw.try_into().unwrap()
@@ -212,6 +217,26 @@ impl Amount {
         Self { raw: Uint(raw) }
     }
 
+    /// Given a u128 and [`MaspDenom`], construct the corresponding
+    /// amount.
+    pub fn from_masp_denominated_u128(
+        val: u128,
+        denom: MaspDenom,
+    ) -> Option<Self> {
+        let lo = val as u64;
+        let hi = (val >> 64) as u64;
+        let lo_pos = denom as usize;
+        let hi_pos = lo_pos + 1;
+        let mut raw = [0u64; 4];
+        raw[lo_pos] = lo;
+        if hi != 0 && hi_pos >= 4 {
+            return None;
+        } else if hi != 0 {
+            raw[hi_pos] = hi;
+        }
+        Some(Self { raw: Uint(raw) })
+    }
+
     /// Get a string representation of a native token amount.
     pub fn to_string_native(&self) -> String {
         DenominatedAmount {

core/src/types/uint.rs

@@ -8,7 +8,7 @@ use std::ops::{Add, AddAssign, BitAnd, Div, Mul, Neg, Rem, Sub, SubAssign};
 use borsh::{BorshDeserialize, BorshSchema, BorshSerialize};
 use impl_num_traits::impl_uint_num_traits;
 use num_integer::Integer;
-use num_traits::CheckedMul;
+use num_traits::{CheckedAdd, CheckedMul, CheckedSub};
 use uint::construct_uint;
 
 use super::dec::{Dec, POS_DECIMAL_PRECISION};
@@ -555,28 +555,6 @@ impl I256 {
         sign
     }
 
-    /// Adds two [`I256`]'s if the absolute value does
-    /// not exceed [`MAX_SIGNED_VALUE`], else returns `None`.
-    pub fn checked_add(&self, other: &Self) -> Option<Self> {
-        if self.non_negative() == other.non_negative() {
-            self.abs().checked_add(other.abs()).and_then(|val| {
-                Self::try_from(val)
-                    .ok()
-                    .map(|val| if !self.non_negative() { -val } else { val })
-            })
-        } else {
-            Some(*self + *other)
-        }
-    }
-
-    /// Subtracts two [`I256`]'s if the absolute value does
-    /// not exceed [`MAX_SIGNED_VALUE`], else returns `None`.
-    pub fn checked_sub(&self, other: &Self) -> Option<Self> {
-        self.checked_add(&other.neg())
-    }
-
-    ///
-
     /// Changed the inner Uint into a canonical representation.
     fn canonical(self) -> Self {
         Self(self.0.canonical())
@@ -738,6 +716,30 @@ impl Mul<Uint> for I256 {
     }
 }
 
+impl CheckedAdd for I256 {
+    /// Adds two [`I256`]'s if the absolute value does
+    /// not exceed [`MAX_SIGNED_VALUE`], else returns `None`.
+    fn checked_add(&self, other: &Self) -> Option<Self> {
+        if self.non_negative() == other.non_negative() {
+            self.abs().checked_add(other.abs()).and_then(|val| {
+                Self::try_from(val)
+                    .ok()
+                    .map(|val| if !self.non_negative() { -val } else { val })
+            })
+        } else {
+            Some(*self + *other)
+        }
+    }
+}
+
+impl CheckedSub for I256 {
+    /// Subtracts two [`I256`]'s if the absolute value does
+    /// not exceed [`MAX_SIGNED_VALUE`], else returns `None`.
+    fn checked_sub(&self, other: &Self) -> Option<Self> {
+        self.checked_add(&other.neg())
+    }
+}
+
 impl CheckedMul for I256 {
     fn checked_mul(&self, v: &Self) -> Option<Self> {
         let is_negative = self.is_negative() != v.is_negative();

proof_of_stake/Cargo.toml

@@ -22,6 +22,7 @@ namada_core = {path = "../core", default-features = false}
 borsh.workspace = true
 data-encoding.workspace = true
 derivative.workspace = true
+num-traits.workspace = true
 once_cell.workspace = true
 proptest = {workspace = true, optional = true}
 serde.workspace = true

proof_of_stake/src/storage.rs

@@ -16,6 +16,7 @@ use namada_core::types::key::{
 };
 use namada_core::types::storage::Epoch;
 use namada_core::types::token;
+use num_traits::CheckedAdd;
 
 use crate::storage_key::consensus_keys_key;
 use crate::types::{

proof_of_stake/src/validator_set_update.rs

@@ -10,6 +10,7 @@ use namada_core::types::address::Address;
 use namada_core::types::key::PublicKeyTmRawHash;
 use namada_core::types::storage::Epoch;
 use namada_core::types::token;
+use num_traits::ops::checked::CheckedAdd;
 use once_cell::unsync::Lazy;
 
 use crate::storage::{

sdk/Cargo.toml

@@ -75,6 +75,7 @@ namada_core = {path = "../core", default-features = false, features = ["rand"]}
 namada_ethereum_bridge = {path = "../ethereum_bridge", default-features = false}
 namada_proof_of_stake = {path = "../proof_of_stake", default-features = false}
 num256.workspace = true
+num-traits.workspace = true
 orion.workspace = true
 owo-colors = "3.5.0"
 parse_duration = "2.1.1"

sdk/src/eth_bridge/bridge_pool.rs

@@ -20,6 +20,7 @@ use namada_core::types::ethereum_events::EthAddress;
 use namada_core::types::keccak::KeccakHash;
 use namada_core::types::token::{balance_key, Amount};
 use namada_core::types::voting_power::FractionalVotingPower;
+use num_traits::ops::checked::CheckedSub;
 use owo_colors::OwoColorize;
 use serde::Serialize;