[audit-fix(#5)] Alloyed asset normalization factor lacks validation

osmosis-labs · May 3, 2024 · 84a2f3a · 84a2f3a
1 parent a5981a8
commit 84a2f3a
Show file tree

Hide file tree

Showing 4 changed files with 106 additions and 16 deletions.
diff --git a/contracts/transmuter/src/asset.rs b/contracts/transmuter/src/asset.rs
@@ -61,13 +61,19 @@ impl Asset {
         amount: impl Into<Uint128>,
         denom: &str,
         normalization_factor: impl Into<Uint128>,
-    ) -> Self {
-        Self {
+    ) -> Result<Self, ContractError> {
+        let normalization_factor = normalization_factor.into();
+        ensure!(
+            normalization_factor > Uint128::zero(),
+            ContractError::NormalizationFactorMustBePositive {}
+        );
+
+        Ok(Self {
             amount: amount.into(),
             denom: denom.to_string(),
-            normalization_factor: normalization_factor.into(),
+            normalization_factor,
             is_corrupted: false,
-        }
+        })
     }
 
     pub fn update_amount<F>(&'_ mut self, f: F) -> Result<&'_ Self, ContractError>

diff --git a/contracts/transmuter/src/migrations/v3_0_0.rs b/contracts/transmuter/src/migrations/v3_0_0.rs
@@ -110,7 +110,7 @@ fn add_normalization_factor_to_pool_assets(
                 }
             })?;
 
-            Ok(Asset::new(coin.amount, &coin.denom, *normalization_factor))
+            Asset::new(coin.amount, &coin.denom, *normalization_factor)
         })
         .collect::<Result<Vec<Asset>, ContractError>>()?;
 
@@ -136,6 +136,11 @@ fn set_alloyed_asset_normalization_factor(
     alloyed_asset_normalization_factor: Uint128,
     storage: &mut dyn Storage,
 ) -> Result<(), ContractError> {
+    ensure!(
+        alloyed_asset_normalization_factor > Uint128::zero(),
+        ContractError::NormalizationFactorMustBePositive {}
+    );
+
     Item::<'_, Uint128>::new(key::ALLOYED_ASSET_NORMALIZATION_FACTOR)
         .save(storage, &alloyed_asset_normalization_factor)?;
 
@@ -235,8 +240,8 @@ mod tests {
         assert_eq!(
             pool_assets,
             vec![
-                Asset::new(10000u128, "denom1", 1000u128),
-                Asset::new(20000u128, "denom2", 10000u128)
+                Asset::new(10000u128, "denom1", 1000u128).unwrap(),
+                Asset::new(20000u128, "denom2", 10000u128).unwrap()
             ]
         );
 
@@ -292,6 +297,78 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_zero_pool_asset_normalization_factor() {
+        let mut deps = mock_dependencies();
+
+        cw2::set_contract_version(&mut deps.storage, CONTRACT_NAME, FROM_VERSION).unwrap();
+
+        Item::new(key::POOL)
+            .save(
+                &mut deps.storage,
+                &TransmuterPoolV2 {
+                    pool_assets: vec![Coin::new(10000, "denom1"), Coin::new(20000, "denom2")],
+                },
+            )
+            .unwrap();
+
+        let migrate_msg = MigrateMsg {
+            asset_configs: vec![
+                AssetConfig {
+                    denom: "denom1".to_string(),
+                    normalization_factor: Uint128::from(1000u128),
+                },
+                AssetConfig {
+                    denom: "denom2".to_string(),
+                    normalization_factor: Uint128::zero(),
+                },
+            ],
+            alloyed_asset_normalization_factor: Uint128::from(2u128),
+            moderator: Some("osmo1cyyzpxplxdzkeea7kwsydadg87357qnahakaks".to_string()),
+        };
+        let msg = migrate_msg;
+
+        let err = execute_migration(deps.as_mut(), msg).unwrap_err();
+
+        assert_eq!(err, ContractError::NormalizationFactorMustBePositive {});
+    }
+
+    #[test]
+    fn test_zero_alloyed_normalization_factor() {
+        let mut deps = mock_dependencies();
+
+        cw2::set_contract_version(&mut deps.storage, CONTRACT_NAME, FROM_VERSION).unwrap();
+
+        Item::new(key::POOL)
+            .save(
+                &mut deps.storage,
+                &TransmuterPoolV2 {
+                    pool_assets: vec![Coin::new(10000, "denom1"), Coin::new(20000, "denom2")],
+                },
+            )
+            .unwrap();
+
+        let migrate_msg = MigrateMsg {
+            asset_configs: vec![
+                AssetConfig {
+                    denom: "denom1".to_string(),
+                    normalization_factor: Uint128::from(1000u128),
+                },
+                AssetConfig {
+                    denom: "denom2".to_string(),
+                    normalization_factor: Uint128::from(1000u128),
+                },
+            ],
+            alloyed_asset_normalization_factor: Uint128::zero(),
+            moderator: Some("osmo1cyyzpxplxdzkeea7kwsydadg87357qnahakaks".to_string()),
+        };
+        let msg = migrate_msg;
+
+        let err = execute_migration(deps.as_mut(), msg).unwrap_err();
+
+        assert_eq!(err, ContractError::NormalizationFactorMustBePositive {});
+    }
+
     #[test]
     fn test_adding_normalization_factor_for_non_pool_asset() {
         let mut deps = mock_dependencies();

diff --git a/contracts/transmuter/src/swap.rs b/contracts/transmuter/src/swap.rs
@@ -763,8 +763,8 @@ mod tests {
                 &mut deps.storage,
                 &TransmuterPool {
                     pool_assets: vec![
-                        Asset::new(Uint128::from(1000u128), "denom1", 1u128),
-                        Asset::new(Uint128::from(1000u128), "denom2", 10u128),
+                        Asset::new(Uint128::from(1000u128), "denom1", 1u128).unwrap(),
+                        Asset::new(Uint128::from(1000u128), "denom2", 10u128).unwrap(),
                     ],
                 },
             )
@@ -900,8 +900,8 @@ mod tests {
                 &mut deps.storage,
                 &TransmuterPool {
                     pool_assets: vec![
-                        Asset::new(Uint128::from(1000000000000u128), "denom1", 1u128),
-                        Asset::new(Uint128::from(1000000000000u128), "denom2", 10u128),
+                        Asset::new(Uint128::from(1000000000000u128), "denom1", 1u128).unwrap(),
+                        Asset::new(Uint128::from(1000000000000u128), "denom2", 10u128).unwrap(),
                     ],
                 },
             )
@@ -997,8 +997,8 @@ mod tests {
                 &mut deps.storage,
                 &TransmuterPool {
                     pool_assets: vec![
-                        Asset::new(Uint128::from(1000000000000u128), "denom1", 1u128),
-                        Asset::new(Uint128::from(1000000000000u128), "denom2", 10u128),
+                        Asset::new(Uint128::from(1000000000000u128), "denom1", 1u128).unwrap(),
+                        Asset::new(Uint128::from(1000000000000u128), "denom2", 10u128).unwrap(),
                     ],
                 },
             )
@@ -1094,8 +1094,8 @@ mod tests {
                 &mut deps.storage,
                 &TransmuterPool {
                     pool_assets: vec![
-                        Asset::new(Uint128::from(1000000000000u128), "denom1", 1u128),
-                        Asset::new(Uint128::from(1000000000000u128), "denom2", 10u128),
+                        Asset::new(Uint128::from(1000000000000u128), "denom1", 1u128).unwrap(),
+                        Asset::new(Uint128::from(1000000000000u128), "denom2", 10u128).unwrap(),
                     ],
                 },
             )

diff --git a/contracts/transmuter/src/transmuter_pool/weight.rs b/contracts/transmuter/src/transmuter_pool/weight.rs
@@ -157,7 +157,14 @@ mod tests {
             ("c".to_string(), Decimal::from_ratio(6000u128, 400_000_012_000u128))
         ]
     )]
-    fn test_all_ratios(#[case] pool_assets: Vec<Asset>, #[case] expected: Vec<(String, Decimal)>) {
+    fn test_all_ratios(
+        #[case] pool_assets: Vec<Result<Asset, ContractError>>,
+        #[case] expected: Vec<(String, Decimal)>,
+    ) {
+        let pool_assets = pool_assets
+            .into_iter()
+            .map(|asset| asset.unwrap())
+            .collect();
         let pool = TransmuterPool { pool_assets };
 
         let ratios = pool.weights().unwrap();