Ironsidesec - New reward tokens from `ZivoeRewards.sol` can be drained completely

[sherlock-admin3]

Ironsidesec

high

New reward tokens from ZivoeRewards.sol can be drained completely

Summary

Issue occurs on ZivoeRewards.sol

Impact - when new reward tokens are added, they can be drained within next block root cause - accountRewardPerTokenPaid for new reward tokens == 0, so its easy for old stakers who staked even before this new reward addition to claim rewards. Likelihood - easy, with MEV, it can be done in 1–3 blocks.

This issue would not occur if there's only one reward token because a user cannot make his staking balance > 0 without calling stake(). So he definitely has to call stake(), which will update accountRewardPerTokenPaid to the latest. But for new additional rewards, he already has staking balance > 0 , so he doesn't have to call stake() which would update his accountRewardPerTokenPaid.

Vulnerability Detail

ZivoeRewards contract allows owner to add new reward token and allows anyone to deposit rewards.

Normal reward claiming flow:

1. A new reward token for the first time is added. And some reward tokens are added, and its rewardPerTokenStored and rewardRate are updated.
2. user stakes, and his balance and accountRewardPerTokenPaid is updated to latest rewardPerTokenStored making him eligible to reward emissions from now on.
3. After a few days rewardPerTokenStored will increase when new rewards are added. And the user will claim rewards = bal * (new rewardPerTokenStored - his current accountRewardPerTokenPaid). And after updating his rewards, rewardPerTokenStored will be assigned to his accountRewardPerTokenPaid.
4. This is how staking and reward mechanisms work.

https://github.com/sherlock-audit/2024-03-zivoe/blob/01e00e6f27b58392a6fa0b82c84a46a783a0df3c/zivoe-core-foundry/src/ZivoeRewards.sol#L228-L243

File: 2024-03-zivoe\zivoe-core-foundry\src\ZivoeRewards.sol

228:    function depositReward( address _rewardsToken, uint256 reward)
            external
    >>>>    updateReward(address(0))
            nonReentrant
        {
229:         IERC20(_rewardsToken).safeTransferFrom(_msgSender(), address(this), reward);
230:
231:         // Update vesting accounting for reward (if existing rewards being distributed, increase proportionally).
232:         if (block.timestamp >= rewardData[_rewardsToken].periodFinish) {
233:             rewardData[_rewardsToken].rewardRate = reward.div(rewardData[_rewardsToken].rewardsDuration);
234:         } else {
235:             uint256 remaining = rewardData[_rewardsToken].periodFinish.sub(block.timestamp);
236:             uint256 leftover = remaining.mul(rewardData[_rewardsToken].rewardRate);
237:             rewardData[_rewardsToken].rewardRate = reward.add(leftover).div(rewardData[_rewardsToken].rewardsDuration);
238:         }
239:
240:         rewardData[_rewardsToken].lastUpdateTime = block.timestamp;
241:         rewardData[_rewardsToken].periodFinish = block.timestamp.add(rewardData[_rewardsToken].rewardsDuration);
242:         emit RewardDeposited(_rewardsToken, reward, _msgSender());
243:     }

Attack path:

1. There's already 1 reward token USDT. And a user has already staked and been claiming rewards for six months.
2. A new reward token, DAI, is added, and 1000 DAI are deposited per staking token as a reward for 30 days. It also updates  rewardPerTokenStored and rewardRate for DAI.
3. Now, as soon as rewards are deposited, the old user can claim this new reward token and completely drain all rewards if his staking token balance is high enough.
4. It is possible because his accountRewardPerTokenPaid is still 0, because he never staked, so it would update his accountRewardPerTokenPaid to latest rewardPerTokenStored.

So for rewards claiming, earned = bal (latest rewardPerTokenStored -  his current accountRewardPerTokenPaid ) = bal ( 1000 (uints DAI/token) - 0 )

This will be so high enough if his staking token balance is high, or even frontrun the deposit reward by depositing heavy. There would be enough time between new reward token addition and deposit reward call, so backrun the addreward token action with stake() and wait till reward deposition, then backrun it to claim the rewards.

Impact

New reward tokens can be drained completely. So high.

Code Snippet

https://github.com/sherlock-audit/2024-03-zivoe/blob/01e00e6f27b58392a6fa0b82c84a46a783a0df3c/zivoe-core-foundry/src/ZivoeRewards.sol#L228-L243

Tool used

Manual Review

Recommendation

Fix the issue either following 1 or 2 or 3. I chose the first because of its flexibility and features.

1. Look at how convex implemented multiple rewards ystem. there will be a staking contract A which will have default reward. If there's a new reward token to be added it will deply a new rewardContract and if any one stakes, it will change the balance on both this staking contract A and that new reward contract. example : L 879 of https://etherscan.deth.net/address/0xc583e81bB36A1F620A804D8AF642B63b0ceEb5c0 and L 855 of https://etherscan.deth.net/address/0x6dF312B6367F53e4b7875738d32DE7925A72a1CF

2. Change the ZivoeRewards contract's storage structure     track the timestamp and amounts, when the stake is called by user. And emit different reward tokens at different staking balances. Do not use the same staking balances to calculate rewards for all reward tokens    

3. Do not add new reward tokens, just swap the new reward token to first reward token and stream the rewards in only one token.

[sherlock-admin3]

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

panprog commented:

invalid, the scenario is incorrect, each reward token has independant rewardPerTokenStored and it starts from 0, so it's correct that user with some amount staked will start accumulating new reward token immediately.

[IronsideSec]

The Lead judge says

invalid, the scenario is incorrect, each reward token has independant rewardPerToken and it starts from 0, so it's correct that user with some amount staked will start accumulating new reward token immediately.

Its true, the New stakers will get rewarded. But at what rewardPertoken ? Since reward per token depends on total supply which includes all the previous staker's balance too, so the rewards get spread to old stakers too. If that is the intended design, then this issue is invalid.

But new stakers are attracted seeing the high % apr in the dapp, if new rewards are shared with old stakers deflating rewardPerToken, then the rate drops significantly. Fix would be to deploy new staking contract if new rewards are added. So only new stakers get these rewards.

POC and discussion comments https://gist.github.com/ironsidesec/9a6b5cacf3a558db6f099d90b408460b?permalink_comment_id=5053903#gistcomment-5053903

[panprog]

The POC shows exactly how the protocol is intended to work. USDC deposited has 3.868e8 rate per second, and "attacker" receives exactly this amount for 1 second. It is impossible to get more - in POC the attacker has 100% of the totalSupply, so he gets full per-second reward. If there are any other depositors, it's split between them.