Description
[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.