[Staking Manager] Permission removal of non-PoS validator #739
Description
Context and scope
The staking manager allows anybody to remove non-PoS validators, such as the initial validators from the conversion or the PoA validator that were set before the L1 migrates to PoS.
I would argue that it is not desirable to allow anybody to do this. To attract PoS validators the L1 must have some kind of value (TVL, brand, ...). That means it is worth attacking. When the L1 launches with PoS or migrates to it, there is no stake at all and the validator set only consists of non-PoS validators. If an attacker is able to act fast and has access to any amount of the staking token (no matter how little), they can stake it and immediately remove all non-PoS validators before any honest PoS validators. With full control over the L1 they can then block any honest PoS validator from joining and steal whatever value is on the L1.
This can be circumvented by implementing an access control system for the removal of non-PoS validators. This way the L1 can define a non-PoS validator set that has for example 9 times the stake weight of the theoretical maximum stake weight for PoS validators if all tokens in circulation were staked.
This would give the non-PoS validators a minimum of 90% stake weight. When more and more PoS validators join in and the economic security of the L1 increases, the non-PoS validator can be removed one-by-one, slowly migrating from PoA to PoS. To make sure that not more then 20% of the stake are removed at once, the non-PoS validators need to have different weights:
Discussion
To allow for further customization and being able to clearly describe the permissions, I'd recommend using the OZ AccessControlUpgradeable pattern.
We need to add it to the StakingManagerSettings in IStakingManager.sol.
struct StakingManagerSettings {
ValidatorManager manager;
address nonPoSValidatorRemovalManager;
// ...
}
Add it to __StakingManager_init (omitted here to keep is concise) and __StakingManager_init_unchained:
// ...
import {ContextUpgradeable} from
"@openzeppelin/[email protected]/contracts/access/AccessControlUpgradeable.sol";
abstract contract StakingManager is
IStakingManager,
AccessControlUpgradeable
ContextUpgradeable,
ReentrancyGuardUpgradeable
{
bytes32 public constant NON_POS_VALIDATOR_REMOVAL_ROLE = keccak256("NON_POS_VALIDATOR_REMOVAL_ROLE");
error UnauthorizedNonPoSValidatorRemoval(address sender);
function __StakingManager_init_unchained(
ValidatorManager manager,
address nonPoSValidatorRemovalManager,
// ...
) internal onlyInitializing {
_grantRole(NON_POS_VALIDATOR_REMOVAL_ROLE, nonPoSValidatorRemovalManager)
StakingManagerStorage storage $ = _getStakingManagerStorage();
// ...
}
// ...
/**
* @dev Helper function that initiates the end of a PoS validation period.
* Returns false if it is possible for the validator to claim rewards, but it is not eligible.
* Returns true otherwise.
*/
function _initiatePoSValidatorRemoval(
bytes32 validationID,
bool includeUptimeProof,
uint32 messageIndex,
address rewardRecipient
) internal returns (bool) {
StakingManagerStorage storage $ = _getStakingManagerStorage();
$._manager.initiateValidatorRemoval(validationID);
// The validator must be fetched after the removal has been initiated, since the above call modifies
// the validator's state.
Validator memory validator = $._manager.getValidator(validationID);
// Non-PoS validators are required to boostrap the network, but are not eligible for rewards.
if (!_isPoSValidator(validationID)) {
if (!hasRole(NON_POS_VALIDATOR_REMOVAL_ROLE, _msgSender()) {
revert UnauthorizedNonPoSValidatorRemoval(_msgSender());
}
return true;
}
// ...
}
Side note: To enhance clarity, maybe we can catch the case even before entering the _initiatePoSValidatorRemoval function, since it could be confusing to check in that function if a validator is actually a PoS validator or not.
Alternatives
Instead of using the the AccessControl pattern, we could use the simpler Ownable pattern. On the one side, this may be confused with the owner of the Validator Manager, which can control the full validator set. Having a specific role with a self-describing name enhances clarity. On the other side, this approach would give easier access to features like transferring the ownership that are more complex with the AccessControl pattern.
Alternatively, we could also not use the OZ pattern by adding another field to the staking manager storage called `_nonPoSValidatorRemovalAdmin' and check for that. This would easily allow to make this feature optional by setting it to the 0x0 address if this access control is not desired by the L1:
if (!_isPoSValidator(validationID)) {
if ($._nonPoSValidatorRemovalAdmin != address(0) && _msgSender() != _nonPoSValidatorRemovalAdmin) {
revert UnauthorizedNonPoSValidatorRemoval(_msgSender());
}
return true;
}
Another alternative would be allowing to specify an owner for each initial validator so the authority to remove these validators could be split across different authorities. I don't see a large benefit and something similar can be achieved by making the single removal manager a smart contract with a permission structure that only allows specific addresses to remove specific non-PoS validators.
Open questions
None at the moment