PUSH0 - New staking between reward epochs will dilute rewards for existing stakers. Anyone can then front-run `OperationalStaking.rewardValidators()` to steal rewards

[sherlock-admin2]

PUSH0

high

New staking between reward epochs will dilute rewards for existing stakers. Anyone can then front-run OperationalStaking.rewardValidators() to steal rewards

Summary

In the Covalent Network, validators perform work to earn rewards, which is distributed through OperationalStaking. The staking manager is expected to regularly invoke rewardValidators() to distribute staking rewards accordingly to the validator, and its delegators.

However, the function takes into account all existing stakes, including new ones. This makes newer stakes being counted equally to existing stakes, despite newer stakes haven't existed for a working epoch yet.

An attacker can also then front-run rewardValidators() to steal a share of the rewards. The attacker gains a share of the reward, despite not having fully staked for the corresponding epoch.

Vulnerability Detail

The function rewardValidators() is callable only by the staking manager to distribute rewards to validators. The rewards is then immediately distributed to the validator, and all their delegators, proportional to the amount staked.

However, any new staking in-between reward epochs still counts as staking. They receive the full reward amount for the epoch, despite not having staked for the full epoch.

An attacker can abuse this by front-run the staking manager's distribution with a stake transaction. The attacker stakes a certain amount right before the staking manager distributes rewards, then the attacker is already considered to have a share of the reward, despite not having staked during the epoch they were entitled to.

This also applies to re-stakings, i.e. unstaked tokens that are re-staked into the same validator: Any stake recovers made through recoverUnstaking() is considered a new stake. Therefore an attacker can use the same funds to repeatedly perform the attack.

Proof of concept

0. Alice is a validator. She has two delegators:
   • Alicia delegating $5000$ CQT.
   • Alisson delegating $15000$ CQT.
   • Alice herself stakes $35000$ CQT, for a total of $50000$.
1. The staking manager distributes $1000$ CQT to Alice. This is then distributed proportionally across Alice and her delegators.
2. Bob notices the staking manager, and front-runs with a $50000$ CQT stake into Alice.
   • Alice now has a total stake of $100000$, with half of it belonging to Bob.
   • Bob's staking tx goes through before the staking manager's tx.
3. Bob now owns half of Alice's shares. He is distributed half of the rewards, despite having staked into Alice for only one second.
4. Bob can repeat this attack for as often as they wish to, by unstaking then restaking through recoverUnstaking().

Impact

Unfair distribution of rewards. New stakers get full rewards for epochs they didn't fully stake into.

Code Snippet

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

Tool used

Manual Review

Recommendation

Use a checkpoint-based shares computation. An idea can be as follow:

• For new stakings, don't mint shares for them right away, but add them into a "pending stake" variable.
• For unstakings, do burn their shares right away, as with the normal setting.
• When reward is distributed, distribute to existing stakes first (i.e. increase the share price only for the existing shares), only then mint new shares for the pending stakes.

[sudeepdino008]

• there is a bit of unpredictability with which rewardValidators is called. It might not be possible to ascertain when it will be called.
• What value does Bob get by getting in just before the reward distribution, and then getting out afterwards? Why would he not simply remain staked?
• Note that for each quorum, the stake states (needed for reward calculation) is fetched for the block height (in which that particular quorum was emitted). Even though rewardValidators is called every 12 hours, the offchain system is doing a lot more frequent stake state checks.

[sherlock-admin2]

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

takarez commented:

valid: users that stakes immediately will be accounted for during the reward distribution; medium as there is no loss of funds; medium(4)

[nevillehuang]

1. This can be done by observing the mempool for calls to rewardValidators() given staking contract is deployed on mainnet
2. I believe the PoC in #34 can help you understand a possible impact:

In the simple demonstration below, we show that an attacker _BOB may steal ~66% of Validator _ALICE's rewards by frontrunning an incoming call to rewardValidators(uint128,uint128[],uint128[]):

3. How does this prevent rewards siphoning stated in this issue?

[sudeepdino008]

1. you can observe the mempool for rewardValidators, but by that point, the offchain system has already calculated the validator rewards by taking the stake states into account i.e. when the call is made for rewardValidators, the rewards for each validator is already decided. Front running this call wouldn't change anything.
2. a cooldown for recoverUnstaking is implemented which prevents delegators from entering and exiting staked positions with the same validator.
3. BOB really doesn't "steal" the rewards; He entered the staking position at a particular time and was part of the staking pool. Surely we can make it difficult for them to re-enter after exiting, which is what 2. achieves.

https://github.com/covalenthq/cqt-staking/pull/125/commits/a609cca0426cb22cbf5064212341c14c288efeda for 2.

[CergyK]

Fix LGTM

[sherlock-admin]

The protocol team fixed this issue in PR/commit https://github.com/covalenthq/cqt-staking/commit/a609cca0426cb22cbf5064212341c14c288efeda.

[sherlock-admin]

The Lead Senior Watson signed off on the fix.