[Low] Usage of unsafe arithmetics in multiple pallets

Summary

Unsafe arithmetic is being used in multiple pallets, which can lead to integer overflows or underflows and cause unexpected results.

Issue details

We found multiple usages of unsafe arithmetics that we could not confirm to be safe. Details for an example follow:

The function do_start_take_sell_order uses unsafe arithmetics in multiple places, e.g.:

pub fn do_start_take_sell_order(
    authority: OriginFor<T>,
    order_id: [u8; 32],
    tax_credit_amount: T::Balance,
) -> DispatchResult
where
    <T as pallet_uniques::Config>::ItemId: From<u32>,
{
// ...
    ensure!(
        Self::do_get_afloat_balance(who.clone()) >=
// --> Unsafe multiplication
            offer.price_per_credit * tax_credit_amount.into(),
        Error::<T>::NotEnoughAfloatBalanceAvailable
    );

// ...

// --> Unsafe multiplication
    let total_price: T::Balance = price_per_credit * tax_credit_amount;

Since price_per_credit is initially set to the price argument from the create_offer extrinsic parameter, an overflow could likely be triggered.

Additional findings are:

afloat (do_start_take_sell_order): Multiple unsafe multiplications (see also lines below the linked finding)
fund-admin (do_calculate_drawdown_total_amount, do_calculate_revenue_total_amount): Usage of unchecked add

Risk

By triggering an integer overflow, a malicious actor can:

Crash the nodes compiled in debug mode with overflow checks enabled
On nodes which have overflow checks disabled, unexpected behaviors and logic inconsistencies

Mitigation

Implement proper integer overflow handling by checking call arguments and using safe arithmetic functions (e.g.: saturating_mul, checked_mul).

Hashed is using unsafe arithmetic operations in multiple locations in the source code. We strongly suggest to review the usage of these functions and to migrate to safe arithmetic functions instead.

hashed-io / hashed-pallets