staking: do not remove an invulnerable in case of bad solution (#10454)

Invulnerables are not automatically removed from the Invulnerables
storage when their solution is rejected.
Removal should occur only through governance, not automatically. 
An operational or network issue that leads to an incomplete submission
is much more likely than a bad faith action from an invulnerable.

Close paritytech-secops/srlabs_findings#602.

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Author: sigurpol

prdoc/pr_10454.prdoc

@@ -0,0 +1,10 @@
+title: 'staking: do not remove an invulnerable in case of bad solution'
+doc:
+- audience: Runtime Dev
+  description: |-
+    Invulnerables are not automatically removed from the Invulnerables storage when their solution is rejected.
+    Removal should occur only through governance, not automatically.
+    An operational or network issue that leads to an incomplete submission is much more likely than a bad faith action from an invulnerable.
+crates:
+- name: pallet-election-provider-multi-block
+  bump: patch

substrate/frame/election-provider-multi-block/src/signed/mod.rs

@@ -1025,7 +1025,11 @@ impl<T: Config> Pallet<T> {
 		if let Some((loser, metadata)) =
 			Submissions::<T>::take_leader_with_data(current_round).defensive()
 		{
-			// Slash the deposit
+			// Slash the deposit.
+			// Note that an invulnerable is not expelled from the list despite the slashing.
+			// Removal should occur only through governance, not automatically. An operational or
+			// network issue that leads to an incomplete submission is much more likely than a bad
+			// faith action from an invulnerable.
 			let slash = metadata.deposit;
 			let _res = T::Currency::burn_held(
 				&HoldReason::SignedSubmission.into(),
@@ -1036,7 +1040,6 @@ impl<T: Config> Pallet<T> {
 			);
 			debug_assert_eq!(_res, Ok(slash));
 			Self::deposit_event(Event::<T>::Slashed(current_round, loser.clone(), slash));
-			Invulnerables::<T>::mutate(|x| x.retain(|y| y != &loser));
 
 			// Try to start verification again if we still have submissions
 			if let crate::types::Phase::SignedValidation(remaining_blocks) =

substrate/frame/election-provider-multi-block/src/signed/tests.rs

@@ -1594,7 +1594,7 @@ mod invulnerables {
 	}
 
 	#[test]
-	fn slashed_invulnerable_is_expelled() {
+	fn slashed_invulnerable_is_not_expelled() {
 		ExtBuilder::signed().build_and_execute(|| {
 			roll_to_signed_open();
 			assert_full_snapshot();
@@ -1630,8 +1630,8 @@ mod invulnerables {
 				                                                                         * (7) ^^^^ */
 			);
 
-			// Verify invulnerable is expelled
-			assert!(!Invulnerables::<T>::get().contains(&99));
+			// Verify invulnerable is not expelled. It remains in the list despite being slashed.
+			assert!(Invulnerables::<T>::get().contains(&99));
 		});
 	}
 }