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> {

primitives/src/math/checked_ops_res.rs

@@ -64,6 +64,13 @@ where
     fn checked_rem_res(&self, other: &Self) -> Result<Self, DispatchError>;
 }
 
+pub trait CheckedIncRes
+where
+    Self: Sized,
+{
+    fn checked_inc_res(&self) -> Result<Self, DispatchError>;
+}
+
 impl<T> CheckedAddRes for T
 where
     T: CheckedAdd,
@@ -123,3 +130,13 @@ where
         self.checked_rem(other).ok_or(DispatchError::Arithmetic(ArithmeticError::DivisionByZero))
     }
 }
+
+impl<T> CheckedIncRes for T
+where
+    T: CheckedAdd + From<u8>,
+{
+    #[inline]
+    fn checked_inc_res(&self) -> Result<Self, DispatchError> {
+        self.checked_add(&1u8.into()).ok_or(DispatchError::Arithmetic(ArithmeticError::Overflow))
+    }
+}

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

@@ -206,7 +206,6 @@ mod pallet {
         #[transactional]
         pub fn split_position(
             origin: OriginFor<T>,
-            // TODO Abstract this into a separate type.
             parent_collection_id: Option<CombinatorialIdOf<T>>,
             market_id: MarketIdOf<T>,
             partition: Vec<Vec<bool>>,
@@ -301,7 +300,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/combinatorial-tokens/src/traits/combinatorial_id_manager.rs

@@ -29,7 +29,6 @@ pub trait CombinatorialIdManager {
     type MarketId;
     type CombinatorialId;
 
-    // TODO Replace `Vec<bool>` with a more effective bit mask type.
     fn get_collection_id(
         parent_collection_id: Option<Self::CombinatorialId>,
         market_id: Self::MarketId,

zrml/futarchy/src/pallet_impls.rs

@@ -23,6 +23,10 @@ use frame_support::{
 };
 use zeitgeist_primitives::traits::FutarchyOracle;
 
+// Following Parity's implementation of pallet-democracy, we're using minimum priority for futarchy
+// proposals.
+const SCHEDULE_PRIORITY: u8 = 63;
+
 impl<T: Config> Pallet<T> {
     /// Evaluates `proposal` using the specified oracle and schedules the contained call if the
     /// oracle approves.
@@ -33,7 +37,7 @@ impl<T: Config> Pallet<T> {
             let result = T::Scheduler::schedule(
                 DispatchTime::At(proposal.when),
                 None,
-                63,
+                SCHEDULE_PRIORITY,
                 RawOrigin::Root.into(),
                 proposal.call.clone(),
             );

zrml/futarchy/src/types/proposal.rs

@@ -21,7 +21,6 @@ use frame_system::pallet_prelude::BlockNumberFor;
 use parity_scale_codec::{Decode, Encode, MaxEncodedLen};
 use scale_info::TypeInfo;
 
-// TODO Make config a generic, keeps things simple.
 #[derive(
     CloneNoBound, Decode, Encode, Eq, MaxEncodedLen, PartialEqNoBound, RuntimeDebugNoBound, TypeInfo,
 )]

zrml/neo-swaps/src/lib.rs

@@ -17,8 +17,8 @@
 
 #![doc = include_str!("../README.md")]
 #![cfg_attr(not(feature = "std"), no_std)]
-#![allow(clippy::too_many_arguments)] // TODO Try to remove this later!
-#![allow(clippy::type_complexity)] // TODO Try to remove this later!
+#![allow(clippy::too_many_arguments)]
+#![allow(clippy::type_complexity)]
 
 extern crate alloc;
 
@@ -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

@@ -17,7 +17,7 @@
 
 use crate::{traits::PoolStorage, Config, Error, Pallet, PoolCount, PoolOf, Pools};
 use sp_runtime::DispatchError;
-use zeitgeist_primitives::math::checked_ops_res::CheckedAddRes;
+use zeitgeist_primitives::math::checked_ops_res::CheckedIncRes;
 
 impl<T> PoolStorage for Pallet<T>
 where
@@ -35,7 +35,7 @@ where
         let pool_id = Self::next_pool_id();
         Pools::<T>::insert(pool_id, pool);
 
-        let pool_count = pool_id.checked_add_res(&1u8.into())?; // TODO Add CheckedInc.
+        let pool_count = pool_id.checked_inc_res()?;
         PoolCount::<T>::set(pool_count);
 
         Ok(pool_id)
@@ -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>;
 }