code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Function _withdraw() can be called from an approved caller to withdraw owner funds. The function accrues rewards for caller and receiver but misses the accrual for owner.

If, for example, the owner didn't accrue any reward from the beginning of time and all staking tokens are withdrawn from the owner by the caller, then the owner will be left with zero shares and his supplierDelta will be calculated as zero. This means that the owner may lose all rewards regardless of how long their stake was active.

Proof of Concept

Put the following test in ./test/ folder and run with forge test --mc MissedAccrueTest. The test fails because Alice's rewards were not accrued:

// 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";

contract MissedAccrueTest 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);

  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
      1e10, 

      // amount
      1e18, 

      // useEscrow
      false, 

      // escrowPercentage
      0, 

      // escrowDuration
      0, 

      // offset
      0
    );

  }

  function testMissedAccrual() public {
    assert (stakingToken.balanceOf(bob) == 0);
    assert (stakingToken.balanceOf(alice) == 0);

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

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

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

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

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

    vm.prank(alice);
    staking.approve(bob, 1);

    assert (staking.balanceOf(bob) == 1);
    assert (staking.balanceOf(alice) == 1);

    vm.warp(block.timestamp + 1);

    vm.prank(bob);
    staking.withdraw(1, bob, alice);

    IERC20[] memory a = new IERC20[](1);
    a[0] = IERC20(address(rewardToken));

    staking.claimRewards(bob, a);
    assert (stakingToken.balanceOf(bob) > 0);

    // NOW THIS INCORRECTLY FAILS BECAUSE ALICE ACCRUAL WAS MISSED 
    staking.claimRewards(alice, a);
    assert (stakingToken.balanceOf(alice) > 0);
  }
}

Tools Used

Manual analysis

Recommended Mitigation Steps

Accrue owner in _withdraw() function.

c4-judge commented 1 year ago

dmvt marked the issue as duplicate of #385

c4-sponsor commented 1 year ago

RedVeil marked the issue as sponsor confirmed

c4-sponsor commented 1 year ago

RedVeil marked the issue as disagree with severity