OperationalStaking.sol: there is no consistency in the application of validatorEnableMinStake.

Summary

enableValidator function and _unstake function of OperationalStaking.sol compares validatorEnableMinStake with different amount respectively. This makes the validatorEnableMinStake not applicable correctly.

Vulnerability Detail

OperationalStaking.sol#enableValidator function is the following.

    function enableValidator(uint128 validatorId) external onlyStakingManagerOrOwner {
        require(validatorId < validatorsN, "Invalid validator");
        Validator storage v = _validators[validatorId];

        if (v.disabledAtBlock == 0) {
            // if validator is already enabled, succeed quietly
            return;
        }

        uint128 staked = _sharesToTokens(v.stakings[v._address].shares, v.exchangeRate);

344:    require(staked >= validatorEnableMinStake, "Validator is insufficiently staked");

        v.disabledAtBlock = 0;
        emit ValidatorEnabled(validatorId);
    }

L344 compares validatorEnableMinStake with staked which is calculated using validator.stakings[validator._address].shares. OperationalStaking.sol#_unstake function is the following.

    function _unstake(uint128 validatorId, uint128 amount) internal {
        require(validatorId < validatorsN, "Invalid validator");

        Validator storage v = _validators[validatorId];
        Staking storage s = v.stakings[msg.sender];
        ......
        // disable validator if they unstaked to below their required self-stake
523:    if (isValidator && validatorEnableMinStake > 0 && v.disabledAtBlock == 0 && s.staked < validatorEnableMinStake) {
            uint256 disabledAtBlock = block.number;
            v.disabledAtBlock = disabledAtBlock;
            emit ValidatorDisabled(validatorId, disabledAtBlock);
        }
        ......
    }

As can be seen, L523 compares validatorEnableMinStake directly with validator.stakings[validator._address].staked. Now, staked and shares of Staking struct are not proportional and vary according to different business logic, respectively. For instance, it is also possible to have staked = 0 and shares > 0. Thus, the validatorEnableMinStake cannot be applied consistently.

Impact

The validatorEnableMinStake cannot be applied correctly, resulting in unexpected errors.

Code Snippet

https://github.com/sherlock-audit/2023-11-covalent/blob/main/cqt-staking/contracts/OperationalStaking.sol#L344

Tool used

Manual Review

Recommendation

OperationalStaking.sol#_stake function is the following.

    function _stake(uint128 validatorId, uint128 amount, bool withTransfer) internal {
        ......
        if (isValidator) {
            // compares with newStaked to ignore compounded rewards
            require(newStaked <= validatorMaxStake, "Validator max stake exceeded");
        } else {
            // cannot stake more than validator delegation max cap
432:        uint128 delegationMaxCap = v.stakings[v._address].staked * maxCapMultiplier;
            uint128 newDelegated = v.delegated + amount;
            require(newDelegated <= delegationMaxCap, "Validator max delegation exceeded");
            v.delegated = newDelegated;
        }
        ......
    }

From above, we can see that it is more reasonable to compare validatorEnableMinStake with staked of Staking struct. Modify OperationalStaking.sol#enableValidator function as follows.

    function enableValidator(uint128 validatorId) external onlyStakingManagerOrOwner {
        require(validatorId < validatorsN, "Invalid validator");
        Validator storage v = _validators[validatorId];

        if (v.disabledAtBlock == 0) {
            // if validator is already enabled, succeed quietly
            return;
        }

--      uint128 staked = _sharesToTokens(v.stakings[v._address].shares, v.exchangeRate);
--
--      require(staked >= validatorEnableMinStake, "Validator is insufficiently staked");
++      require(v.stakings[v._address].staked >= validatorEnableMinStake, "Validator is insufficiently staked");

        v.disabledAtBlock = 0;
        emit ValidatorEnabled(validatorId);
    }

sherlock-admin2 commented 7 months ago

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

takarez commented:

invalid

nevillehuang commented 6 months ago

Invalid, no issue here logic is correct.

sherlock-audit / 2023-11-covalent-judging

dany.armstrong90 - OperationalStaking.sol: there is no consistency in the application of validatorEnableMinStake. #105