Merge pull request #24 from osmosis-labs/audit-fix/unbounded-loop

[FIX-3] Unbounded loops could render main features unusable

contracts/transmuter/src/contract.rs

@@ -83,7 +83,7 @@ impl Transmuter<'_> {
 
         // store pool
         self.pool
-            .save(deps.storage, &TransmuterPool::new(&pool_asset_denoms))?;
+            .save(deps.storage, &TransmuterPool::new(&pool_asset_denoms)?)?;
 
         // set active status to true
         self.active_status.save(deps.storage, &true)?;

contracts/transmuter/src/error.rs

@@ -36,6 +36,13 @@ pub enum ContractError {
     #[error("Not a pool asset denom: {denom}")]
     InvalidPoolAssetDenom { denom: String },
 
+    #[error("Pool asset denom count must be within {min} - {max} inclusive, but got: {actual}")]
+    PoolAssetDenomCountOutOfRange {
+        min: Uint64,
+        max: Uint64,
+        actual: Uint64,
+    },
+
     #[error("Insufficient pool asset: required: {required}, available: {available}")]
     InsufficientPoolAsset { required: Coin, available: Coin },
 
@@ -82,6 +89,9 @@ pub enum ContractError {
     #[error("Unauthorized")]
     Unauthorized {},
 
+    #[error("Limiter count for {denom} exceed maximum per denom: {max}")]
+    MaxLimiterCountPerDenomExceeded { denom: String, max: Uint64 },
+
     #[error("Limiter label must not be empty")]
     EmptyLimiterLabel {},
 

contracts/transmuter/src/limiter/limiters.rs

@@ -11,6 +11,11 @@ use super::division::Division;
 /// which will cause high gas usage when checking the limit, cleaning up divisions, etc.
 const MAX_DIVISION_COUNT: Uint64 = Uint64::new(10u64);
 
+/// Maximum number of limiters allowed per denom.
+/// This limited so that the contract can't be abused by setting a large number of limiters,
+/// causing high gas usage when checking the limit, cleaning up divisions, etc.
+const MAX_LIMITER_COUNT_PER_DENOM: Uint64 = Uint64::new(10u64);
+
 #[cw_serde]
 pub struct WindowConfig {
     /// Size of the window in nanoseconds
@@ -309,6 +314,16 @@ impl<'a> Limiters<'a> {
             }
         };
 
+        // ensure limiters for the denom has not yet reached the maximum
+        let limiter_count_for_denom = self.list_limiters_by_denom(storage, denom)?.len() as u64;
+        ensure!(
+            limiter_count_for_denom < MAX_LIMITER_COUNT_PER_DENOM.u64(),
+            ContractError::MaxLimiterCountPerDenomExceeded {
+                denom: denom.to_string(),
+                max: MAX_LIMITER_COUNT_PER_DENOM
+            }
+        );
+
         self.limiters
             .save(storage, (denom, label), &limiter)
             .map_err(Into::into)
@@ -730,6 +745,75 @@ mod tests {
             );
         }
 
+        #[test]
+        fn test_register_limiter_exceed_max_limiter_per_denom() {
+            let mut deps = mock_dependencies();
+            let limiter = Limiters::new("limiters");
+
+            for h in 1..10u64 {
+                let label = format!("{}h", h);
+                let result = limiter.register(
+                    &mut deps.storage,
+                    "denoma",
+                    &label,
+                    LimiterParams::ChangeLimiter {
+                        window_config: WindowConfig {
+                            window_size: Uint64::from(3_600_000_000_000u64 * h),
+                            division_count: Uint64::from(5u64),
+                        },
+                        boundary_offset: Decimal::percent(10),
+                    },
+                );
+
+                if h <= 10 {
+                    assert!(result.is_ok());
+                } else {
+                    assert_eq!(
+                        result.unwrap_err(),
+                        ContractError::MaxLimiterCountPerDenomExceeded {
+                            denom: "denoma".to_string(),
+                            max: MAX_LIMITER_COUNT_PER_DENOM
+                        }
+                    );
+                }
+            }
+
+            // deregister to register should work
+            limiter.deregister(&mut deps.storage, "denoma", "0h");
+
+            // register static limiter
+            limiter
+                .register(
+                    &mut deps.storage,
+                    "denoma",
+                    "static",
+                    LimiterParams::StaticLimiter {
+                        upper_limit: Decimal::percent(10),
+                    },
+                )
+                .unwrap();
+
+            // register another one should fail
+            let err = limiter
+                .register(
+                    &mut deps.storage,
+                    "denoma",
+                    "static2",
+                    LimiterParams::StaticLimiter {
+                        upper_limit: Decimal::percent(9),
+                    },
+                )
+                .unwrap_err();
+
+            assert_eq!(
+                err,
+                ContractError::MaxLimiterCountPerDenomExceeded {
+                    denom: "denoma".to_string(),
+                    max: MAX_LIMITER_COUNT_PER_DENOM
+                }
+            );
+        }
+
         #[test]
         fn test_deregister() {
             let mut deps = mock_dependencies();

contracts/transmuter/src/transmuter_pool/join_pool.rs

@@ -47,7 +47,8 @@ mod tests {
     #[test]
     fn test_join_pool_increasingly() {
         let mut pool =
-            TransmuterPool::new(&[Denom::unchecked(ETH_USDC), Denom::unchecked(COSMOS_USDC)]);
+            TransmuterPool::new(&[Denom::unchecked(ETH_USDC), Denom::unchecked(COSMOS_USDC)])
+                .unwrap();
 
         // join pool
         pool.join_pool(&[Coin::new(1000, COSMOS_USDC)]).unwrap();
@@ -75,7 +76,8 @@ mod tests {
     #[test]
     fn test_join_pool_error_with_wrong_denom() {
         let mut pool =
-            TransmuterPool::new(&[Denom::unchecked(ETH_USDC), Denom::unchecked(COSMOS_USDC)]);
+            TransmuterPool::new(&[Denom::unchecked(ETH_USDC), Denom::unchecked(COSMOS_USDC)])
+                .unwrap();
 
         assert_eq!(
             pool.join_pool(&[Coin::new(1000, "urandom")]).unwrap_err(),
@@ -100,7 +102,8 @@ mod tests {
     #[test]
     fn test_join_pool_error_with_overflow() {
         let mut pool =
-            TransmuterPool::new(&[Denom::unchecked(ETH_USDC), Denom::unchecked(COSMOS_USDC)]);
+            TransmuterPool::new(&[Denom::unchecked(ETH_USDC), Denom::unchecked(COSMOS_USDC)])
+                .unwrap();
 
         assert_eq!(
             {

contracts/transmuter/src/transmuter_pool/mod.rs

@@ -4,9 +4,18 @@ mod transmute;
 mod weight;
 
 use cosmwasm_schema::cw_serde;
-use cosmwasm_std::Coin;
+use cosmwasm_std::{ensure, Coin, Uint64};
 
-use crate::denom::Denom;
+use crate::{denom::Denom, ContractError};
+
+/// Minimum number of pool assets. This is required since if the
+/// number of pool assets is less than 2, then the contract will
+/// not function as a pool.
+const MIN_POOL_ASSET_DENOMS: Uint64 = Uint64::new(2u64);
+
+/// Maximum number of pool assets. This is required in order to
+/// prevent the contract from running out of gas when iterating
+const MAX_POOL_ASSET_DENOMS: Uint64 = Uint64::new(20u64);
 
 #[cw_serde]
 pub struct TransmuterPool {
@@ -15,14 +24,76 @@ pub struct TransmuterPool {
 }
 
 impl TransmuterPool {
-    pub fn new(pool_asset_denoms: &[Denom]) -> Self {
-        assert!(pool_asset_denoms.len() >= 2);
+    pub fn new(pool_asset_denoms: &[Denom]) -> Result<Self, ContractError> {
+        let count = Uint64::new(pool_asset_denoms.len() as u64);
+
+        ensure!(
+            count >= MIN_POOL_ASSET_DENOMS && count <= MAX_POOL_ASSET_DENOMS,
+            ContractError::PoolAssetDenomCountOutOfRange {
+                min: MIN_POOL_ASSET_DENOMS,
+                max: MAX_POOL_ASSET_DENOMS,
+                actual: count
+            }
+        );
 
-        Self {
+        Ok(Self {
             pool_assets: pool_asset_denoms
                 .iter()
                 .map(|denom| Coin::new(0, denom.as_str()))
                 .collect(),
-        }
+        })
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_denom_count_within_range() {
+        // 1 denom
+        assert_eq!(
+            TransmuterPool::new(&[Denom::unchecked("a")]).unwrap_err(),
+            ContractError::PoolAssetDenomCountOutOfRange {
+                min: MIN_POOL_ASSET_DENOMS,
+                max: MAX_POOL_ASSET_DENOMS,
+                actual: Uint64::new(1),
+            }
+        );
+
+        // 2 denoms
+        assert_eq!(
+            TransmuterPool::new(&[Denom::unchecked("a"), Denom::unchecked("b")]).unwrap(),
+            TransmuterPool {
+                pool_assets: vec![Coin::new(0, "a"), Coin::new(0, "b"),],
+            }
+        );
+
+        // 20 denoms
+        let denoms: Vec<Denom> = (0..20)
+            .map(|i| Denom::unchecked(&format!("d{}", i)))
+            .collect();
+        assert_eq!(
+            TransmuterPool::new(&denoms).unwrap(),
+            TransmuterPool {
+                pool_assets: denoms
+                    .iter()
+                    .map(|denom| Coin::new(0, denom.as_str()))
+                    .collect(),
+            }
+        );
+
+        // 21 denoms should fail
+        let denoms: Vec<Denom> = (0..21)
+            .map(|i| Denom::unchecked(&format!("d{}", i)))
+            .collect();
+        assert_eq!(
+            TransmuterPool::new(&denoms).unwrap_err(),
+            ContractError::PoolAssetDenomCountOutOfRange {
+                min: MIN_POOL_ASSET_DENOMS,
+                max: MAX_POOL_ASSET_DENOMS,
+                actual: Uint64::new(21),
+            }
+        );
     }
 }

contracts/transmuter/src/transmuter_pool/transmute.rs

@@ -85,7 +85,8 @@ mod tests {
     #[test]
     fn test_transmute_succeed() {
         let mut pool =
-            TransmuterPool::new(&[Denom::unchecked(ETH_USDC), Denom::unchecked(COSMOS_USDC)]);
+            TransmuterPool::new(&[Denom::unchecked(ETH_USDC), Denom::unchecked(COSMOS_USDC)])
+                .unwrap();
 
         pool.join_pool(&[Coin::new(70_000, COSMOS_USDC)]).unwrap();
         assert_eq!(
@@ -136,7 +137,8 @@ mod tests {
     #[test]
     fn test_transmute_token_out_denom_eq_token_in_denom() {
         let mut pool =
-            TransmuterPool::new(&[Denom::unchecked(ETH_USDC), Denom::unchecked(COSMOS_USDC)]);
+            TransmuterPool::new(&[Denom::unchecked(ETH_USDC), Denom::unchecked(COSMOS_USDC)])
+                .unwrap();
         pool.join_pool(&[Coin::new(70_000, COSMOS_USDC)]).unwrap();
 
         let token_in = Coin::new(70_000, COSMOS_USDC);
@@ -146,7 +148,8 @@ mod tests {
     #[test]
     fn test_transmute_fail_token_out_not_enough() {
         let mut pool =
-            TransmuterPool::new(&[Denom::unchecked(ETH_USDC), Denom::unchecked(COSMOS_USDC)]);
+            TransmuterPool::new(&[Denom::unchecked(ETH_USDC), Denom::unchecked(COSMOS_USDC)])
+                .unwrap();
 
         pool.join_pool(&[Coin::new(70_000, COSMOS_USDC)]).unwrap();
         assert_eq!(
@@ -162,7 +165,8 @@ mod tests {
     #[test]
     fn test_transmute_fail_token_in_not_allowed() {
         let mut pool =
-            TransmuterPool::new(&[Denom::unchecked(ETH_USDC), Denom::unchecked(COSMOS_USDC)]);
+            TransmuterPool::new(&[Denom::unchecked(ETH_USDC), Denom::unchecked(COSMOS_USDC)])
+                .unwrap();
 
         pool.join_pool(&[Coin::new(70_000, COSMOS_USDC)]).unwrap();
         assert_eq!(
@@ -178,7 +182,8 @@ mod tests {
     #[test]
     fn test_transmute_fail_token_out_denom_not_allowed() {
         let mut pool =
-            TransmuterPool::new(&[Denom::unchecked(ETH_USDC), Denom::unchecked(COSMOS_USDC)]);
+            TransmuterPool::new(&[Denom::unchecked(ETH_USDC), Denom::unchecked(COSMOS_USDC)])
+                .unwrap();
 
         pool.join_pool(&[Coin::new(70_000, COSMOS_USDC)]).unwrap();
         assert_eq!(