SIP-77: StakingRewards bug fix's and Pausable stake()

Simple Summary

stake() needs to be pausable for completed incentives and two bug fixes.

Abstract

Enhancements include:

• Inheriting Pausable contract and add notPaused modifer to stake() to prevent staking into deprecated pools
• Fix a potential overflow bug in the reward notification function reported by samcsun
• Fix to setRewardsDuration to allow rewardsDuration to be updated after the initial setting

Motivation

Pause stake when rewards completed

When a StakingRewards campaign has completed the contract needs to prevent anyone from staking into it. The staker will not accrue any rewards and can cause blocking issues with inverse Synths that need to be purged so that they can be relanced. 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

Fix to setRewardsDuration to allow updates after the initial setting

setRewardsDuration was intended to allow the rewardsDuration to be set after the duration had completed. However a flaw in the require meant it could be changed after the initial setting.

Current code

require(periodFinish == 0 || block.timestamp > periodFinish);

Proposed change

require(block.timestamp > periodFinish);

Technical Specification

• Inherit the Pausable.sol contract and add modifier notPaused to stake()
• Revert the notifyRewardAmount transaction if the computer reward rate would pay out more than the balance of the contract over the reward period.
• Change the require in setRewardsDuration to only check the period has finished

Test Cases

• Pausable
  • should revert when stake is called when paused is true
  • should allow a call to stake to succeed when paused is false
• 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
• setRewardsDuration bug fix
  • should revert when setting setRewardsDuration before the period has finished
  • should update rewardsDuration when calling setRewardsDuration after the rewards period has finished

Configurable Values (Via SCCP)

Please list all values configurable via SCCP under this implementation.

Copyright

Copyright and related rights waived via CC0.