Skip to content

Commit

Permalink
[audit-fix(#2)] Resetting change limiters creates an opportunity wind…
Browse files Browse the repository at this point in the history 
…ow for economic attacks
  • Loading branch information
@iboss-ptk
iboss-ptk committed May 6, 2024
1 parent 4ec82e2 commit 7064908
Show file tree
Hide file tree
Showing 6 changed files with 160 additions and 60 deletions.
53 changes: 42 additions & 11 deletions contracts/transmuter/src/contract.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ impl Transmuter<'_> {
#[sv::msg(exec)]
fn add_new_assets(
&self,
ExecCtx { deps, env: _, info }: ExecCtx,
ExecCtx { deps, env, info }: ExecCtx,
asset_configs: Vec<AssetConfig>,
) -> Result<Response, ContractError> {
non_empty_input_required("asset_configs", &asset_configs)?;
Expand Down Expand Up @@ -217,10 +217,11 @@ impl Transmuter<'_> {
pool.add_new_assets(assets)?;
self.pool.save(deps.storage, &pool)?;

// staled divisions in change limiters has become invalid after
// new assets are added to the pool
// so we reset change limiter states
self.limiters.reset_change_limiter_states(deps.storage)?;
self.limiters.reset_change_limiter_states(
deps.storage,
env.block.time,
pool.weights()?.unwrap_or_default(),
)?;

Ok(Response::new().add_attribute("method", "add_new_assets"))
}
Expand Down Expand Up @@ -882,6 +883,7 @@ pub struct GetModeratorResponse {

#[cfg(test)]
mod tests {

use super::sv::*;
use super::*;
use crate::limiter::{ChangeLimiter, StaticLimiter, WindowConfig};
Expand Down Expand Up @@ -1004,9 +1006,20 @@ mod tests {
}

// join pool a bit more to make limiters dirty
let mut env = env.clone();
env.block.time = env.block.time.plus_nanos(360);

let info = mock_info(
"someone",
&[Coin::new(550, "uosmo"), Coin::new(500, "uion")],
);
let join_pool_msg = ContractExecMsg::Transmuter(ExecMsg::JoinPool {});
execute(deps.as_mut(), env.clone(), info.clone(), join_pool_msg).unwrap();

env.block.time = env.block.time.plus_nanos(3000);
let info = mock_info(
"someone",
&[Coin::new(1000, "uosmo"), Coin::new(1000, "uion")],
&[Coin::new(450, "uosmo"), Coin::new(500, "uion")],
);
let join_pool_msg = ContractExecMsg::Transmuter(ExecMsg::JoinPool {});
execute(deps.as_mut(), env.clone(), info.clone(), join_pool_msg).unwrap();
Expand All @@ -1031,6 +1044,8 @@ mod tests {
.collect(),
});

env.block.time = env.block.time.plus_nanos(360);

let res = execute(
deps.as_mut(),
env.clone(),
Expand All @@ -1054,6 +1069,8 @@ mod tests {
.collect(),
});

env.block.time = env.block.time.plus_nanos(360);

// Attempt to add assets by non-admin
let non_admin_info = mock_info("non_admin", &[]);
let res = execute(
Expand All @@ -1070,17 +1087,26 @@ mod tests {
"Adding assets by non-admin should be unauthorized"
);

env.block.time = env.block.time.plus_nanos(360);

// successful asset addition
execute(deps.as_mut(), env.clone(), info, add_assets_msg).unwrap();

let reset_at = env.block.time;
let transmuter = Transmuter::new();

// Reset change limiter states if new assets are added
for denom in ["uosmo", "uion"] {
assert_clean_change_limiters_by_denom!(
assert_reset_change_limiters_by_denom!(
denom,
Transmuter::new().limiters,
reset_at,
transmuter,
deps.as_ref().storage
);
}

env.block.time = env.block.time.plus_nanos(360);

// Check if the new assets were added
let res = query(
deps.as_ref(),
Expand Down Expand Up @@ -1550,9 +1576,14 @@ mod tests {
vec![]
);

assert_clean_change_limiters_by_denom!("tbtc", Transmuter::new().limiters, &deps.storage);
assert_clean_change_limiters_by_denom!("nbtc", Transmuter::new().limiters, &deps.storage);
assert_clean_change_limiters_by_denom!("stbtc", Transmuter::new().limiters, &deps.storage);
for denom in ["tbtc", "nbtc", "stbtc"] {
assert_reset_change_limiters_by_denom!(
denom,
env.block.time,
Transmuter::new(),
deps.as_ref().storage
);
}

// try unmark nbtc should fail
let unmark_corrupted_assets_msg =
Expand Down
4 changes: 2 additions & 2 deletions contracts/transmuter/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,10 @@ mod entry_points {
#[entry_point]
pub fn migrate(
deps: DepsMut,
_env: Env,
env: Env,
msg: migrations::v3_0_0::MigrateMsg,
) -> Result<Response, ContractError> {
migrations::v3_0_0::execute_migration(deps, msg)
migrations::v3_0_0::execute_migration(deps, env, msg)
}
}

Expand Down
96 changes: 67 additions & 29 deletions contracts/transmuter/src/limiter/limiters.rs
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::collections::HashMap;

use cosmwasm_schema::cw_serde;
use cosmwasm_std::{ensure, Decimal, StdError, Storage, Timestamp, Uint64};
use cw_storage_plus::Map;
Expand Down Expand Up @@ -79,6 +81,14 @@ impl ChangeLimiter {
.ensure_window_config_constraint()
}

pub fn divisions(&self) -> &[Division] {
&self.divisions
}

pub fn latest_value(&self) -> Decimal {
self.latest_value
}

pub fn reset(self) -> Self {
Self {
divisions: vec![],
Expand Down Expand Up @@ -540,22 +550,33 @@ impl<'a> Limiters<'a> {
Ok(())
}

/// If the normalization factor updated, or new asset being added, staled divisions will become invalid.
/// This function cleans up the staled divisions.
/// If the normalization factor has a non-uniform update, staled divisions will become invalid.
/// In case of adding new assets, even if there is nothing wrong with the normalization factor,
/// the asset composition change required some time to be properly reflected.
///
/// This function cleans up the staled divisions and create new division with updated state,
/// which is a start over with the new asset composition and normalization factor.
pub fn reset_change_limiter_states(
&self,
storage: &mut dyn Storage,
block_time: Timestamp,
weights: Vec<(String, Decimal)>,
) -> Result<(), ContractError> {
// there is no need to limit, since the number of limiters is expected to be small
let limiters = self.list_limiters(storage)?;
let weights: HashMap<String, Decimal> = weights.into_iter().collect();

for ((denom, label), limiter) in limiters {
match limiter {
Limiter::ChangeLimiter(limiter) => self.limiters.save(
storage,
(denom.as_str(), label.as_str()),
&Limiter::ChangeLimiter(limiter.reset()),
)?,
Limiter::ChangeLimiter(limiter) => {
self.limiters
.save(storage, (denom.as_str(), label.as_str()), {
let value = weights.get(denom.as_str()).copied().ok_or_else(|| {
StdError::not_found(format!("weight for {}", denom))
})?;
&Limiter::ChangeLimiter(limiter.reset().update(block_time, value)?)
})?
}
Limiter::StaticLimiter(_) => {}
};
}
Expand All @@ -567,24 +588,28 @@ impl<'a> Limiters<'a> {
/// This is used for testing if all change limiters has been newly created or reset.
#[cfg(test)]
#[macro_export]
macro_rules! assert_clean_change_limiters_by_denom {
($denom:expr, $lim:expr, $storage:expr) => {
let limiters = $lim
macro_rules! assert_reset_change_limiters_by_denom {
($denom:expr, $reset_at:expr, $transmuter:expr, $storage:expr) => {
let pool = $transmuter.pool.load($storage).unwrap();
let weights = pool
.weights()
.unwrap()
.unwrap_or_default()
.into_iter()
.collect::<std::collections::HashMap<_, _>>();

let limiters = $transmuter
.limiters
.list_limiters_by_denom($storage, $denom)
.expect("failed to list limiters");

for (label, limiter) in limiters {
match limiter {
Limiter::ChangeLimiter(limiter) => {
assert_eq!(
limiter,
limiter.clone().reset(),
"Change Limiter `{}/{}` is dirty but expect clean",
$denom,
label
);
}
Limiter::StaticLimiter(_) => {}
for (_label, limiter) in limiters {
if let $crate::limiter::Limiter::ChangeLimiter(limiter) = limiter {
let value = *weights.get($denom).unwrap();
assert_eq!(
limiter.divisions(),
&[$crate::limiter::Division::new($reset_at, $reset_at, value, value).unwrap()]
)
};
}
};
Expand All @@ -602,6 +627,7 @@ macro_rules! assert_dirty_change_limiters_by_denom {
for (label, limiter) in limiters {
match limiter {
Limiter::ChangeLimiter(limiter) => {
limiter.divisions();
assert_ne!(
limiter,
limiter.clone().reset(),
Expand Down Expand Up @@ -2772,11 +2798,11 @@ mod tests {
// update limiters

let block_time = Timestamp::from_nanos(1661231280000000000);
let value_a = Decimal::one();
let value = Decimal::one();
limiters
.check_limits_and_update(
&mut deps.storage,
vec![("denoma".to_string(), value_a)],
vec![("denoma".to_string(), value)],
block_time,
)
.unwrap();
Expand All @@ -2790,23 +2816,35 @@ mod tests {
for (denom, window) in keys.iter() {
let divisions =
list_divisions(&limiters, denom.as_str(), window.as_str(), &deps.storage);
assert_eq!(divisions.len(), 1);

assert_eq!(
divisions,
vec![Division::new(block_time, block_time, value, value).unwrap()]
)
}

assert_dirty_change_limiters_by_denom!("denoma", &limiters, &deps.storage);

// reset limiters
let block_time = block_time.plus_hours(1);
let value = Decimal::percent(2);
limiters
.reset_change_limiter_states(&mut deps.storage)
.reset_change_limiter_states(
&mut deps.storage,
block_time,
vec![("denoma".to_string(), value)],
)
.unwrap();

for (denom, window) in keys.iter() {
let divisions =
list_divisions(&limiters, denom.as_str(), window.as_str(), &deps.storage);
assert_eq!(divisions.len(), 0);
}

assert_clean_change_limiters_by_denom!("denoma", &limiters, &deps.storage);
assert_eq!(
divisions,
vec![Division::new(block_time, block_time, value, value).unwrap()]
);
}
}
}

Expand Down
2 changes: 2 additions & 0 deletions contracts/transmuter/src/limiter/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,7 @@ mod limiters;

pub use limiters::{Limiter, LimiterParams, Limiters};

#[cfg(test)]
pub use division::Division;
#[cfg(test)]
pub use limiters::{ChangeLimiter, StaticLimiter, WindowConfig};
Loading

0 comments on commit 7064908

Please sign in to comment.