SIP-68: Minor enhancements to StakingRewards.sol

Simple Summary

The StakingRewards contracts for liquidity mining need some minor enhancements.

Abstract

Enhancements include:

• Recover airdropped rewards tokens from other protocols such as BAL & CRV
• Ability to update the rewards duration
• Remove the redundant LPTokenWrapper
• Refactor to set rewards and staking tokens via the constructor on deployment
• Adding Pausable and notPaused to stake() to prevent staking into deprecated pools
• Fix a potential overflow bug in the reward notification function

Motivation

Recover airdropped rewards tokens from other protocols such as BAL & CRV

When providing liquidity to Balancer or Curve.fi and you stake your LP tokens in the Synthetix StakingRewards contract you will also be eligible for BAL and CRV Liquidity Mining rewards also and potentially other AMMs in the future.

This would include a recoverERC20 function that is accessible by the owner only, the protocolDAO and can recover any ERC20 except the staking and rewards tokens to protect the stakers that their LP tokens and their rewards tokens are not accessible by the owner. These additional LP rewards will then be distributed to the LP providers.

Update the rewards duration

Synthetix often runs trials on Liquidity Mining. Right now the Rewards duration is 7 days hard coded. Allowing a configurable rewardsDuration means the sDAO can set the duration and supply the total duration rewards for the trial without having to send the rewards manually each week. i.e. the curve renBTC/sBTC/wBTC pool gets 10 BPT per week. Where the trial is 10 weeks we could have set the duration to 10 weeks and send all 100 BPT upfront and it will distribute for the full term of the trial.

When a trial is complete the contract can either be shut down or wired into the Synthetix Inflationary supply via the Rewards Distribution contract where the rewardsDuration can be set back to 7 days and automatically receive SNX weekly. Similar to current LP SNX rewards incentives.

Remove the redundant LPToken Wrapper

The LPTokenWrapper added additional complexity to the code without adding any additional benefits. To simplify the code we propose to remove it.

Refactor to set rewards and staking tokens via the constructor on deployment

The staking and rewards tokens were hard coded addresses in each contract. Now that there are many of these on MAINNET and deploying almost 1 a week, instead of having to edit the code directly it is prefered to send the staking and rewards tokens as arguments to the constructor on contract creation.

Pause stake when rewards completed

When a StakingRewards campaign has completed the contract needs to prevent anyone from staking into it. They won't accrue rewards and can cause blocking issues with inverse Synths that need to be rebalanced which need to be purged. Adding Pausable.sol and modifier notPaused to stake() will allow the admin to set paused to true preventing anyone from staking. SelfDestructible has not been implemented and given the amount of value in these contracts probably best not to implement.

Potential overflow bug fix

Summary

There is a multiplication overflow that can occur inside the rewardPerToken function, on line 66:

lastTimeRewardApplicable().sub(lastUpdateTime).mul(rewardRate).mul(1e18).div(_totalSupply)

An overflow occurs whenever rewardRate >= 2^256 / (10^18 * (lastTimeRewardApplicable() - lastUpdateTime)).

This can happen when the updateReward modifier is invoked, which will cause the following functions to revert:

• earned
• stake
• withdraw
• getReward
• exit
• notifyRewardAmount

The reward rate is set inside notifyRewardAmount, if a value that is too large is provided to the function. Of particular note is that notifyRewardAmount is itself affected by this problem, which means that if the provided reward is incorrect, then the problem is unrecoverable.

Solution

The notifyRewardAmount transaction should be reverted if a value greater than 2^256 / 10^18 is provided. As an additional safety mechanism, this value will be required to be no greater than the remaining balance of the rewards token in the contract. This will both prevent the overflow, and also provide an additional check that the reward rate is being set to a value in the appropriate range (for example, no extra/missing zeroes).

Details

Specifically, this problem occurs when rewardRate is too high; it is set inside the notifyRewardAmount function on lines 114 and 118.

rewardRate = floor(reward / rewardsDuration) = (reward - k) / rewardsDuration

for some 0 <= k < rewardsDuration.

For the bug to occur, we need:

(reward - k) / rewardsDuration >= 2^256 / (10^18 * (lastTimeRewardApplicable - lastUpdateTime))
reward                         >= rewardsDuration * 2^256 / (10^18 * (lastTimeRewardApplicable - lastUpdateTime)) + k

Hence, we can ensure the bug does not occur if we force:

reward < rewardsDuration * 2^256 / (10^18 * (lastTimeRewardApplicable - lastUpdateTime))

So we should constrain reward to be less than the minimum value of the RHS.

The smallest possible value of lastUpdateTime is the block timestamp when notifyRewardAmount was last called. The largest possible value of lastTimeRewardApplicable is periodFinish, and periodFinish = notificationBlock.timestamp + rewardsDuration (line 121). Putting these together we have:

(lastTimeRewardApplicable - lastUpdateTime) <= rewardsDuration

Ergo, we need:

reward < rewardsDuration * 2^256 / (10^18 * rewardsDuration)
	                     = 2^256 / 10^18

So the problem will not emerge whenever we require

    reward < uint(-1) / UNIT

Technical Specification

• Add recoverERC20 and setRewardsDuration that have onlyOwner modifiers.
• constructor to take _rewardsToken & _stakingToken as arguments
• Refactor to remove the LPTokenWrapper contract. The original implementation to not include this.
• Revert the notifyRewardAmount transaction if the computer reward rate would pay out more than the balance of the contract over the reward period.
• Inherit the Pausable.sol contract and add modifier notPaused to stake()

Test Cases

• External Rewards Recovery
  • only owner can call recoverERC20
  • should revert if recovering staking token
  • should revert if recovering rewards token (SNX)
  • should revert if recovering the SNX Proxy
  • should retrieve external token from StakingRewards and reduce contracts balance
  • should retrieve external token from StakingRewards and increase owners balance
  • should emit RewardsDurationUpdated event
• setRewardsDuration
  • should increase rewards duration
  • should emit RewardsDurationUpdated event
  • should revert when setting setRewardsDuration before the period has finished
  • should distribute rewards
• Constructor & Settings
  • should set rewards token on constructor
  • should staking token on constructor
• Pausable
  • should revert when stake is called when paused is true
• Overflow bugfix
  • should revert notifyRewardAmount if reward is greater than the contract balance
  • should revert notifyRewardAmount if reward plus any leftover from the previous period is greater than the contract balance
  • should not revert notifyRewardAmount if reward is equal to the contract balance

Configurable Values (Via SCCP)

Please list all values configurable via SCCP under this implementation.

Copyright

Copyright and related rights waived via CC0.