Disallow using existing TSS account IDs in Staking pallet (#657)

* Add check to staking pallet to disallow using existing TSS account IDs

* Update CHANGELOG

* Shorter description in changelog

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

* Move ensure assertion to beginning of fn

* Move other ensure assertion to beginning of fn

* Update / refactor tests

---------

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

CHANGELOG.md

@@ -29,6 +29,7 @@ At the moment this project **does not** adhere to
 - Change test-cli default access mode and update readme for recent changes ([#643](https://github.com/entropyxyz/entropy-core/pull/643))
  - Test CLI - dont send hardcoded auxiliary data by default when signing ([#614](https://github.com/entropyxyz/entropy-core/pull/614))
   - Add proactive refresh keys on-chain ([#629](https://github.com/entropyxyz/entropy-core/pull/629))
+-  Disallow using existing TSS account IDs in Staking pallet #657 ([#657](https://github.com/entropyxyz/entropy-core/pull/657))
 
 ## [0.0.10](https://github.com/entropyxyz/entropy-core/compare/release/v0.0.9...release/v0.0.10) - 2024-01-24
 

pallets/staking/src/lib.rs

@@ -222,6 +222,7 @@ pub mod pallet {
         NoThresholdKey,
         InvalidValidatorId,
         SigningGroupError,
+        TssAccountAlreadyExists,
     }
 
     #[pallet::event]
@@ -288,6 +289,11 @@ pub mod pallet {
             tss_account: T::AccountId,
             x25519_public_key: X25519PublicKey,
         ) -> DispatchResult {
+            ensure!(
+                !ThresholdToStash::<T>::contains_key(&tss_account),
+                Error::<T>::TssAccountAlreadyExists
+            );
+
             let who = ensure_signed(origin)?;
             let stash = Self::get_stash(&who)?;
             let validator_id = <T as pallet_session::Config>::ValidatorId::try_from(stash)
@@ -351,6 +357,12 @@ pub mod pallet {
                 endpoint.len() as u32 <= T::MaxEndpointLength::get(),
                 Error::<T>::EndpointTooLong
             );
+
+            ensure!(
+                !ThresholdToStash::<T>::contains_key(&tss_account),
+                Error::<T>::TssAccountAlreadyExists
+            );
+
             let stash = Self::get_stash(&who)?;
             pallet_staking::Pallet::<T>::validate(origin, prefs)?;
             let validator_id = <T as pallet_session::Config>::ValidatorId::try_from(stash)

pallets/staking/src/tests.rs

@@ -74,14 +74,49 @@ fn it_takes_in_an_endpoint() {
                 RuntimeOrigin::signed(4),
                 pallet_staking::ValidatorPrefs::default(),
                 vec![20, 20],
-                3,
+                5,
                 NULL_ARR
             ),
             Error::<Test>::NotController
         );
     });
 }
 
+#[test]
+fn it_will_not_allow_validator_to_use_existing_tss_account() {
+    new_test_ext().execute_with(|| {
+        assert_ok!(FrameStaking::bond(
+            RuntimeOrigin::signed(1),
+            100u64,
+            pallet_staking::RewardDestination::Account(1),
+        ));
+        assert_ok!(Staking::validate(
+            RuntimeOrigin::signed(1),
+            pallet_staking::ValidatorPrefs::default(),
+            vec![20],
+            3,
+            NULL_ARR
+        ));
+
+        // Attempt to call validate with a TSS account which already exists
+        assert_ok!(FrameStaking::bond(
+            RuntimeOrigin::signed(2),
+            100u64,
+            pallet_staking::RewardDestination::Account(2),
+        ));
+        assert_noop!(
+            Staking::validate(
+                RuntimeOrigin::signed(2),
+                pallet_staking::ValidatorPrefs::default(),
+                vec![20],
+                3,
+                NULL_ARR
+            ),
+            Error::<Test>::TssAccountAlreadyExists
+        );
+    });
+}
+
 #[test]
 fn it_changes_endpoint() {
     new_test_ext().execute_with(|| {
@@ -132,6 +167,62 @@ fn it_changes_threshold_account() {
             Staking::change_threshold_accounts(RuntimeOrigin::signed(4), 5, NULL_ARR),
             Error::<Test>::NotController
         );
+
+        // Check that we cannot change to a TSS account which already exists
+        assert_ok!(FrameStaking::bond(
+            RuntimeOrigin::signed(2),
+            100u64,
+            pallet_staking::RewardDestination::Account(2),
+        ));
+        assert_ok!(Staking::validate(
+            RuntimeOrigin::signed(2),
+            pallet_staking::ValidatorPrefs::default(),
+            vec![20],
+            5,
+            NULL_ARR
+        ));
+
+        assert_noop!(
+            Staking::change_threshold_accounts(RuntimeOrigin::signed(1), 5, NULL_ARR),
+            Error::<Test>::TssAccountAlreadyExists
+        );
+    });
+}
+
+#[test]
+fn it_will_not_allow_existing_tss_account_when_changing_threshold_account() {
+    new_test_ext().execute_with(|| {
+        assert_ok!(FrameStaking::bond(
+            RuntimeOrigin::signed(1),
+            100u64,
+            pallet_staking::RewardDestination::Account(1),
+        ));
+        assert_ok!(Staking::validate(
+            RuntimeOrigin::signed(1),
+            pallet_staking::ValidatorPrefs::default(),
+            vec![20],
+            3,
+            NULL_ARR
+        ));
+
+        // Check that we cannot change to a TSS account which already exists
+        assert_ok!(FrameStaking::bond(
+            RuntimeOrigin::signed(2),
+            100u64,
+            pallet_staking::RewardDestination::Account(2),
+        ));
+        assert_ok!(Staking::validate(
+            RuntimeOrigin::signed(2),
+            pallet_staking::ValidatorPrefs::default(),
+            vec![20],
+            5,
+            NULL_ARR
+        ));
+
+        assert_noop!(
+            Staking::change_threshold_accounts(RuntimeOrigin::signed(1), 5, NULL_ARR),
+            Error::<Test>::TssAccountAlreadyExists
+        );
     });
 }