bond_extra should be authorised only from stash (paritytech#2096)

* bond_extra should be authorised only from stash, lest the controller
gets compromised.

* Fix tests

* Fix grumbles

* Pass compact balances

node/runtime/src/lib.rs

@@ -58,7 +58,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	spec_name: create_runtime_str!("node"),
 	impl_name: create_runtime_str!("substrate-node"),
 	authoring_version: 10,
-	spec_version: 41,
+	spec_version: 42,
 	impl_version: 42,
 	apis: RUNTIME_API_VERSIONS,
 };

srml/staking/src/lib.rs

@@ -228,7 +228,7 @@ use srml_support::traits::{
 };
 use session::OnSessionChange;
 use primitives::Perbill;
-use primitives::traits::{Zero, One, As, StaticLookup, Saturating, Bounded};
+use primitives::traits::{Zero, One, As, StaticLookup, CheckedSub, Saturating, Bounded};
 #[cfg(feature = "std")]
 use primitives::{Serialize, Deserialize};
 use system::ensure_signed;
@@ -549,14 +549,17 @@ decl_module! {
 		///
 		/// Use this if there are additional funds in your stash account that you wish to bond.
 		///
-		/// The dispatch origin for this call must be _Signed_ by the controller, not the stash.
-		fn bond_extra(origin, max_additional: BalanceOf<T>) {
-			let controller = ensure_signed(origin)?;
+		/// The dispatch origin for this call must be _Signed_ by the stash, not the controller.
+		fn bond_extra(origin, #[compact] max_additional: BalanceOf<T>) {
+			let stash = ensure_signed(origin)?;
+
+			let controller = Self::bonded(&stash).ok_or("not a stash")?;
 			let mut ledger = Self::ledger(&controller).ok_or("not a controller")?;
-			let stash_balance = T::Currency::free_balance(&ledger.stash);
 
-			if stash_balance > ledger.total {
-				let extra = (stash_balance - ledger.total).min(max_additional);
+			let stash_balance = T::Currency::free_balance(&stash);
+
+			if let Some(extra) = stash_balance.checked_sub(&ledger.total) {
+				let extra = extra.min(max_additional);
 				ledger.total += extra;
 				ledger.active += extra;
 				Self::update_ledger(&controller, &ledger);
@@ -583,8 +586,7 @@ decl_module! {
 				ledger.active -= value;
 
 				// Avoid there being a dust balance left in the staking system.
-				let ed = T::Currency::minimum_balance();
-				if ledger.active < ed {
+				if ledger.active < T::Currency::minimum_balance() {
 					value += ledger.active;
 					ledger.active = Zero::zero();
 				}
@@ -672,7 +674,7 @@ decl_module! {
 		///
 		/// Effects will be felt at the beginning of the next era.
 		///
-		/// The dispatch origin for this call must be _Signed_ by the controller, not the stash.
+		/// The dispatch origin for this call must be _Signed_ by the stash, not the controller.
 		fn set_controller(origin, controller: <T::Lookup as StaticLookup>::Source) {
 			let stash = ensure_signed(origin)?;
 			let old_controller = Self::bonded(&stash).ok_or("not a stash")?;

srml/staking/src/tests.rs

@@ -1104,12 +1104,12 @@ fn bond_extra_works() {
 		let _ = Balances::make_free_balance_be(&11, 1000000);
 
 		// Call the bond_extra function from controller, add only 100
-		assert_ok!(Staking::bond_extra(Origin::signed(10), 100));
+		assert_ok!(Staking::bond_extra(Origin::signed(11), 100));
 		// There should be 100 more `total` and `active` in the ledger
 		assert_eq!(Staking::ledger(&10), Some(StakingLedger { stash: 11, total: 1000 + 100, active: 1000 + 100, unlocking: vec![] }));
 
 		// Call the bond_extra function with a large number, should handle it
-		assert_ok!(Staking::bond_extra(Origin::signed(10), u64::max_value()));
+		assert_ok!(Staking::bond_extra(Origin::signed(11), u64::max_value()));
 		// The full amount of the funds should now be in the total and active
 		assert_eq!(Staking::ledger(&10), Some(StakingLedger { stash: 11, total: 1000000, active: 1000000, unlocking: vec![] }));
 
@@ -1161,7 +1161,7 @@ fn bond_extra_and_withdraw_unbonded_works() {
 		assert_eq!(Staking::stakers(&11), Exposure { total: 1000, own: 1000, others: vec![] });
 
 		// deposit the extra 100 units
-		Staking::bond_extra(Origin::signed(10), 100).unwrap();
+		Staking::bond_extra(Origin::signed(11), 100).unwrap();
 
 		assert_eq!(Staking::ledger(&10), Some(StakingLedger { stash: 11, total: 1000 + 100, active: 1000 + 100, unlocking: vec![] }));
 		// Exposure is a snapshot! only updated after the next era update.