From 99327559e1f10364b089c7f27c6bff4f2ae4e2d5 Mon Sep 17 00:00:00 2001
From: Valery Gantchev <v@lery.dev>
Date: Wed, 29 Jan 2025 10:11:04 +0100
Subject: [PATCH] Use checked math in frame-balances named_reserve (#7365)
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 sum of the
values 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.
---------
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
(cherry picked from commit f373af0d1c1e296c1b07486dd74710b40089250e)
---
prdoc/pr_7365.prdoc | 12 ++++++++++++
substrate/frame/balances/src/impl_currency.rs | 6 ++++--
2 files changed, 16 insertions(+), 2 deletions(-)
create mode 100644 prdoc/pr_7365.prdoc
diff --git a/prdoc/pr_7365.prdoc b/prdoc/pr_7365.prdoc
new file mode 100644
index 00000000000..dcee76e01c7
--- /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 23feb46b72c..89a4f3d2aa6 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
--
GitLab