Leave the scale factor into the unclaimed reward calculations

src/UniStaker.sol

@@ -174,12 +174,12 @@ contract UniStaker is INotifiableRewardReceiver, Multicall, EIP712, Nonces {
  /// used to calculate the interim rewards earned by given account.
  mapping(address account => uint256) public beneficiaryRewardPerTokenCheckpoint;
 
- /// @notice Checkpoint of the unclaimed rewards earned by a given beneficiary. This value is
- /// stored any time an action is taken that specifically impacts the rate at which rewards are
- /// earned by a given beneficiary account. Total unclaimed rewards for an account are thus this
- /// value plus all rewards earned after this checkpoint was taken. This value is reset to zero
- /// when a beneficiary account claims their earned rewards.
- mapping(address account => uint256 amount) public unclaimedRewardCheckpoint;
+ /// @notice Checkpoint of the unclaimed rewards earned by a given beneficiary with the scale
+ /// factor included. This value is stored any time an action is taken that specifically impacts
+ /// the rate at which rewards are earned by a given beneficiary account. Total unclaimed rewards
+ /// for an account are thus this value plus all rewards earned after this checkpoint was taken.
+ /// This value is reset to zero when a beneficiary account claims their earned rewards.
+ mapping(address account => uint256 amount) public scaledUnclaimedRewardCheckpoint;
 
  /// @notice Maps addresses to whether they are authorized to call `notifyRewardAmount`.
  mapping(address rewardNotifier => bool) public isRewardNotifier;
@@ -237,13 +237,14 @@ contract UniStaker is INotifiableRewardReceiver, Multicall, EIP712, Nonces {
  /// sum of the last checkpoint value of their unclaimed rewards with the live calculation of the
  /// rewards that have accumulated for this account in the interim. This value can only increase,
  /// until it is reset to zero once the beneficiary account claims their unearned rewards.
+ ///
+ /// Note that the contract tracks the unclaimed rewards internally with the scale factor
+ /// included, in order to avoid the accrual of precision losses as users takes actions that
+ /// cause rewards to be checkpointed. This external helper method is useful for integrations, and
+ /// returns the value after it has been scaled down to the reward token's raw decimal amount.
  /// @return Live value of the unclaimed rewards earned by a given beneficiary account.
- function unclaimedReward(address _beneficiary) public view returns (uint256) {
- return unclaimedRewardCheckpoint[_beneficiary]
- + (
- earningPower[_beneficiary]
- * (rewardPerTokenAccumulated() - beneficiaryRewardPerTokenCheckpoint[_beneficiary])
- ) / SCALE_FACTOR;
+ function unclaimedReward(address _beneficiary) external view returns (uint256) {
+ return _scaledUnclaimedReward(_beneficiary) / SCALE_FACTOR;
  }
 
  /// @notice Stake tokens to a new deposit. The caller must pre-approve the staking contract to
@@ -598,6 +599,20 @@ contract UniStaker is INotifiableRewardReceiver, Multicall, EIP712, Nonces {
  emit RewardNotified(_amount, msg.sender);
  }
 
+ /// @notice Live value of the unclaimed rewards earned by a given beneficiary account with the
+ /// scale factor included. Used internally for calculating reward checkpoints while minimizing
+ /// precision loss.
+ /// @return Live value of the unclaimed rewards earned by a given beneficiary account with the
+ /// scale factor included.
+ /// @dev See documentation for the public, non-scaled `unclaimedReward` method for more details.
+ function _scaledUnclaimedReward(address _beneficiary) internal view returns (uint256) {
+ return scaledUnclaimedRewardCheckpoint[_beneficiary]
+ + (
+ earningPower[_beneficiary]
+ * (rewardPerTokenAccumulated() - beneficiaryRewardPerTokenCheckpoint[_beneficiary])
+ );
+ }
+
  /// @notice Internal method which finds the existing surrogate contract—or deploys a new one if
  /// none exists—for a given delegatee.
  /// @param _delegatee Account for which a surrogate is sought.
@@ -741,9 +756,9 @@ contract UniStaker is INotifiableRewardReceiver, Multicall, EIP712, Nonces {
  _checkpointGlobalReward();
  _checkpointReward(_beneficiary);
 
- uint256 _reward = unclaimedRewardCheckpoint[_beneficiary];
+ uint256 _reward = scaledUnclaimedRewardCheckpoint[_beneficiary] / SCALE_FACTOR;
  if (_reward == 0) return;
- unclaimedRewardCheckpoint[_beneficiary] = 0;
+ scaledUnclaimedRewardCheckpoint[_beneficiary] = 0;
  emit RewardClaimed(_beneficiary, _reward);
 
  SafeERC20.safeTransfer(REWARD_TOKEN, _beneficiary, _reward);
@@ -762,7 +777,7 @@ contract UniStaker is INotifiableRewardReceiver, Multicall, EIP712, Nonces {
  /// accumulator has been checkpointed. It assumes the global `rewardPerTokenCheckpoint` is up to
  /// date.
  function _checkpointReward(address _beneficiary) internal {
- unclaimedRewardCheckpoint[_beneficiary] = unclaimedReward(_beneficiary);
+ scaledUnclaimedRewardCheckpoint[_beneficiary] = _scaledUnclaimedReward(_beneficiary);
  beneficiaryRewardPerTokenCheckpoint[_beneficiary] = rewardPerTokenAccumulatedCheckpoint;
  }
 

test/UniStaker.integration.t.sol

@@ -265,7 +265,7 @@ contract Stake is IntegrationTest, PercentAssertions {
  ) public {
  vm.assume(_depositor != address(0) && _delegatee != address(0) && _amount != 0);
  // Make sure depositor is not UniStaker
- vm.assume(_depositor != 0xE2307e3710d108ceC7a4722a020a050681c835b3);
+ vm.assume(_depositor != address(uniStaker));
  _passQueueAndExecuteProposals();
  _swapAndClaimFees(_swapAmount);
  _amount = _dealStakingToken(_depositor, _amount);

test/UniStaker.t.sol

@@ -2679,8 +2679,8 @@ contract UniStakerRewardsTest is UniStakerTest {
  console2.log(uniStaker.earningPower(_depositor));
  console2.log("beneficiaryRewardPerTokenCheckpoint[_depositor]");
  console2.log(uniStaker.beneficiaryRewardPerTokenCheckpoint(_depositor));
- console2.log("unclaimedRewardCheckpoint[_depositor]");
- console2.log(uniStaker.unclaimedRewardCheckpoint(_depositor));
+ console2.log("scaledUnclaimedRewardCheckpoint[_depositor]");
+ console2.log(uniStaker.scaledUnclaimedRewardCheckpoint(_depositor));
  console2.log("unclaimedReward(_depositor)");
  console2.log(uniStaker.unclaimedReward(_depositor));
  console2.log("-----------------------------------------------");
@@ -4464,6 +4464,49 @@ contract UnclaimedReward is UniStakerRewardsTest {
  // Rewards earned by depositors should always at most equal to the actual reward amount
  assertLteWithinOnePercent(_earned1 + _earned2, _rewardAmount);
  }
+
+ function test_CalculatesEarningsInAWayThatMitigatesRewardGriefing() public {
+ address _depositor1 = makeAddr("Depositor 1");
+ address _depositor2 = makeAddr("Depositor 2");
+ address _depositor3 = makeAddr("Depositor 3");
+ address _delegatee = makeAddr("Delegatee");
+ address _attacker = makeAddr("Attacker");
+
+ uint256 _smallDepositAmount = 0.1e18;
+ uint256 _largeDepositAmount = 25_000_000e18;
+ _mintGovToken(_depositor1, _smallDepositAmount);
+ _mintGovToken(_depositor2, _smallDepositAmount);
+ _mintGovToken(_depositor3, _largeDepositAmount);
+ uint256 _rewardAmount = 1e14;
+ rewardToken.mint(rewardNotifier, _rewardAmount);
+
+ // The contract is notified of a reward
+ vm.startPrank(rewardNotifier);
+ rewardToken.transfer(address(uniStaker), _rewardAmount);
+ uniStaker.notifyRewardAmount(_rewardAmount);
+ vm.stopPrank();
+
+ // User deposit staking tokens
+ _stake(_depositor1, _smallDepositAmount, _delegatee);
+ _stake(_depositor2, _smallDepositAmount, _delegatee);
+ _stake(_depositor3, _largeDepositAmount, _delegatee);
+
+ // Every block _attacker deposits 0 stake and assigns _depositor1 as beneficiary, thus leading
+ // to frequent updates of the reward checkpoint for _depositor1, during which rounding errors
+ // could accrue.
+ UniStaker.DepositIdentifier _depositId = _stake(_attacker, 0, _delegatee, _depositor1);
+ for (uint256 i = 0; i < 1000; ++i) {
+ _jumpAhead(12);
+ vm.prank(_attacker);
+ uniStaker.stakeMore(_depositId, 0);
+ }
+
+ // Despite the attempted griefing attack, the unclaimed rewards for the two depositors should
+ // be ~the same.
+ assertLteWithinOnePercent(
+ uniStaker.unclaimedReward(_depositor1), uniStaker.unclaimedReward(_depositor2)
+ );
+ }
 }
 
 contract ClaimReward is UniStakerRewardsTest {

test/helpers/UniStaker.handler.sol

@@ -157,7 +157,8 @@ contract UniStakerHandler is CommonBase, StdCheats, StdUtils {
  function claimReward(uint256 _actorSeed) public countCall("claimReward") doCheckpoints {
  _useActor(_beneficiaries, _actorSeed);
  vm.startPrank(_currentActor);
- uint256 rewardsClaimed = uniStaker.unclaimedRewardCheckpoint(_currentActor);
+ uint256 rewardsClaimed =
+ uniStaker.scaledUnclaimedRewardCheckpoint(_currentActor) / uniStaker.SCALE_FACTOR();
  uniStaker.claimReward();
  vm.stopPrank();
  ghost_rewardsClaimed += rewardsClaimed;