From 6c8d221f5b1c46fbb0a68c83b83f35374d187ae2 Mon Sep 17 00:00:00 2001 From: maltekliemann Date: Mon, 28 Mar 2022 17:12:08 +0200 Subject: [PATCH 1/5] Check that pool_join-pool_exit is safe --- Cargo.lock | 30 ++++++++++++------------ zrml/swaps/src/tests.rs | 51 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ba010090b..fb382c82c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11903,7 +11903,7 @@ dependencies = [ [[package]] name = "zeitgeist-node" -version = "0.2.4" +version = "0.3.0" dependencies = [ "cfg-if 1.0.0", "cumulus-client-cli", @@ -11978,7 +11978,7 @@ dependencies = [ [[package]] name = "zeitgeist-primitives" -version = "0.2.4" +version = "0.3.0" dependencies = [ "arbitrary", "frame-support", @@ -11995,7 +11995,7 @@ dependencies = [ [[package]] name = "zeitgeist-runtime" -version = "0.2.4" +version = "0.3.0" dependencies = [ "cfg-if 1.0.0", "cumulus-pallet-dmp-queue", @@ -12093,7 +12093,7 @@ dependencies = [ [[package]] name = "zrml-authorized" -version = "0.2.4" +version = "0.3.0" dependencies = [ "frame-benchmarking", "frame-support", @@ -12110,7 +12110,7 @@ dependencies = [ [[package]] name = "zrml-court" -version = "0.2.4" +version = "0.3.0" dependencies = [ "arrayvec 0.7.2", "frame-benchmarking", @@ -12130,7 +12130,7 @@ dependencies = [ [[package]] name = "zrml-liquidity-mining" -version = "0.2.4" +version = "0.3.0" dependencies = [ "frame-benchmarking", "frame-support", @@ -12148,7 +12148,7 @@ dependencies = [ [[package]] name = "zrml-market-commons" -version = "0.2.4" +version = "0.3.0" dependencies = [ "frame-support", "frame-system", @@ -12161,7 +12161,7 @@ dependencies = [ [[package]] name = "zrml-orderbook-v1" -version = "0.2.4" +version = "0.3.0" dependencies = [ "frame-benchmarking", "frame-support", @@ -12190,7 +12190,7 @@ dependencies = [ [[package]] name = "zrml-prediction-markets" -version = "0.2.4" +version = "0.3.0" dependencies = [ "frame-benchmarking", "frame-support", @@ -12234,7 +12234,7 @@ dependencies = [ [[package]] name = "zrml-prediction-markets-runtime-api" -version = "0.2.4" +version = "0.3.0" dependencies = [ "parity-scale-codec 2.3.1", "sp-api", @@ -12243,7 +12243,7 @@ dependencies = [ [[package]] name = "zrml-rikiddo" -version = "0.2.4" +version = "0.3.0" dependencies = [ "arbitrary", "cfg-if 1.0.0", @@ -12276,7 +12276,7 @@ dependencies = [ [[package]] name = "zrml-simple-disputes" -version = "0.2.4" +version = "0.3.0" dependencies = [ "frame-benchmarking", "frame-support", @@ -12293,7 +12293,7 @@ dependencies = [ [[package]] name = "zrml-swaps" -version = "0.2.4" +version = "0.3.0" dependencies = [ "frame-benchmarking", "frame-support", @@ -12330,7 +12330,7 @@ dependencies = [ [[package]] name = "zrml-swaps-rpc" -version = "0.2.4" +version = "0.3.0" dependencies = [ "jsonrpc-core", "jsonrpc-core-client", @@ -12345,7 +12345,7 @@ dependencies = [ [[package]] name = "zrml-swaps-runtime-api" -version = "0.2.4" +version = "0.3.0" dependencies = [ "parity-scale-codec 2.3.1", "sp-api", diff --git a/zrml/swaps/src/tests.rs b/zrml/swaps/src/tests.rs index a3dafacb2..597d94829 100644 --- a/zrml/swaps/src/tests.rs +++ b/zrml/swaps/src/tests.rs @@ -1096,6 +1096,57 @@ fn create_pool_fails_on_too_few_assets() { }); } +// Macro for comparing fixed point u128. +macro_rules! assert_approx { + ($left:expr, $right:expr, $precision:expr $(,)?) => { + match (&$left, &$right, &$precision) { + (left_val, right_val, precision_val) => { + let diff = if *left_val > *right_val { + *left_val - *right_val + } else { + *right_val - *left_val + }; + if diff > $precision { + panic!("{} is not {}-close to {}", *left_val, *precision_val, *right_val); + } + } + } + }; +} + +#[test] +fn join_pool_exit_pool_does_not_create_extra_tokens() { + ExtBuilder::default().build().execute_with(|| { + create_initial_pool_with_funds_for_alice(ScoringRule::CPMM, true); + + ASSETS.iter().cloned().for_each(|asset| { + let _ = Currencies::deposit(asset, &CHARLIE, _100); + }); + + let amount = 123_456_789_123; // Strange number to force rounding errors! + assert_ok!(Swaps::pool_join( + Origin::signed(CHARLIE), + 0, + amount, + vec![_10000, _10000, _10000, _10000] + )); + assert_ok!(Swaps::pool_exit(Origin::signed(CHARLIE), 0, amount, vec![0, 0, 0, 0])); + + // It's not true that the balances are _exactly_ the same. But they only vary up to + // negligible precision! + let pool_account_id = Swaps::pool_account_id(0); + let precision = 30; + assert_approx!(Currencies::free_balance(ASSET_A, &pool_account_id), _100, precision); + assert_approx!(Currencies::free_balance(ASSET_B, &pool_account_id), _100, precision); + assert_approx!(Currencies::free_balance(ASSET_C, &pool_account_id), _100, precision); + assert_approx!(Currencies::free_balance(ASSET_D, &pool_account_id), _100, precision); + assert_approx!(Currencies::free_balance(ASSET_A, &CHARLIE), _100, precision); + assert_approx!(Currencies::free_balance(ASSET_B, &CHARLIE), _100, precision); + assert_approx!(Currencies::free_balance(ASSET_C, &CHARLIE), _100, precision); + assert_approx!(Currencies::free_balance(ASSET_D, &CHARLIE), _100, precision); + }); +} + fn alice_signed() -> Origin { Origin::signed(ALICE) } From b386e8492b8df4c11100e48e7b50ea75b5ae073a Mon Sep 17 00:00:00 2001 From: maltekliemann Date: Mon, 28 Mar 2022 17:40:37 +0200 Subject: [PATCH 2/5] Verify that minimum/maximum weights are respected --- zrml/swaps/src/tests.rs | 42 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/zrml/swaps/src/tests.rs b/zrml/swaps/src/tests.rs index 597d94829..ad767a290 100644 --- a/zrml/swaps/src/tests.rs +++ b/zrml/swaps/src/tests.rs @@ -1147,6 +1147,48 @@ fn join_pool_exit_pool_does_not_create_extra_tokens() { }); } +#[test] +fn create_pool_fails_on_weight_below_minimum_weight() { + ExtBuilder::default().build().execute_with(|| { + ASSETS.iter().cloned().for_each(|asset| { + let _ = Currencies::deposit(asset, &BOB, _100); + }); + assert_noop!( + Swaps::create_pool( + BOB, + ASSETS.iter().cloned().collect(), + Some(ASSETS.last().unwrap().clone()), + 0, + ScoringRule::CPMM, + Some(0), + Some(vec!(_2, ::MinWeight::get() - 1, _2, _2)) + ), + crate::Error::::BelowMinimumWeight, + ); + }); +} + +#[test] +fn create_pool_fails_on_weight_above_maximum_weight() { + ExtBuilder::default().build().execute_with(|| { + ASSETS.iter().cloned().for_each(|asset| { + let _ = Currencies::deposit(asset, &BOB, _100); + }); + assert_noop!( + Swaps::create_pool( + BOB, + ASSETS.iter().cloned().collect(), + Some(ASSETS.last().unwrap().clone()), + 0, + ScoringRule::CPMM, + Some(0), + Some(vec!(_2, ::MaxWeight::get() + 1, _2, _2)) + ), + crate::Error::::AboveMaximumWeight, + ); + }); +} + fn alice_signed() -> Origin { Origin::signed(ALICE) } From 256281afe6952208114f4b544166b82b2c54fa24 Mon Sep 17 00:00:00 2001 From: maltekliemann Date: Mon, 28 Mar 2022 19:06:32 +0200 Subject: [PATCH 3/5] Check that total weight does not exceed maximum --- zrml/swaps/src/tests.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/zrml/swaps/src/tests.rs b/zrml/swaps/src/tests.rs index ad767a290..37788b790 100644 --- a/zrml/swaps/src/tests.rs +++ b/zrml/swaps/src/tests.rs @@ -1189,6 +1189,28 @@ fn create_pool_fails_on_weight_above_maximum_weight() { }); } +#[test] +fn create_pool_fails_on_total_weight_above_maximum_total_weight() { + ExtBuilder::default().build().execute_with(|| { + ASSETS.iter().cloned().for_each(|asset| { + let _ = Currencies::deposit(asset, &BOB, _100); + }); + let weight = ::MaxTotalWeight::get() / 4 + 100; + assert_noop!( + Swaps::create_pool( + BOB, + ASSETS.iter().cloned().collect(), + Some(ASSETS.last().unwrap().clone()), + 0, + ScoringRule::CPMM, + Some(0), + Some(vec![weight; 4]), + ), + crate::Error::::MaxTotalWeight, + ); + }); +} + fn alice_signed() -> Origin { Origin::signed(ALICE) } From 5ac736d6d08e7295c6afeecc566aeadd44086a51 Mon Sep 17 00:00:00 2001 From: maltekliemann Date: Mon, 28 Mar 2022 19:11:27 +0200 Subject: [PATCH 4/5] Check that all pool join/exit's raise on zero `util::pool` raises if either `pool_amount` or `asset_amount` is zero. This is an unfortunate result of the abstraction of `pool_join` and `pool_exit` into `util::pool`, but it does protect the user from paying for something they get nothing for. The other four functions only raise an error if what the joining/exiting user pays is zero. So this protects the LPs, but not the interacting user. --- zrml/swaps/src/lib.rs | 3 +++ zrml/swaps/src/tests.rs | 22 +++++++++++++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/zrml/swaps/src/lib.rs b/zrml/swaps/src/lib.rs index 99c24987c..b485bb742 100644 --- a/zrml/swaps/src/lib.rs +++ b/zrml/swaps/src/lib.rs @@ -292,6 +292,7 @@ mod pallet { pool_amount: BalanceOf, min_asset_amount: BalanceOf, ) -> DispatchResult { + ensure!(pool_amount != Zero::zero(), Error::::MathApproximation); let pool = Self::pool_by_id(pool_id)?; let pool_ref = &pool; let who = ensure_signed(origin)?; @@ -313,6 +314,7 @@ mod pallet { pool.swap_fee.ok_or(Error::::PoolMissingFee)?.saturated_into(), )? .saturated_into(); + ensure!(asset_amount != Zero::zero(), Error::::MathApproximation); ensure!(asset_amount >= min_asset_amount, Error::::LimitOut); ensure!( asset_amount @@ -1554,6 +1556,7 @@ mod pallet { asset_amount: BalanceOf, min_pool_amount: BalanceOf, ) -> Result { + ensure!(asset_amount != Zero::zero(), Error::::MathApproximation); let pool = Pallet::::pool_by_id(pool_id)?; let pool_ref = &pool; let pool_account_id = Pallet::::pool_account_id(pool_id); diff --git a/zrml/swaps/src/tests.rs b/zrml/swaps/src/tests.rs index 37788b790..88890c8bf 100644 --- a/zrml/swaps/src/tests.rs +++ b/zrml/swaps/src/tests.rs @@ -555,7 +555,7 @@ fn out_amount_must_be_equal_or_less_than_max_out_ratio() { } #[test] -fn pool_amount_must_not_be_zero() { +fn pool_join_or_exit_raises_on_zero_value() { ExtBuilder::default().build().execute_with(|| { create_initial_pool_with_funds_for_alice(ScoringRule::CPMM, true); @@ -568,6 +568,26 @@ fn pool_amount_must_not_be_zero() { Swaps::pool_exit(alice_signed(), 0, 0, vec!(_1, _1, _1, _1)), crate::Error::::MathApproximation ); + + assert_noop!( + Swaps::pool_join_with_exact_pool_amount(alice_signed(), 0, ASSET_A, 0, 0), + crate::Error::::MathApproximation + ); + + assert_noop!( + Swaps::pool_join_with_exact_asset_amount(alice_signed(), 0, ASSET_A, 0, 0), + crate::Error::::MathApproximation + ); + + assert_noop!( + Swaps::pool_exit_with_exact_pool_amount(alice_signed(), 0, ASSET_A, 0, 0), + crate::Error::::MathApproximation + ); + + assert_noop!( + Swaps::pool_exit_with_exact_asset_amount(alice_signed(), 0, ASSET_A, 0, 0), + crate::Error::::MathApproximation + ); }); } From 05e10782a42d70d96c433ab1f14c6fdbe3049977 Mon Sep 17 00:00:00 2001 From: maltekliemann Date: Mon, 28 Mar 2022 21:03:28 +0200 Subject: [PATCH 5/5] Fix test --- zrml/prediction-markets/src/tests.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/zrml/prediction-markets/src/tests.rs b/zrml/prediction-markets/src/tests.rs index 5cdd90f40..a8d9ac815 100644 --- a/zrml/prediction-markets/src/tests.rs +++ b/zrml/prediction-markets/src/tests.rs @@ -686,7 +686,9 @@ fn create_market_and_deploy_assets_is_identical_to_sequential_calls() { weights )); - for (idx, amount) in amounts.into_iter().enumerate() { + for (idx, amount) in + amounts.into_iter().enumerate().filter(|(_, amount)| *amount > min_liqudity) + { assert_ok!(Swaps::pool_join_with_exact_asset_amount( Origin::signed(ALICE), pool_id,