Vulnerability in pallet-staking-extension's change_threshold_accounts and validate extrinsics

[ameba23]

change_threshold_accounts allows a validator to change the TSS account ID and x25519 public key associated with their validator ID:

entropy-core/pallets/staking/src/lib.rs

Line 286 in fe96ae4

pub fn change_threshold_accounts(

It also does the reverse - it updates which Validator ID is associated with the newly given TSS account ID on this line:

entropy-core/pallets/staking/src/lib.rs

Line 301 in fe96ae4

ThresholdToStash::<T>::insert(&tss_account, &validator_id);

This means a malicious validator can impersonate another validator, by making the threshold_to_stash getter function resolve to its own validator ID rather than the validator ID of the TSS server who actually owns the private key to that TSS account ID.

This is dangerous because we use threshold_to_stash in several places in entropy-tss, for example to get our own subgroup ID:

entropy-core/crates/threshold-signature-server/src/helpers/substrate.rs

Line 47 in fe96ae4

entropy::storage().staking_extension().threshold_to_stash(threshold_address);

This means validators can be tricked into thinking they are in a different subgroup, and therefore send keyshares to the wrong TSS nodes following DKG or proactive refresh. This is dangerous because it could be used to collect keyshares from other subgroups, and if repeatedly exploited, to get a full set of keyshares.

This could be mitigated by requiring that the caller prove ownership of the TSS account ID when calling change_threshold_accounts, for example with a signature.

[ameba23]

Im not 100% sure this exploit is possible, so please check this over.

[ameba23]

I think this issue also applies to validate as this also allows us to choose a TSS account ID which maybe already exists, without having to prove ownership of it:

entropy-core/pallets/staking/src/lib.rs

Line 367 in fe96ae4

ThresholdToStash::<T>::insert(&tss_account, validator_id);

The simplest non-breaking-change way to fix this would be to only allow TSS account IDs which don't already exist