sherlock-admin commented 7 months ago

Al-Qa-qa

medium

Validators can get prevented from unstaking all their tokens

Summary

Validators will not be able to unstake their tokens (in case of leaving the staking) if there are a lot of delegates.

Vulnerability Detail

The validator will get enabled when they stake > validatorEnableMinStake, and accepts other users to delegate their tokens to him, and if the validator unstaked his tokens with amount < validatorEnableMinStake it will be un-enable.

The problem arises when the validator staked tokens are > validatorEnableMinStake by a certain amount, and the number of delegates is relatively big, we will illustrate how, and why this occurs.

In OS::_unstake(), The validator maxCap check is done before validatorEnableMinStake check. So if the validator wanted to unstake all his tokens (has no plans to complete investing in the protocol for example), his function will get reverted if the validatorMaxCap decreases significantly.

OperationalStaking.sol#L466-L537

function _unstake(uint128 validatorId, uint128 amount) internal {
    ...

    bool isValidator = msg.sender == v._address;
    // checking for `newValidatorMaxCap`, and reverts if it does not met 
    if (isValidator && v.disabledAtBlock == 0) {
        // validators will have to disable themselves if they want to unstake tokens below delegation max cap
        uint128 newValidatorMaxCap = newStaked * maxCapMultiplier;

        require(v.delegated <= newValidatorMaxCap, "Cannot decrease delegation max-cap below current delegation while validator is enabled");
    }

    ...

    // disable validator if they unstaked to below their required self-stake
    if (isValidator && validatorEnableMinStake > 0 && v.disabledAtBlock == 0 && s.staked < validatorEnableMinStake) {
        uint256 disabledAtBlock = block.number;
        v.disabledAtBlock = disabledAtBlock;
        emit ValidatorDisabled(validatorId, disabledAtBlock);
    }

    ...
}

NOTE: the validator can not be able to withdraw tokens which makes validatorMaxCap smaller than validator.delegates, and this is the system design. we are pointing out that the validator will not be able to unstake all his tokens or below validatorEnableMinStake if he wants to un-enable himself. So if the validator wanted to unstake all his tokens and simply leaves the protocol he would not be able to do this.

Let's illustrate an example when validatorEnableMinStake = 35_000e18, and multiplier = 10.

validator.stakes	validator unstake tokens	delegated tokens	newValidatorMaxCap <=> delegated	reverts
35_000e18	35_000e18	0e18	0e18 = 0e18	no
35_000e18	35_000e18	10_000e18	0e18 < 10_000e18	yes
100_000e18	80_000e18	150_000e18	20_000e18 * 10 > 150_000e18	no
100_000e18	80_000e18	500_000e18	20_000e18 * 10 < 500_000e18	yes
350_000e18	320_000e18	200_000e18	30_000e18 * 10 > 200_000e18	no
350_000e18	320_000e18	2_000_000e18	30_000e18 * 10 < 2_000_000e18	yes

So from the table of results, we can conclude that:

The more delegates nears the validatorMaxCap, the more probability the validator will not be able to unstake all there tokens.
When validator.stakes are large compared to validator.delegates, things work properly
When validator.stakes are small compared to validator.delegates, validators will not be able to unstake all there tokens.

Impact

Validators will not be able to unstake all their tokens if they want to leave the staking.

Code Snippet

OperationalStaking.sol#L495-L502
OperationalStaking.sol#L523-L527
Tool used

Manual Review + Foundry

Recommendation

validatorEnableMinStake should be done before the validatorMaxCap check. So that the validator can unstake all his tokens, without getting his transaction reverted.

OS._unstake()


function _unstake(uint128 validatorId, uint128 amount) internal {
...

bool isValidator = msg.sender == v._address;

if (isValidator && validatorEnableMinStake > 0 && v.disabledAtBlock == 0 && newStaked < validatorEnableMinStake) {
uint256 disabledAtBlock = block.number;
v.disabledAtBlock = disabledAtBlock;
emit ValidatorDisabled(validatorId, disabledAtBlock);
}

// checking for newValidatorMaxCap, and reverts if it does not met if (isValidator && v.disabledAtBlock == 0) { // validators will have to disable themselves if they want to unstake tokens below delegation max cap uint128 newValidatorMaxCap = newStaked * maxCapMultiplier;
```
require(v.delegated <= newValidatorMaxCap, "Cannot decrease delegation max-cap below current delegation while validator is enabled");
```
}

...

// disable validator if they unstaked to below their required self-stake
if (isValidator && validatorEnableMinStake > 0 && v.disabledAtBlock == 0 && s.staked < validatorEnableMinStake) {
uint256 disabledAtBlock = block.number;
v.disabledAtBlock = disabledAtBlock;
emit ValidatorDisabled(validatorId, disabledAtBlock);
}

... }

sherlock-admin2 commented 7 months ago

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid: medium(9)

noslav commented 7 months ago

This issue begs the question: Do we want to allow validators to exit the system whenever possible even if they have a lot of delegations and are themselves staked at a lower (in comparison to total delegation amount or other validators)?

This could be a feature than a bug aka when the validator has a lot of delegations in proportion to their own stake the _unstake tx reverts, not allowing them to proceed. This means they have convince the delegators to first unstake their stakes and only then be able to execute their own unstake. By these means the protocol applies a pressure to keep validators with high delegations in the system (and not be able to rug pull delegators at a whim).

Covering cases

When validator.stakes are small compared to validator.delegates, validators will not be able to unstake all there tokens.
The more delegates nears the validatorMaxCap, the more probability the validator will not be able to unstake all there tokens.

noslav commented 7 months ago

fixed by move validatorEnableMinStake check before validatorMaxCap - sa98

sudeepdino008 commented 7 months ago

cc: @rogarcia Good point from @noslav. Right now if the validator wants to exit the system, they have to ask us to disable the validator and only then they can unstake. This doesn't work al the time though -- the validator is only blocked when the maxDelegationCap limit is crossed.

I'm still not sure if we should completely incorporate this, and allow validators to exit when they want. At least currently the validators with large delegations have to come to us and ask us to disable. Incorporating this change can surprise us.

nevillehuang commented 7 months ago

@noslav @sudeepdino008 This seems to me like a valid design consideration, given covalent wants to retain large delegation within the system, while still being trusted to rectify the situation if large validators wish to unstake from the system.

However, is the max delegation cap enforced during delegation to validators? If not this could be a griefing vector by delegates toward validators trying to unstake.

noslav commented 7 months ago

@nevillehuang @sudeepdino008 good points about reversing the argument on its head with the delegators being in control of what the validator should or shouldn't do - ideally this should be a governance process before it makes it on-chain (that being said we're working on getting upto speed on enabling the same for all CQT holders). In the meantime we have decided to move forward with the suggested change as ideally both delegators and validators should be able to come in and go in a permission-less way even with the current iteration of the network (and in the coming future with proposed migration from moonbeam onto ethereum), this translates to increased trust between all parties encouraging positive reinforcement behaviour and actions. Since setting up a validator requires a lot of work and responsibilities including the ones towards your delegators (the validator name is always known to delegators pre and post the fact). We just have to get better at penalizing explicit malicious behaviour, where coming and going within the network shouldn't be one of them for a system that is tending towards a fully permissionless setup.

noslav commented 7 months ago

also check commit fix check for validator disable below required self-stake caught during unit-tests

midori-fuse commented 6 months ago

Escalate

This issue has no real impacts. Validators who want to self-disable can still contact the Staking Manager/owner to do so. These entities are trusted, so it should be expected that they do their job correctly, resolving such requests in a timely manner.

No funds are lost, no functionalities are broken. Self-disabling cannot be considered a core functionality because:

There is no dedicated function for self-disabling, only through staking manager.
Quoting the repo's README (the internal one, not Sherlock's README, but this should still counts as code within the repo)

If a validator is willing to do so, the validator will have to disable itself through the StakingManager.

This implies that the intention was that the validator should be going through the Staking Manager to disable.

sherlock-admin commented 6 months ago

Escalate

This issue has no real impacts. Validators who want to self-disable can still contact the Staking Manager/owner to do so. These entities are trusted, so it should be expected that they do their job correctly, resolving such requests in a timely manner.

No funds are lost, no functionalities are broken. Self-disabling cannot be considered a core functionality because:

There is no dedicated function for self-disabling, only through staking manager.

Quoting the repo's README (the internal one, not Sherlock's README, but this should still counts as code within the repo)

If a validator is willing to do so, the validator will have to disable itself through the StakingManager.

This implies that the intention was that the validator should be going through the Staking Manager to disable.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

Oot2k commented 6 months ago

Agree with escalation, if validator wants to disable itself we should just use the staking manager. In addition to sponsors and judges comments: the report does not mention a grieving factor and

Another question is that it is really good to allow validators to unstake if they only take little risk, but a lot of delegator funds are at risk.

In my opinion based on the first point mentioned this report might be a good addition but should be judged as low, based on the fact that validators can disable themselves trough the manager at any time, and withdraw after this.

sudeepdino008 commented 6 months ago