Skip to content

Commit f373af0

Browse files
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>
1 parent 0dab441 commit f373af0

2 files changed

Lines changed: 16 additions & 2 deletions

File tree

prdoc/pr_7365.prdoc

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
title: Use checked math in frame-balances named_reserve
2+
doc:
3+
- audience: Runtime Dev
4+
description: |-
5+
This PR modifies `named_reserve()` in frame-balances to use checked math instead of defensive saturating math.
6+
7+
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)
8+
9+
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.
10+
crates:
11+
- name: pallet-balances
12+
bump: patch

substrate/frame/balances/src/impl_currency.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -674,8 +674,10 @@ where
674674
Reserves::<T, I>::try_mutate(who, |reserves| -> DispatchResult {
675675
match reserves.binary_search_by_key(id, |data| data.id) {
676676
Ok(index) => {
677-
// this add can't overflow but just to be defensive.
678-
reserves[index].amount = reserves[index].amount.defensive_saturating_add(value);
677+
reserves[index].amount = reserves[index]
678+
.amount
679+
.checked_add(&value)
680+
.ok_or(ArithmeticError::Overflow)?;
679681
},
680682
Err(index) => {
681683
reserves

0 commit comments

Comments
 (0)