Refactor: address remaining issues found by Spearbit audit (#16)

* docs(StakingRewards): add warning regarding `notifyRewardAmount` usage

* refactor(StakingRewards): add check in constructor

Prevent `StakingRewards` from using the same token for staking and rewards, which can lead to potential accounting issues.

* docs(StakingRewards): update link with the diff to the original code

* chore: remove NatSpec tag and fix typo

* docs: remove link to the diff to the original Synthetix code

* chore: remove diff link from `StakingRewards`

README.md

@@ -56,7 +56,7 @@ through a [`StakingRewards`](#stakingrewards) contract.
 
 #### `StakingRewards`
 
-`StakingRewards` is a port of [Synthetix `StakingRewards`][staking-rewards]. Full diff can be found [here](https://www.diffchecker.com/9JdI2pIN/). The changes made include:
+`StakingRewards` is a port of [Synthetix `StakingRewards`][staking-rewards]. The changes made include:
 
 - Upgrade to the Solidity version from 0.5.x to 0.8.x.
   - It was required some reorganization of the internal structure because of changes in the inheritance resolution
@@ -69,6 +69,12 @@ through a [`StakingRewards`](#stakingrewards) contract.
 - Update `setRewardsDuration()` to support changing the reward duration during an active distribution.
   - This allows the duration to be changed even when new reward is added immediately after the end of a distribution
     period, which would be the case when `rewardDistribution` is a smart contract distributing new reward every period.
+- Prevent `rewardsToken` and `stakingToken` from being the same token.
+  - Instead of using the _pull pattern_, this contract expect the address with the `rewardsDistribution` role to
+    transfer the tokens to it and call `notifyRewardAmount` with the correct value.
+  - There is a possible scenario where the amount of tokens transferred to the contract is less than the amount used as
+    a parameter in `notifyRewardAmount`. In such case, if the staking token and rewards token are the same, the staked
+    tokens would be incorporated into the new reward period, causing stakers to lose their capital.
 
 ## Contributing
 

src/synthetix/StakingRewards.sol

@@ -5,7 +5,6 @@
  *  - Update `setRewardDuration()` to support changing the reward duration during an active distribution.
  *
  * Original: https://github.com/Synthetixio/synthetix/blob/5e9096ac4aea6c4249828f1e8b95e3fb9be231f8/contracts/StakingRewards.sol
- *     Diff: https://www.diffchecker.com/9JdI2pIN/
  */
 
 // SPDX-FileCopyrightText: © 2019-2021 Synthetix
@@ -61,6 +60,8 @@ contract StakingRewards is IStakingRewards, Pausable, ReentrancyGuard {
         address _rewardsToken,
         address _stakingToken
     ) Pausable(_owner) {
+        require(_rewardsToken != _stakingToken, "Rewards and staking tokens must not be the same");
+
         rewardsToken = IERC20(_rewardsToken);
         stakingToken = IERC20(_stakingToken);
         rewardsDistribution = _rewardsDistribution;
@@ -135,6 +136,11 @@ contract StakingRewards is IStakingRewards, Pausable, ReentrancyGuard {
 
     /* ========== RESTRICTED FUNCTIONS ========== */
 
+    /// Before calling this function, the caller should send at least `reward` tokens to the contract.
+    /// Otherwise, if the amount sent is less than `reward` passed as a parameter,
+    /// the unclaimed rewards of other users would be used in the new period.
+    /// This would result in missing tokens, which might cause `getReward` to fail for some users.
+    /// We advise the use of wrapper contracts to perform the transfer and call this function atomically.
     function notifyRewardAmount(uint256 reward) external override onlyRewardsDistribution updateReward(address(0)) {
         if (block.timestamp >= periodFinish) {
             rewardRate = reward / rewardsDuration;
@@ -148,7 +154,7 @@ contract StakingRewards is IStakingRewards, Pausable, ReentrancyGuard {
         // This keeps the reward rate in the right range, preventing overflows due to
         // very high values of rewardRate in the earned and rewardsPerToken functions;
         // Reward + leftover must be less than 2^256 / 10^18 to avoid overflow.
-        uint balance = rewardsToken.balanceOf(address(this));
+        uint256 balance = rewardsToken.balanceOf(address(this));
         require(rewardRate <= balance / rewardsDuration, "Provided reward too high");
 
         lastUpdateTime = block.timestamp;

src/synthetix/StakingRewards.t.sol

@@ -69,6 +69,11 @@ contract StakingRewardsTest is Test {
         assertEq(f.rewardsDistribution(), address(this));
     }
 
+    function testRevertConstructorWhenStakingAndRewardsTokenAreTheSame() public {
+        vm.expectRevert("Rewards and staking tokens must not be the same");
+        new StakingRewards(address(this), address(this), address(gem), address(gem));
+    }
+
     function testSetRewardsDistribution() public {
         rewards.setRewardsDistribution(address(0));
         assertEq(rewards.rewardsDistribution(), address(0));
@@ -297,17 +302,21 @@ contract StakingRewardsTest is Test {
 
         rewards.setRewardsDuration(newDuration);
 
-        assertEq(rewards.rewardPerTokenStored(), initialRun * (totalReward / initialDuration) * WAD / totalStake);
+        assertEq(rewards.rewardPerTokenStored(), (initialRun * (totalReward / initialDuration) * WAD) / totalStake);
         assertEq(rewards.lastUpdateTime(), block.timestamp);
-        assertEq(rewards.rewardRate(), (initialDuration - initialRun) * (totalReward / initialDuration) / newDuration);
+        assertEq(
+            rewards.rewardRate(),
+            ((initialDuration - initialRun) * (totalReward / initialDuration)) / newDuration
+        );
         assertEq(rewards.periodFinish(), block.timestamp + newDuration);
         assertEq(rewards.rewardsDuration(), newDuration);
 
         skip(newDuration);
 
         rewards.exit();
 
-        uint256 rewardLeft = totalReward % initialDuration + (initialDuration - initialRun) * (totalReward / initialDuration) % newDuration; // dust lost due to rewardRate being rounded down
+        uint256 rewardLeft = (totalReward % initialDuration) +
+            (((initialDuration - initialRun) * (totalReward / initialDuration)) % newDuration); // dust lost due to rewardRate being rounded down
         uint256 rewardPaid = totalReward - rewardLeft;
         assertEq(rewardGem.balanceOf(address(this)), rewardPaid);
         assertEq(rewardGem.balanceOf(address(rewards)), rewardLeft);
@@ -336,7 +345,7 @@ contract StakingRewardsTest is Test {
 
         rewards.setRewardsDuration(initialDuration - initialRun); // same duration as time left
 
-        assertEq(rewards.rewardPerTokenStored(), initialRun * (totalReward / initialDuration) * WAD / totalStake);
+        assertEq(rewards.rewardPerTokenStored(), (initialRun * (totalReward / initialDuration) * WAD) / totalStake);
         assertEq(rewards.lastUpdateTime(), block.timestamp);
         assertEq(rewards.rewardRate(), prevRewardRate);
         assertEq(rewards.periodFinish(), prevPeriodFinish);
@@ -358,17 +367,21 @@ contract StakingRewardsTest is Test {
         rewards.setRewardsDuration(13 days);
         setupReward(additionalReward);
 
-        assertEq(rewards.rewardPerTokenStored(), initialRun * (initialReward / initialDuration) * WAD / totalStake);
+        assertEq(rewards.rewardPerTokenStored(), (initialRun * (initialReward / initialDuration) * WAD) / totalStake);
         assertEq(rewards.lastUpdateTime(), block.timestamp);
-        assertEq(rewards.rewardRate(), ((initialDuration - initialRun) * (initialReward / initialDuration) + additionalReward) / 13 days);
+        assertEq(
+            rewards.rewardRate(),
+            ((initialDuration - initialRun) * (initialReward / initialDuration) + additionalReward) / 13 days
+        );
         assertEq(rewards.periodFinish(), block.timestamp + 13 days);
         assertEq(rewards.rewardsDuration(), 13 days);
 
         skip(13 days);
 
         rewards.exit();
 
-        uint256 rewardLeft = initialReward % initialDuration + ((initialDuration - initialRun) * (initialReward / initialDuration) + additionalReward) % 13 days; // dust lost due to rewardRate being rounded down
+        uint256 rewardLeft = (initialReward % initialDuration) +
+            (((initialDuration - initialRun) * (initialReward / initialDuration) + additionalReward) % 13 days); // dust lost due to rewardRate being rounded down
         uint256 rewardPaid = initialReward + additionalReward - rewardLeft;
         assertEq(rewardGem.balanceOf(address(this)), rewardPaid);
         assertEq(rewardGem.balanceOf(address(rewards)), rewardLeft);
@@ -383,9 +396,12 @@ contract StakingRewardsTest is Test {
         rewards.setRewardsDuration(13 days);
 
         // same values as before
-        assertEq(rewards.rewardPerTokenStored(), initialRun * (initialReward / initialDuration) * WAD / totalStake);
+        assertEq(rewards.rewardPerTokenStored(), (initialRun * (initialReward / initialDuration) * WAD) / totalStake);
         assertEq(rewards.lastUpdateTime(), block.timestamp);
-        assertEq(rewards.rewardRate(), ((initialDuration - initialRun) * (initialReward / initialDuration) + additionalReward) / 13 days);
+        assertEq(
+            rewards.rewardRate(),
+            ((initialDuration - initialRun) * (initialReward / initialDuration) + additionalReward) / 13 days
+        );
         assertEq(rewards.periodFinish(), block.timestamp + 13 days);
         assertEq(rewards.rewardsDuration(), 13 days);
 
@@ -394,7 +410,9 @@ contract StakingRewardsTest is Test {
         rewards.exit();
 
         // same values as before
-        rewardLeft = initialReward % initialDuration + ((initialDuration - initialRun) * (initialReward / initialDuration) + additionalReward) % 13 days; // dust lost due to rewardRate being rounded down
+        rewardLeft =
+            (initialReward % initialDuration) +
+            (((initialDuration - initialRun) * (initialReward / initialDuration) + additionalReward) % 13 days); // dust lost due to rewardRate being rounded down
         rewardPaid = initialReward + additionalReward - rewardLeft;
         assertEq(rewardGem.balanceOf(address(this)), rewardPaid);
         assertEq(rewardGem.balanceOf(address(rewards)), rewardLeft);