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 454aead1773f29e2ee2db0f1fad4711aa8b19b95..ee41035b0fd3ab01e7f6b613ce73533261981540 100644 --- a/substrate/frame/balances/src/impl_currency.rs +++ b/substrate/frame/balances/src/impl_currency.rs @@ -651,8 +651,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