TempleGold

Summary

In TempleGoldStakingcontract, vestingPeriod can be updated by calling setVestingPeriod. However, updating vestingPeriod affects the vestingRate of stakers whose rewards are still not vested fully. This can also result claiming of more rewards by the users or revert of withdraw, withdrawAll and getReward function.

Vulnerability Details

vestingRateof stakerfor an epochis calculated using function _getVestingRate. The function uses _stakeInfo.fullyVestedAtto determine if rewards are fully vested. If rewards are not fully vested, the vestingRateis calculated using timeDifferenceand vestingPeriod. However, vestingPeriodused may be different from when userdid the staking if vestingPeriodis updated later.

https://github.com/Cyfrin/2024-07-templegold/blob/main/protocol/contracts/templegold/TempleGoldStaking.sol#L484-L493

https://github.com/Cyfrin/2024-07-templegold/blob/main/protocol/contracts/templegold/TempleGoldStaking.sol#L463-L482

There can be following 2 vulnerabilities due to this:

Vulnerability 1

If the new vestingPeriodis less than old vestingPeriod, rewards will be fully vested before _stakeInfo.fullyVestedAt. The issue with this is vestingRateof users in this case can exceed1e18 which can result in more amounts of rewards to be claimed by the user than their earned amount.

Steps to reporduce:

Initial vestingPeriod= 5 days = 5 * 24 * 3600 seconds

reward.periodFinish= 24 hours = 24 * 3600 seconds

rewardRate= 5

1) User calls stakeat time = 0seconds with amount = 1 ether. _stakeInfoswill be set as following:

rewardDataand other params will be set as following:

After 24 hours, at time = 24 * 3600. reward period is finished

2) User calls getReward with index = 1 at time = 24 hours = 24 * 3600.

rewardDataand other params will be updated as following:

3) ownerupdates vestingPeriodto 1 days.

3) At time = 4 days and 23 hours = 119 * 3600 seconds. User calls getRewardwith index = 1.

Thus, claimableRewards of user is more than expected due to increased vestingPeriod.

Vulnerability 2

If new vestingPeriodis more than old vestingPeriod, the vestingRatewill be less than expected while rewards will again be fully vested at _stakeInfo.fullyVestedAt. This type of vesting doesn't make sense for users because it won't be linear vesting. In this case, functions like withdraw, withdrawAll and getReward may revert for some stakers due to underflowin _earned function.

Let's see how can underflow happen in _earnedfunction in this case.

Some points to note:

rewardData.rewardPerTokenStored will keep on increasing as time passes because rewardswill be accumulated. It will increase in linear proportion till reward duration ends if there are no additional staking or withdrawals.

userRewardPerTokenPaid[_account][_index]stores vestingRate * rewardData.rewardPerTokenStoredwhen any user action is done(eg. staking, withdrawor claim of rewards.)

Steps to reporduce:

Initial vestingPeriod= 1 days = 24 * 3600 seconds

reward.periodFinish= 18 hours = 18 * 3600 seconds

rewardRate= 5

1) User calls stakeat time = 15seconds with amount = 1 ether. _stakeInfoswill be set as following:

rewardDataand other params will be set as following:

2) User calls getReward with index = 1 at time = 17 hours and 15 seconds = 17 * 3600 + 15 seconds .

rewardDataand other params will be updated as following:

3) after 1 hour at time = 18 hours and 15 seconds = 18 * 3600 + 15 seconds, reward's period finishes. ownerupdates vestingPeriodto 2 days.

New vesting Period = 2 days = 2 * 24 * 3600 seconds

after 15 secondsat time = 18 * 3600 + 15 + 15 seconds, user calls withdrawwhich will revert due to underflow.

3) User calls withdrawAllwith index = 1 at time = 18 * 3600 + 30 seconds.

calling updateReward for the user and index will revert. rewardDataand other params will be updated as following:

NOTE: vestingRateof user decreased due to new vesting period

Impact

update in vestingPeriodwill affect previously opened positions. Users can claim more funds than their expected rewards if new vestingPeriodis less.

If new vestingPeriodis more than previous vestingPeriod, functions like withdraw, withdrawAll and getRewardmay revert due to underflow.

Tools Used

Manual review and calculation on pen-paper.

Recommendations

Add the vestingPeriodin stakeInfostruct and do not use the updated vestingPeriodfor the previously opened positions.