diff --git a/src/UniStaker.sol b/src/UniStaker.sol index babdc1a..689b335 100644 --- a/src/UniStaker.sol +++ b/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; } diff --git a/test/UniStaker.integration.t.sol b/test/UniStaker.integration.t.sol index 4c0cee8..73f7b6f 100644 --- a/test/UniStaker.integration.t.sol +++ b/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); diff --git a/test/UniStaker.t.sol b/test/UniStaker.t.sol index 23e427e..c661a51 100644 --- a/test/UniStaker.t.sol +++ b/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 { diff --git a/test/helpers/UniStaker.handler.sol b/test/helpers/UniStaker.handler.sol index f8fe335..e4f03b5 100644 --- a/test/helpers/UniStaker.handler.sol +++ b/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;