bitsurfer - `sharesToBurn` on redeeming rewards doesn't rounding up, which tend towards favoring validators and Covalent slowly loosing the CQT

[sherlock-admin2]

bitsurfer

medium

sharesToBurn on redeeming rewards doesn't rounding up, which tend towards favoring validators and Covalent slowly loosing the CQT

Summary

sharesToBurn on redeeming rewards doesn't rounding up making validator slightly getting more advantage, while Covalent slowly loosing the CQT

Vulnerability Detail

On redeeming, sharesToBurn amount which is returning from _tokensToShares doesn't rounding up, making validator slightly getting more advantage, they are getting amount of CQT with lesser share to burn, thus Covalent will slowly decreasing their CQT

As we can see, _tokensToShares function clearly do a rounding down.

also, on line 620, reasoning of conversion inconsistency and sanity check doesn't related with this issue.

This rounding down, will make redeeming will burn or decrease lesser share than the amount (effectiveAmount).

File: OperationalStaking.sol
393:     function _tokensToShares(uint128 amount, uint128 rate) internal pure returns (uint128) {
394:         return uint128((uint256(amount) * DIVIDER) / uint256(rate));
395:     }
...
589:     function _redeemRewards(uint128 validatorId, address beneficiary, uint128 amount) internal {
...
611:         uint128 effectiveAmount = isRedeemingAll ? totalUnlockedValue : amount;
612: 
613:         // can only redeem above redeem threshold
614:         require(effectiveAmount >= REWARD_REDEEM_THRESHOLD, "Requested amount must be higher than redeem threshold");
615: 
616:         uint128 sharesToBurn = _tokensToShares(effectiveAmount, v.exchangeRate);
617: 
618:         // sometimes, due to conversion inconsistencies, sharesToBurn might end up larger than s.shares;
619:         // so we clamp sharesToBurn to s.shares (the redeemer gets trivially more value out in this case)
620:         if (sharesToBurn > s.shares) sharesToBurn = s.shares;
621: 
622:         // sanity check: sharesToBurn should never be zero while effectiveAmount is nonzero, as this
623:         // would enable infinite draining of funds
624:         require(sharesToBurn > 0, "Underflow error");
625: 
626:         unchecked {
627:             v.totalShares -= sharesToBurn;
628:         }
629:         unchecked {
630:             s.shares -= sharesToBurn;
631:         }
632: 
633:         emit RewardRedeemed(validatorId, beneficiary, effectiveAmount);
634:         _transferFromContract(beneficiary, effectiveAmount);
635:     }

Impact

Covalent slowly loosing the CQT reward due to burn share rounding more towards to validator

Code Snippet

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

Tool used

Manual Review

Recommendation

Consider to use rounding up on sharesToBurn

Duplicate of #39