diff --git a/prdoc/pr_7365.prdoc b/prdoc/pr_7365.prdoc new file mode 100644 index 0000000000000000000000000000000000000000..dcee76e01c789ce2c67f714701d0725e1a59db18 --- /dev/null +++ b/prdoc/pr_7365.prdoc @@ -0,0 +1,12 @@ +title: Use checked math in frame-balances named_reserve +doc: +- audience: Runtime Dev + description: |- + This PR modifies `named_reserve()` in frame-balances to use checked math instead of defensive saturating math. + + The use of saturating math relies on the assumption that the value will always fit in `u128::MAX`. However, there is nothing preventing the implementing pallet from passing a larger value which overflows. This can happen if the implementing pallet does not validate user input and instead relies on `named_reserve()` to return an error (this saves an additional read) + + This is not a security concern, as the method will subsequently return an error thanks to `<Self as ReservableCurrency<_>>::reserve(who, value)?;`. However, the `defensive_saturating_add` will panic in `--all-features`, creating false positive crashes in fuzzing operations. +crates: +- name: pallet-balances + bump: patch diff --git a/substrate/frame/balances/src/impl_currency.rs b/substrate/frame/balances/src/impl_currency.rs index bc7e77c191db86dd2805f200c3ceb17a293fce73..f453b23420c400dddd01fe1013f358c3199c1ff2 100644 --- a/substrate/frame/balances/src/impl_currency.rs +++ b/substrate/frame/balances/src/impl_currency.rs @@ -674,8 +674,10 @@ where Reserves::<T, I>::try_mutate(who, |reserves| -> DispatchResult { match reserves.binary_search_by_key(id, |data| data.id) { Ok(index) => { - // this add can't overflow but just to be defensive. - reserves[index].amount = reserves[index].amount.defensive_saturating_add(value); + reserves[index].amount = reserves[index] + .amount + .checked_add(&value) + .ok_or(ArithmeticError::Overflow)?; }, Err(index) => { reserves