OperationalStaking.sol has rounding errors.

Summary

_unstake function and _redeemRewards function of OperationalStaking.sol round down the calculations of shares to remove/burn. This issue causes CQT tokens to be transferred/redeemed to users more than it should be.

Vulnerability Detail

OperationalStaking.sol#_redeemRewards function is the following.

    function _redeemRewards(uint128 validatorId, address beneficiary, uint128 amount) internal {
        require(validatorId < validatorsN, "Invalid validator");
        require(beneficiary != address(0x0), "Invalid beneficiary");
        Validator storage v = _validators[validatorId];
        Staking storage s = v.stakings[msg.sender];

        require(!v.frozen, "Validator is frozen");

        // how many tokens a delegator/validator has in total on the contract
        // include earned commission if the delegator is the validator
        uint128 totalValue = _sharesToTokens(s.shares, v.exchangeRate);

        // how many tokens a delegator/validator has "unlocked", free to be redeemed
        // (i.e. not staked or in unstaking cooldown)
        uint128 totalUnlockedValue = (totalValue < s.staked) ? 0 : (totalValue - s.staked);

        bool isRedeemingAll = (amount == 0 || amount == totalUnlockedValue); // amount is 0 when it's requested to redeem all rewards

        // make sure rewards exist
        // (note that this still works in the case where we're redeeming all! always doing this check saves a branch op)
        require(amount <= totalUnlockedValue, "Cannot redeem amount greater than held, unstaked rewards");

        uint128 effectiveAmount = isRedeemingAll ? totalUnlockedValue : amount;

        // can only redeem above redeem threshold
614:    require(effectiveAmount >= REWARD_REDEEM_THRESHOLD, "Requested amount must be higher than redeem threshold");

616:    uint128 sharesToBurn = _tokensToShares(effectiveAmount, v.exchangeRate);

        // sometimes, due to conversion inconsistencies, sharesToBurn might end up larger than s.shares;
        // so we clamp sharesToBurn to s.shares (the redeemer gets trivially more value out in this case)
        if (sharesToBurn > s.shares) sharesToBurn = s.shares;

        // sanity check: sharesToBurn should never be zero while effectiveAmount is nonzero, as this
        // would enable infinite draining of funds
624:    require(sharesToBurn > 0, "Underflow error");

        unchecked {
            v.totalShares -= sharesToBurn;
        }
        unchecked {
            s.shares -= sharesToBurn;
        }

        emit RewardRedeemed(validatorId, beneficiary, effectiveAmount);
        _transferFromContract(beneficiary, effectiveAmount);
    }

_tokensToShares (of L616) and _sharesToTokens functions are the following.

    function _tokensToShares(uint128 amount, uint128 rate) internal pure returns (uint128) {
        return uint128((uint256(amount) * DIVIDER) / uint256(rate));
    }

    function _sharesToTokens(uint128 sharesN, uint128 rate) internal pure returns (uint128) {
        return uint128((uint256(sharesN) * uint256(rate)) / DIVIDER);
    }

As can be seen, _tokensToShares function round down the calculation of shares.

Now, let us estimate the amount of CQT tokens effectiveAmount - _sharesToTokens(sharesToBurn, v.exchangeRate) that are overpaid to users. In L616, _sharesToTokens(sharesToBurn, v.exchangeRate) <= effectiveAmount < _sharesToTokens(sharesToBurn + 1, v.exchangeRate) holds. On the other hand, L614 means effectiveAmount >= REWARD_REDEEM_THRESHOLD = 10**8, L624 means sharesToBurn >= 1 and we can see that v.exchangeRate >= DIVIDER = 10**18 from the codes of contract.

Assume that effectiveAmount = 10**8.

When v.exchangeRate = DIVIDER + 1, L616 means sharesToBurn = 10**8 - 1 and thus effectiveAmount - _sharesToTokens(sharesToBurn, v.exchangeRate) = 1 holds true. That is, the caller(validator/delegator) will receive 1/(10^8) times more CQT tokens than he should receive.

When v.exchangeRate = (10**8 / 2) * DIVIDER + 1, L616 means sharesToBurn = 1 and thus effectiveAmount - _sharesToTokens(sharesToBurn, v.exchangeRate) = (10**8 / 2) - 1 holds true. That is, the caller receives almost twice more CQT tokens than he/she should receive.

In the meantime, it is necessary to consider the case of v.exchangeRate > (10**8 / 2) * DIVIDER. It is because the case of sharesToBurn = 0 in L624 holds only if v.exchangeRate > effectiveAmount * DIVIDER >= (10**8) * DIVIDER.

Impact

the caller(validator/delegator) of _redeemRewards function can receive 1/(10^8) ~ 1/2 time more CQT tokens than he/she should receive. This issue causes lack of CQT tokens in the contract.

The same problem also exists in _unstake function.

Code Snippet

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

Tool used

Manual Review

Recommendation

Modify OperationalStaking.sol#_redeemRewards function as follows.

    function _redeemRewards(uint128 validatorId, address beneficiary, uint128 amount) internal {
        ......
        // can only redeem above redeem threshold
        require(effectiveAmount >= REWARD_REDEEM_THRESHOLD, "Requested amount must be higher than redeem threshold");

        uint128 sharesToBurn = _tokensToShares(effectiveAmount, v.exchangeRate);
++      if (sharesToBurn * v.exchangeRate < effectiveAmount * DIVIDER) sharesToBurn += 1;

        // sometimes, due to conversion inconsistencies, sharesToBurn might end up larger than s.shares;
        // so we clamp sharesToBurn to s.shares (the redeemer gets trivially more value out in this case)
        if (sharesToBurn > s.shares) sharesToBurn = s.shares;
        ......
    }

Modify _unstake function similarly.

Duplicate of #39

sherlock-audit / 2023-11-covalent-judging

dany.armstrong90 - OperationalStaking.sol has rounding errors. #104