Staking module: approximate fraction into perbill to avoid overflow (#2889)

* approximate fraction into perbill

* test

* fix comment

* line width

* bump impl version

* rename test for better naming

* test overflow

* Apply suggestions from code review

Co-Authored-By: Niklas Adolfsson <niklasadolfsson1@gmail.com>

substrate/srml/staking/src/lib.rs

@@ -940,11 +940,10 @@ impl<T: Trait> Module<T> {
 			// The total to be slashed from the nominators.
 			let total = exposure.total - exposure.own;
 			if !total.is_zero() {
-				// FIXME #1572 avoid overflow
-				let safe_mul_rational = |b| b * rest_slash / total;
 				for i in exposure.others.iter() {
+					let per_u64 = Perbill::from_rational_approximation(i.value, total);
 					// best effort - not much that can be done on fail.
-					imbalance.subsume(T::Currency::slash(&i.who, safe_mul_rational(i.value)).0)
+					imbalance.subsume(T::Currency::slash(&i.who, per_u64 * rest_slash).0)
 				}
 			}
 		}
@@ -986,13 +985,14 @@ impl<T: Trait> Module<T> {
 		} else {
 			let exposure = Self::stakers(stash);
 			let total = exposure.total.max(One::one());
-			// FIXME #1572:  avoid overflow
-			let safe_mul_rational = |b| b * reward / total;
+
 			for i in &exposure.others {
-				let nom_payout = safe_mul_rational(i.value);
-				imbalance.maybe_subsume(Self::make_payout(&i.who, nom_payout));
+				let per_u64 = Perbill::from_rational_approximation(i.value, total);
+				imbalance.maybe_subsume(Self::make_payout(&i.who, per_u64 * reward));
 			}
-			safe_mul_rational(exposure.own)
+
+			let per_u64 = Perbill::from_rational_approximation(exposure.own, total);
+			per_u64 * reward
 		};
 		imbalance.maybe_subsume(Self::make_payout(stash, validator_cut + off_the_table));
 		T::Reward::on_unbalanced(imbalance);

substrate/srml/staking/src/tests.rs

@@ -665,13 +665,21 @@ fn nominating_and_rewards_should_work() {
 		// nothing else will happen, era ends and rewards are paid again,
 		// it is expected that nominators will also be paid. See below
 
+		// Approximation resulting from Perbill conversion
+		let approximation = 1;
 		// Nominator 2: has [400/1800 ~ 2/9 from 10] + [600/2200 ~ 3/11 from 20]'s reward. ==> 2/9 + 3/11
-		assert_eq!(Balances::total_balance(&2), initial_balance + (2*new_session_reward/9 + 3*new_session_reward/11) - 1);
+		assert_eq!(
+			Balances::total_balance(&2),
+			initial_balance + (2*new_session_reward/9 + 3*new_session_reward/11) - 1 - approximation
+		);
 		// Nominator 4: has [400/1800 ~ 2/9 from 10] + [600/2200 ~ 3/11 from 20]'s reward. ==> 2/9 + 3/11
-		assert_eq!(Balances::total_balance(&4), initial_balance + (2*new_session_reward/9 + 3*new_session_reward/11) - 1);
+		assert_eq!(
+			Balances::total_balance(&4),
+			initial_balance + (2*new_session_reward/9 + 3*new_session_reward/11) - 1 - approximation
+		);
 
 		// 10 got 800 / 1800 external stake => 8/18 =? 4/9 => Validator's share = 5/9
-		assert_eq!(Balances::total_balance(&10), initial_balance + 5*new_session_reward/9);
+		assert_eq!(Balances::total_balance(&10), initial_balance + 5*new_session_reward/9 - approximation);
 		// 10 got 1200 / 2200 external stake => 12/22 =? 6/11 => Validator's share = 5/11
 		assert_eq!(Balances::total_balance(&20), initial_balance + 5*new_session_reward/11+ 2);
 
@@ -1655,11 +1663,13 @@ fn bond_with_no_staked_value() {
 
 		start_era(3);
 
+		// Approximation resulting from Perbill conversion
+		let approximation = 1;
 		let reward = Staking::current_session_reward() * 3;
 		// 2 will not get a reward of only 1
 		// 4 will get the rest
-		assert_eq!(Balances::free_balance(&2), initial_balance_2 + 3);
-		assert_eq!(Balances::free_balance(&4), initial_balance_4 + reward - 3);
+		assert_eq!(Balances::free_balance(&2), initial_balance_2 + 3 - approximation);
+		assert_eq!(Balances::free_balance(&4), initial_balance_4 + reward - 3 - approximation);
 	});
 }
 
@@ -1975,3 +1985,36 @@ fn phragmen_large_scale_test_2() {
 		assert_total_expo(5, nom_budget / 2 + c_budget);
 	})
 }
+
+#[test]
+fn reward_validator_slashing_validator_doesnt_overflow() {
+	with_externalities(&mut ExtBuilder::default()
+		.build(),
+	|| {
+		let stake = u32::max_value() as u64 * 2;
+		let reward_slash = u32::max_value() as u64 * 2;
+
+		// Assert multiplication overflows in balance arithmetic.
+		assert!(stake.checked_mul(reward_slash).is_none());
+
+		// Set staker
+		let _ = Balances::make_free_balance_be(&11, stake);
+		<Stakers<Test>>::insert(&11, Exposure { total: stake, own: stake, others: vec![] });
+
+		// Check reward
+		Staking::reward_validator(&11, reward_slash);
+		assert_eq!(Balances::total_balance(&11), stake * 2);
+
+		// Set staker
+		let _ = Balances::make_free_balance_be(&11, stake);
+		let _ = Balances::make_free_balance_be(&2, stake);
+		<Stakers<Test>>::insert(&11, Exposure { total: stake, own: 1, others: vec![
+			IndividualExposure { who: 2, value: stake - 1 }
+		]});
+
+		// Check slashing
+		Staking::slash_validator(&11, reward_slash);
+		assert_eq!(Balances::total_balance(&11), stake - 1);
+		assert_eq!(Balances::total_balance(&2), 1);
+	})
+}