[balances] Safeguard against consumer ref underflow (#3865)

There are some accounts that do not have a consumer ref while having a
reserve.
This adds a fail-safe mechanism to trigger in the case that
`does_consume` is true, but the assumption of `consumer>0` is not.

This should prevent those accounts from loosing balance and the TI from
getting messed up even more, but is not an "ideal" fix. TBH an ideal fix
is not possible, since on-chain data is in an invalid state.

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

prdoc/pr_3865.prdoc

@@ -0,0 +1,11 @@
+title: "Balances: add failsafe for consumer ref underflow"
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      Pallet balances now handles the case that historic accounts violate a invariant that they should have a consumer ref on `reserved > 0` balance.  
+      This disallows such accounts from reaping and should prevent TI from getting messed up even more.
+
+crates:
+  - name: pallet-balances
+    bump: patch

substrate/frame/balances/Cargo.toml

@@ -28,6 +28,7 @@ docify = "0.2.8"
 
 [dev-dependencies]
 pallet-transaction-payment = { path = "../transaction-payment" }
+frame-support = { path = "../support", features = ["experimental"] }
 sp-core = { path = "../../primitives/core" }
 sp-io = { path = "../../primitives/io" }
 paste = "1.0.12"

substrate/frame/balances/src/lib.rs

@@ -954,6 +954,13 @@ pub mod pallet {
 				if !did_consume && does_consume {
 					frame_system::Pallet::<T>::inc_consumers(who)?;
 				}
+				if does_consume && frame_system::Pallet::<T>::consumers(who) == 0 {
+					// NOTE: This is a failsafe and should not happen for normal accounts. A normal
+					// account should have gotten a consumer ref in `!did_consume && does_consume`
+					// at some point.
+					log::error!(target: LOG_TARGET, "Defensively bumping a consumer ref.");
+					frame_system::Pallet::<T>::inc_consumers(who)?;
+				}
 				if did_provide && !does_provide {
 					// This could reap the account so must go last.
 					frame_system::Pallet::<T>::dec_providers(who).map_err(|r| {

substrate/frame/balances/src/tests/general_tests.rs

@@ -0,0 +1,111 @@
+// This file is part of Substrate.
+
+// Copyright (C) Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#![cfg(test)]
+
+use crate::{
+	system::AccountInfo,
+	tests::{ensure_ti_valid, Balances, ExtBuilder, System, Test, TestId, UseSystem},
+	AccountData, ExtraFlags, TotalIssuance,
+};
+use frame_support::{
+	assert_noop, assert_ok, hypothetically,
+	traits::{
+		fungible::{Mutate, MutateHold},
+		tokens::Precision,
+	},
+};
+use sp_runtime::DispatchError;
+
+/// There are some accounts that have one consumer ref too few. These accounts are at risk of losing
+/// their held (reserved) balance. They do not just lose it - it is also not accounted for in the
+/// Total Issuance. Here we test the case that the account does not reap in such a case, but gets
+/// one consumer ref for its reserved balance.
+#[test]
+fn regression_historic_acc_does_not_evaporate_reserve() {
+	ExtBuilder::default().build_and_execute_with(|| {
+		UseSystem::set(true);
+		let (alice, bob) = (0, 1);
+		// Alice is in a bad state with consumer == 0 && reserved > 0:
+		Balances::set_balance(&alice, 100);
+		TotalIssuance::<Test>::put(100);
+		ensure_ti_valid();
+
+		assert_ok!(Balances::hold(&TestId::Foo, &alice, 10));
+		// This is the issue of the account:
+		System::dec_consumers(&alice);
+
+		assert_eq!(
+			System::account(&alice),
+			AccountInfo {
+				data: AccountData {
+					free: 90,
+					reserved: 10,
+					frozen: 0,
+					flags: ExtraFlags(1u128 << 127),
+				},
+				nonce: 0,
+				consumers: 0, // should be 1 on a good acc
+				providers: 1,
+				sufficients: 0,
+			}
+		);
+
+		ensure_ti_valid();
+
+		// Reaping the account is prevented by the new logic:
+		assert_noop!(
+			Balances::transfer_allow_death(Some(alice).into(), bob, 90),
+			DispatchError::ConsumerRemaining
+		);
+		assert_noop!(
+			Balances::transfer_all(Some(alice).into(), bob, false),
+			DispatchError::ConsumerRemaining
+		);
+
+		// normal transfers still work:
+		hypothetically!({
+			assert_ok!(Balances::transfer_keep_alive(Some(alice).into(), bob, 40));
+			// Alice got back her consumer ref:
+			assert_eq!(System::consumers(&alice), 1);
+			ensure_ti_valid();
+		});
+		hypothetically!({
+			assert_ok!(Balances::transfer_all(Some(alice).into(), bob, true));
+			// Alice got back her consumer ref:
+			assert_eq!(System::consumers(&alice), 1);
+			ensure_ti_valid();
+		});
+
+		// un-reserving all does not add a consumer ref:
+		hypothetically!({
+			assert_ok!(Balances::release(&TestId::Foo, &alice, 10, Precision::Exact));
+			assert_eq!(System::consumers(&alice), 0);
+			assert_ok!(Balances::transfer_keep_alive(Some(alice).into(), bob, 40));
+			assert_eq!(System::consumers(&alice), 0);
+			ensure_ti_valid();
+		});
+		// un-reserving some does add a consumer ref:
+		hypothetically!({
+			assert_ok!(Balances::release(&TestId::Foo, &alice, 5, Precision::Exact));
+			assert_eq!(System::consumers(&alice), 1);
+			assert_ok!(Balances::transfer_keep_alive(Some(alice).into(), bob, 40));
+			assert_eq!(System::consumers(&alice), 1);
+			ensure_ti_valid();
+		});
+	});
+}

substrate/frame/balances/src/tests/mod.rs

@@ -19,7 +19,7 @@
 
 #![cfg(test)]
 
-use crate::{self as pallet_balances, AccountData, Config, CreditOf, Error, Pallet};
+use crate::{self as pallet_balances, AccountData, Config, CreditOf, Error, Pallet, TotalIssuance};
 use codec::{Decode, Encode, MaxEncodedLen};
 use frame_support::{
 	assert_err, assert_noop, assert_ok, assert_storage_noop, derive_impl,
@@ -47,6 +47,7 @@ mod currency_tests;
 mod dispatchable_tests;
 mod fungible_conformance_tests;
 mod fungible_tests;
+mod general_tests;
 mod reentrancy_tests;
 
 type Block = frame_system::mocking::MockBlock<Test>;
@@ -278,6 +279,23 @@ pub fn info_from_weight(w: Weight) -> DispatchInfo {
 	DispatchInfo { weight: w, ..Default::default() }
 }
 
+/// Check that the total-issuance matches the sum of all accounts' total balances.
+pub fn ensure_ti_valid() {
+	let mut sum = 0;
+
+	for acc in frame_system::Account::<Test>::iter_keys() {
+		if UseSystem::get() {
+			let data = frame_system::Pallet::<Test>::account(acc);
+			sum += data.data.total();
+		} else {
+			let data = crate::Account::<Test>::get(acc);
+			sum += data.total();
+		}
+	}
+
+	assert_eq!(TotalIssuance::<Test>::get(), sum, "Total Issuance wrong");
+}
+
 #[test]
 fn weights_sane() {
 	let info = crate::Call::<Test>::transfer_allow_death { dest: 10, value: 4 }.get_dispatch_info();

substrate/frame/balances/src/types.rs

@@ -111,7 +111,7 @@ pub struct AccountData<Balance> {
 const IS_NEW_LOGIC: u128 = 0x80000000_00000000_00000000_00000000u128;
 
 #[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug, MaxEncodedLen, TypeInfo)]
-pub struct ExtraFlags(u128);
+pub struct ExtraFlags(pub(crate) u128);
 impl Default for ExtraFlags {
 	fn default() -> Self {
 		Self(IS_NEW_LOGIC)