Validator manager pallet fails to unapprove validators.

[PopcornPaws]

Description

The validator manager pallet has three functions exposed:

• add_validator
• remove_validator
• add_validator_again

And two main storage items:

• Validators (active validator set)
• ApprovedValidators (validators who were once added by a sudo entity, however they might not be active at the moment)

When running remove_validator the pallet is supposed to run do_remove_validator which removes the account from Validators and then run unapprove_validator which removes the account from the ApprovedValidators storage.

However, the pallet doesn't remove accounts from the ApprovedValidators set, only from the Validators set. Therefore, when you attempt to add the validator again, you get a Duplicate error, because the validator is already included in ApprovedValidators even though it was removed from Validators.

Solution

There's a missing line in unapprove_validator that actually overwrites the storage with the removed entry:

let mut approved_set = <ApprovedValidators<T>>::get();
approved_set.retain(|v| *v != validator_id);
<ApprovedValidators<T>>::put(approved_set); // MISSING

[PopcornPaws]

Not a high priority right now, we can circumvent this problem. Although, this functionality might be intended at some level, because the im-online pallet only removes active validators (when they go offline) and not approved validators. Thus, when a validator comes back online, they can re-join the network as validators because they are still approved.