galacticcouncil
Bug: [Audit_RV] Missing overflow checking for account balance (ORML tokens)
It is suspected that overflow checking when minting LRNA token is not sufficient, in the do_deposit function of open-runtime-module-library-d3634ee624a28945/63b3219/tokens/src/lib.rs.
Description
let new_total_issuance = Self::total_issuance(currency_id)
.checked_add(&amount)
.ok_or(ArithmeticError::Overflow)?;
if change_total_issuance {
TotalIssuance::<T>::mutate(currency_id, |v| *v = new_total_issuance);
}
account.free += amount;
The above code is where omnipool account balance of HubAssetID (a.k.a., LRNA) is updated. However, unless
account.free <= new_total_issuance is guaranteed, there is a possibility of overflow since + for Balance won't handle overflow checking.
Since currency crate by ORML is out of the scope of this audit, we recommend Omnipool developer to use this crate with caution.
This issue might be updated later after we've go through all the code of Omnipool., if we find or not find a potential overflow.
Expected Behavior
Always use safe math operations and check for arithmetic errors.
Context
Part of security audit by Runtime Verification Inc..
I believe this is actually fine.
because the total issuance is updated just before that and it uses checked math.
and the account.free cannot be > the issuance.
Even if all is minted for this account - account.free would be equal to tottal issuance.
and it checks that it is possible to safely add amount to total issuance, and if that does not overflow -> free += amount wont overflow either.
Well, as pointed out in the issue description, account.free += amount is guaranteed not overflow, unless account.free <= new_total_issuance is guaranteed.
We might assume/desire account.free <= new_total_issuance is always true, but we will need evidence from the code (e.g., a proof) to validate this assumption. It could be possible that account.free or total_issurance(HubAssetId) is changed in other functions that would break this assumption.
looks like code was changed since then, but it can still overflow https://github.com/galacticcouncil/open-runtime-module-library/blob/master/tokens/src/lib.rs#L545
I am surprised to see updating an important variable, balance without safe math operation. Add an additional overflow checking on an account's free balance should do no harm but make the code robust to invariant breaking changes.
I would suggest raise an issue at orml repo. I would also recommend to add a post condition checking in the initialise_pool(), for example, after https://github.com/galacticcouncil/HydraDX-node/blob/20c8ac6079b8b3d6f16a9f15e9e43353ea6b9abc/pallets/omnipool/src/lib.rs#L428 add something similar to
let hub_asset_liguidity = T::Currency::free_balance(T::HubAssetId::get(), &Self::protocol_account());
let increased_liquidity = stable_asset_hub_reserve.checked_add(native_asset_hub_reserve).ok_or(ArithmeticError::Overflow)?;
ensure!( hub_asset_liguidity >= increased_liquidity, Error::<T>:: HubAssetAccountOverflow);
Assume there is an error type HubAssetAccountOverflow.
It will protect Omnipool against mistakes/errors from depended libraries towards intentional or unintentional errors.
Since the hub asset is very important in Omnipool, any mistakes on this asset balance or total issuance would affect all the tradable assets in the pool. It is highly recommended to be extra careful (with sufficient checking) on this asset's operations.
looks like code was changed since then, but it can still overflow https://github.com/galacticcouncil/open-runtime-module-library/blob/master/tokens/src/lib.rs#L545
fyi: this is actually old one. our forked repo has not been updated.
in the latest version - it has not changed. It does not use safe math.
Issue raised at orml-repo
https://github.com/open-web3-stack/open-runtime-module-library/issues/824
