Audit fixes

Cargo.toml

@@ -291,6 +291,15 @@ rand_chacha = { version = "0.3.1", default-features = false }
 serde = { version = "1.0.198", default-features = false }
 typenum = { version = "1.17.0", default-features = false }
 
+[profile.test]
+overflow-checks = true
+
+[profile.test.package."*"]
+overflow-checks = true
+
+[profile.dev]
+overflow-checks = true
+
 [profile.dev.package]
 blake2 = { opt-level = 3 }
 blake2b_simd = { opt-level = 3 }
@@ -336,17 +345,28 @@ x25519-dalek = { opt-level = 3 }
 yamux = { opt-level = 3 }
 zeroize = { opt-level = 3 }
 
+[profile.dev.package."*"]
+overflow-checks = true
+
 [profile.production]
 codegen-units = 1
 incremental = false
 inherits = "release"
 lto = true
+overflow-checks = true
+
+[profile.production.package."*"]
+overflow-checks = true
 
 [profile.release]
 opt-level = 3
+overflow-checks = true
 # Zeitgeist runtime requires unwinding.
 panic = "unwind"
 
+[profile.release.package."*"]
+overflow-checks = true
+
 
 # xcm-emulator incompatible block number type fixed
 # Commits:

primitives/src/asset.rs

@@ -48,14 +48,14 @@ use serde::{Deserialize, Serialize};
 pub enum Asset<MarketId> {
     CategoricalOutcome(MarketId, CategoryIndex),
     ScalarOutcome(MarketId, ScalarPosition),
-    CombinatorialToken(CombinatorialId),
+    CombinatorialOutcomeLegacy, // Here to avoid having to migrate all holdings on the chain.
     PoolShare(PoolId),
     #[default]
     Ztg,
     ForeignAsset(u32),
     ParimutuelShare(MarketId, CategoryIndex),
+    CombinatorialToken(CombinatorialId),
 }
-// TODO Needs storage migration
 
 #[cfg(feature = "runtime-benchmarks")]
 impl<MarketId: MaxEncodedLen> ZeitgeistAssetEnumerator<MarketId> for Asset<MarketId> {

runtime/battery-station/src/parameters.rs

@@ -456,6 +456,7 @@ parameter_type_with_key! {
         match currency_id {
             Asset::CategoricalOutcome(_,_) => ExistentialDeposit::get(),
             Asset::CombinatorialToken(_) => ExistentialDeposit::get(),
+            Asset::CombinatorialOutcomeLegacy => ExistentialDeposit::get(),
             Asset::PoolShare(_)  => ExistentialDeposit::get(),
             Asset::ScalarOutcome(_,_)  => ExistentialDeposit::get(),
             #[cfg(feature = "parachain")]

runtime/zeitgeist/src/parameters.rs

@@ -456,6 +456,7 @@ parameter_type_with_key! {
         match currency_id {
             Asset::CategoricalOutcome(_,_) => ExistentialDeposit::get(),
             Asset::CombinatorialToken(_) => ExistentialDeposit::get(),
+            Asset::CombinatorialOutcomeLegacy => ExistentialDeposit::get(),
             Asset::PoolShare(_)  => ExistentialDeposit::get(),
             Asset::ScalarOutcome(_,_)  => ExistentialDeposit::get(),
             #[cfg(feature = "parachain")]

zrml/combinatorial-tokens/src/lib.rs

@@ -301,7 +301,7 @@ mod pallet {
                 TransmutationType::VerticalWithParent => {
                     // Split combinatorial token into higher level position.
                     // This will fail if the market has a different collateral than the previous
-                    // markets. FIXME A cleaner error message would be nice though...
+                    // markets.
                     T::MultiCurrency::ensure_can_withdraw(position, &who, amount)?;
                     T::MultiCurrency::withdraw(position, &who, amount)?;
 

zrml/combinatorial-tokens/src/tests/redeem_position.rs

@@ -46,7 +46,7 @@ fn redeem_position_fails_on_market_not_found() {
 
 #[test_case(vec![B0, B1, B0, B1]; "incorrect_len")]
 #[test_case(vec![B0, B0, B0]; "all_zero")]
-#[test_case(vec![B0, B0, B0]; "all_one")]
+#[test_case(vec![B1, B1, B1]; "all_one")]
 fn redeem_position_fails_on_incorrect_index_set(index_set: Vec<bool>) {
     ExtBuilder::build().execute_with(|| {
         let alice = Account::new(0).deposit(Asset::Ztg, _100).unwrap();

zrml/neo-swaps/src/lib.rs

@@ -1008,10 +1008,7 @@ mod pallet {
         ) -> DispatchResult {
             ensure!(pool_shares_amount != Zero::zero(), Error::<T>::ZeroAmount);
 
-            // FIXME Should this also be made part of the `PoolStorage` interface?
-            Pools::<T>::try_mutate_exists(pool_id, |maybe_pool| {
-                let pool =
-                    maybe_pool.as_mut().ok_or::<DispatchError>(Error::<T>::PoolNotFound.into())?;
+            <Self as PoolStorage>::try_mutate_exists(&pool_id, |pool| {
                 let ratio = {
                     let mut ratio = pool_shares_amount
                         .bdiv_floor(pool.liquidity_shares_manager.total_shares()?)?;
@@ -1047,13 +1044,14 @@ mod pallet {
                     for asset in pool.assets().iter() {
                         withdraw_remaining(asset)?;
                     }
-                    *maybe_pool = None; // Delete the storage map entry.
                     Self::deposit_event(Event::<T>::PoolDestroyed {
                         who: who.clone(),
                         pool_id,
                         amounts_out,
                     });
-                    // No need to clear `MarketIdToPoolId`.
+
+                    // Delete the pool. No need to clear `MarketIdToPoolId`.
+                    Ok(((), true))
                 } else {
                     let old_liquidity_parameter = pool.liquidity_parameter;
                     let new_liquidity_parameter = old_liquidity_parameter
@@ -1083,8 +1081,9 @@ mod pallet {
                         amounts_out,
                         new_liquidity_parameter,
                     });
+
+                    Ok(((), false))
                 }
-                Ok(())
             })
         }
 

zrml/neo-swaps/src/math/traits/combo_math_ops.rs

@@ -23,13 +23,17 @@ pub(crate) trait ComboMathOps<T>
 where
     T: Config,
 {
+    /// Calculates the amount swapped out of a pool with liquidity parameter `liquidity` when
+    /// swapping in `amount_in` units of assets whose reserves in the pool are `sell` and swapping
+    /// out assets whose reserves in the pool are `buy`.
     fn calculate_swap_amount_out_for_buy(
         buy: Vec<BalanceOf<T>>,
         sell: Vec<BalanceOf<T>>,
         amount_in: BalanceOf<T>,
         liquidity: BalanceOf<T>,
     ) -> Result<BalanceOf<T>, DispatchError>;
 
+    /// Calculates the amount eventually held by the user after equalizing holdings.
     #[allow(dead_code)]
     fn calculate_equalize_amount(
         buy: Vec<BalanceOf<T>>,
@@ -39,6 +43,10 @@ where
         liquidity: BalanceOf<T>,
     ) -> Result<BalanceOf<T>, DispatchError>;
 
+    /// Calculates the amount of each asset of a pool with liquidity parameter `liquidity` held
+    /// in the user's wallet after equalizing positions whose reserves in the pool are `buy`, `keep`
+    /// and `sell`, resp. The parameters `amount_buy` and `amount_keep` refer to the user's holdings
+    /// of `buy` and `keep`.
     fn calculate_swap_amount_out_for_sell(
         buy: Vec<BalanceOf<T>>,
         keep: Vec<BalanceOf<T>>,

zrml/neo-swaps/src/pool_storage.rs

@@ -47,10 +47,26 @@ where
 
     fn try_mutate_pool<R, F>(pool_id: &Self::PoolId, mutator: F) -> Result<R, DispatchError>
     where
-        F: FnMut(&mut PoolOf<T>) -> Result<R, DispatchError>,
+        F: FnMut(&mut Self::Pool) -> Result<R, DispatchError>,
     {
         Pools::<T>::try_mutate(pool_id, |maybe_pool| {
             maybe_pool.as_mut().ok_or(Error::<T>::PoolNotFound.into()).and_then(mutator)
         })
     }
+
+    fn try_mutate_exists<R, F>(pool_id: &Self::PoolId, mutator: F) -> Result<R, DispatchError>
+    where
+        F: FnMut(&mut Self::Pool) -> Result<(R, bool), DispatchError>,
+    {
+        Pools::<T>::try_mutate_exists(pool_id, |maybe_pool| {
+            let (result, delete) =
+                maybe_pool.as_mut().ok_or(Error::<T>::PoolNotFound.into()).and_then(mutator)?;
+
+            if delete {
+                *maybe_pool = None;
+            }
+
+            Ok(result)
+        })
+    }
 }

zrml/neo-swaps/src/tests/combo_buy.rs

@@ -362,27 +362,45 @@ fn combo_buy_fails_on_amount_out_below_min() {
             ALICE,
             BASE_ASSET,
             vec![MarketType::Categorical(2), MarketType::Scalar(0..=1)],
-            _10,
+            100_000_000 * _100, // Massive liquidity to keep slippage low.
             vec![_1_4, _1_4, _1_4, _1_4],
             CENT,
         );
         let pool = <Pallet<Runtime> as PoolStorage>::get(pool_id).unwrap();
         let assets = pool.assets();
-        let amount_in = _1;
+
+        let asset_count = 4;
+        let buy = assets[0..1].to_vec();
+        let sell = assets[1..4].to_vec();
+        // amount_in is _1 / 0.97 (i.e. _1 after deducting 3% fees - 1% trading fees, 1% external
+        // fees _for each market_)
+        let amount_in = 10_309_278_350;
+
         assert_ok!(AssetManager::deposit(BASE_ASSET, &BOB, amount_in));
-        // Buying 1 at price of .25 will return less than 4 outcomes due to slippage.
+        // Buying for 1 at a price of .25 will return less than 4 outcomes due to slippage.
         assert_noop!(
             NeoSwaps::combo_buy(
                 RuntimeOrigin::signed(BOB),
                 pool_id,
-                4,
-                vec![assets[0]],
-                vec![assets[1]],
+                asset_count,
+                buy.clone(),
+                sell.clone(),
                 amount_in,
                 _4,
             ),
             Error::<Runtime>::AmountOutBelowMin,
         );
+
+        // Post OakSecurity audit: Show that the slippage limit is tight.
+        assert_ok!(NeoSwaps::combo_buy(
+            RuntimeOrigin::signed(BOB),
+            pool_id,
+            asset_count,
+            buy,
+            sell,
+            amount_in,
+            _4 - 33,
+        ));
     });
 }
 

zrml/neo-swaps/src/traits/pool_storage.rs

@@ -17,6 +17,8 @@
 
 use sp_runtime::DispatchError;
 
+/// Slot map interface for pool storage. Undocumented functions behave like their counterparts in
+/// substrate's `StorageMap`.
 pub(crate) trait PoolStorage {
     type PoolId;
     type Pool;
@@ -30,4 +32,11 @@ pub(crate) trait PoolStorage {
     fn try_mutate_pool<R, F>(pool_id: &Self::PoolId, mutator: F) -> Result<R, DispatchError>
     where
         F: FnMut(&mut Self::Pool) -> Result<R, DispatchError>;
+
+    /// Mutate and maybe remove the pool indexed by `pool_id`. Unlike `try_mutate_exists` in
+    /// `StorageMap`, the `mutator` must return a `(R, bool)`. If and only if the pool is positive,
+    /// the pool is removed.
+    fn try_mutate_exists<R, F>(pool_id: &Self::PoolId, mutator: F) -> Result<R, DispatchError>
+    where
+        F: FnMut(&mut Self::Pool) -> Result<(R, bool), DispatchError>;
 }

zrml/neo-swaps/src/utility.rs

@@ -15,20 +15,59 @@
 // You should have received a copy of the GNU General Public License
 // along with Zeitgeist. If not, see <https://www.gnu.org/licenses/>.
 
-use sp_runtime::SaturatedConversion;
+use sp_runtime::{
+    traits::{One, Zero},
+    SaturatedConversion,
+};
 
 pub(crate) trait LogCeil {
+    /// Calculates the ceil of the log with base 2 of `self`.
     fn log_ceil(&self) -> Self;
 }
 
 impl LogCeil for u16 {
     fn log_ceil(&self) -> Self {
         let x = *self;
 
-        let bits_minus_one = u16::MAX.saturating_sub(1);
+        if x.is_zero() {
+            return One::one();
+        }
+
+        let bits_minus_one: u16 = u16::BITS.saturating_sub(1).saturated_into();
         let leading_zeros: u16 = x.leading_zeros().saturated_into();
         let floor_log2 = bits_minus_one.saturating_sub(leading_zeros);
 
         if x.is_power_of_two() { floor_log2 } else { floor_log2.saturating_add(1) }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use test_case::test_case;
+
+    #[test_case(0, 1)]
+    #[test_case(1, 0)]
+    #[test_case(2, 1)]
+    #[test_case(3, 2)]
+    #[test_case(4, 2)]
+    #[test_case(5, 3)]
+    #[test_case(6, 3)]
+    #[test_case(7, 3)]
+    #[test_case(8, 3)]
+    #[test_case(9, 4)]
+    #[test_case(15, 4)]
+    #[test_case(16, 4)]
+    #[test_case(17, 5)]
+    #[test_case(1023, 10)]
+    #[test_case(1024, 10)]
+    #[test_case(1025, 11)]
+    #[test_case(32767, 15)]
+    #[test_case(32768, 15)]
+    #[test_case(32769, 16)]
+    #[test_case(65535, 16)]
+    fn log_ceil_works(value: u16, expected: u16) {
+        let actual = value.log_ceil();
+        assert_eq!(actual, expected);
+    }
+}