Incorrect computation in MultiRewardStaking `changeRewardSpeed()` leads to loss of rewards

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L308 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L357

Vulnerability details

Impact

The changeRewardSpeed() function computes rewardsEndTimestamp incorrectly for the case block.timestamp < prevEndTime. For example, it may increase the rewardsEndTimestamp by several years forward despite the fact that the fund will be enough for one day only. As a result, some users will claim extra rewards at the expense of other users.

Proof of Concept

The function changeRewardSpeed() calculates rewardsEndTimestamp via

_calcRewardsEnd(
    prevEndTime > block.timestamp ? prevEndTime : block.timestamp.safeCastTo32(),
    rewardsPerSecond,
    remainder
)

If the prevEndTime > block.timestamp then it can be reduced to _calcRewardsEnd(prevEndTime, rewardsPerSecond, remainder) where the remainder is the contract's reward token balance.

Then, in the _calcRewardsEnd() the first condition is met and the amount value is increased from remainder to remainder + rewardsPerSecond * (timeLeft):

if (rewardsEndTimestamp > block.timestamp)
    amount += uint256(rewardsPerSecond) * (rewardsEndTimestamp - block.timestamp);

Thus, the amount becomes larger the original contract's reward token balance and the new rewardsEndTimestamp is calculated incorrectly:

return (block.timestamp + (amount / uint256(rewardsPerSecond))).safeCastTo32();

You can use the following forge test to check for the error. Put the following test in ./test/ folder and run with forge test --mc IncorrectComputation. The test fails because rewardsEndTimestamp calculations are wrong:

// SPDX-License-Identifier: GPL-3.0
// Docgen-SOLC: 0.8.15

pragma solidity ^0.8.15;

import { Test } from "forge-std/Test.sol";
import { SafeCastLib } from "solmate/utils/SafeCastLib.sol";
import { MockERC20 } from "./utils/mocks/MockERC20.sol";
import { IMultiRewardEscrow } from "../src/interfaces/IMultiRewardEscrow.sol";
import { MultiRewardStaking, IERC20 } from "../src/utils/MultiRewardStaking.sol";
import { MultiRewardEscrow } from "../src/utils/MultiRewardEscrow.sol";
import { RewardInfo, EscrowInfo } from "../src/interfaces/IMultiRewardStaking.sol";

contract IncorrectComputation is Test {
  using SafeCastLib for uint256;

  MockERC20 stakingToken;
  MockERC20 rewardToken;
  MultiRewardStaking staking;
  MultiRewardEscrow escrow;

  address alice = address(0xABCD);
  address bob = address(0xDCBA);
  address feeRecipient = address(0x9999);

  uint160 constant rps = 1 ether;
  uint constant funds = 100 ether;

  function setUp() public {
    vm.label(alice, "alice");
    vm.label(bob, "bob");

    stakingToken = new MockERC20("Staking Token", "STKN", 18);

    rewardToken = new MockERC20("RewardsToken1", "RTKN1", 18);

    escrow = new MultiRewardEscrow(address(this), feeRecipient);

    staking = new MultiRewardStaking();
    staking.initialize(IERC20(address(stakingToken)), IMultiRewardEscrow(address(escrow)), address(this));

    rewardToken.mint(address(this), 1000 ether);
    rewardToken.approve(address(staking), 1000 ether);

    staking.addRewardToken(
      // rewardToken
      IERC20(address(rewardToken)), 

      // rewardsPerSecond
      rps,

      // amount
      funds, 

      // useEscrow
      false, 

      // escrowPercentage
      0, 

      // escrowDuration
      0, 

      // offset
      0
    );

  }

  function testIncorrectComputation() public {

    stakingToken.mint(address(bob), 1);

    vm.prank(bob);
    stakingToken.approve(address(staking), 1);

    vm.prank(bob);
    staking.deposit(1);

    (
      /*uint64 ONE1*/,
      /*uint160 rewardsPerSecond1*/,
      uint32 rewardsEndTimestamp1,
      /*uint224 index1*/,
      /*uint32 lastUpdatedTimestamp1*/
    ) = staking.rewardInfos(IERC20(address(rewardToken)));

    assert (rewardsEndTimestamp1 == block.timestamp + funds / rps);

    // now we change the speed to the same number
    // so it shouldn't change anything
    // (but it incorrectly does)
    staking.changeRewardSpeed(IERC20(address(rewardToken)), rps);

    (
      /*uint64 ONE2*/,
      /*uint160 rewardsPerSecond2*/,
      uint32 rewardsEndTimestamp2,
      /*uint224 index2*/,
      /*uint32 lastUpdatedTimestamp2*/
    ) = staking.rewardInfos(IERC20(address(rewardToken)));

    // THIS WILL INCORRECTLY REVERT BECAUSE COMPUTATIONS IN changeRewardSpeed() are wrong!
    assert (rewardsEndTimestamp2 == rewardsEndTimestamp1);
  }
}

Tools Used

Manual analysis

Recommended Mitigation Steps

Fix rewardsEndTimestamp calculations

[c4-judge]

dmvt marked the issue as duplicate of #156

[c4-sponsor]

RedVeil marked the issue as disagree with severity

[c4-sponsor]

RedVeil marked the issue as sponsor confirmed

[c4-judge]

dmvt changed the severity to QA (Quality Assurance)

[Simon-Busch]

Revert back to M as requested by @dmvt

[c4-judge]

dmvt marked the issue as satisfactory

[c4-judge]

dmvt marked the issue as partial-50