-
Notifications
You must be signed in to change notification settings - Fork 7
Feature/reward distributor #152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
/// @notice The maximum value to which the claim fee can be set. | ||
/// @dev For anything other than a zero value, this immutable parameter should be set in the | ||
/// constructor of a concrete implementation inheriting from Staker. | ||
uint256 public immutable MAX_CLAIM_FEE; |
Check failure
Code scanning / Slither
Uninitialized state variables High
- RewardDistributor._setClaimFeeParameters(RewardDistributor.ClaimFeeParameters)
function lastTimeRewardDistributed() public view virtual returns (uint256) { | ||
if (rewardEndTime <= block.timestamp) return rewardEndTime; | ||
else return block.timestamp; | ||
} |
Check notice
Code scanning / Slither
Block timestamp Low
Dangerous comparisons:
- rewardEndTime <= block.timestamp
function _claimReward( | ||
DepositIdentifier _depositId, | ||
DelegateReward storage deposit, | ||
address _claimer | ||
) internal virtual returns (uint256) { | ||
_checkpointGlobalReward(); | ||
_checkpointReward(deposit); | ||
|
||
uint256 _reward = deposit.scaledUnclaimedRewardCheckpoint / SCALE_FACTOR; | ||
// Intentionally reverts due to overflow if unclaimed rewards are less than fee. | ||
uint256 _payout = _reward - claimFeeParameters.feeAmount; | ||
if (_payout == 0) return 0; | ||
|
||
// retain sub-wei dust that would be left due to the precision loss | ||
deposit.scaledUnclaimedRewardCheckpoint = | ||
deposit.scaledUnclaimedRewardCheckpoint - (_reward * SCALE_FACTOR); | ||
|
||
uint256 _newEarningPower = | ||
earningPowerCalculator.getEarningPower(0, deposit.owner, deposit.owner); | ||
|
||
emit RewardClaimed(_depositId, _claimer, _payout, _newEarningPower); | ||
|
||
totalEarningPower = | ||
_calculateTotalEarningPower(deposit.earningPower, _newEarningPower, totalEarningPower); | ||
depositorTotalEarningPower[deposit.owner] = _calculateTotalEarningPower( | ||
deposit.earningPower, _newEarningPower, depositorTotalEarningPower[deposit.owner] | ||
); | ||
deposit.earningPower = _newEarningPower.toUint96(); | ||
|
||
SafeERC20.safeTransfer(REWARD_TOKEN, _claimer, _payout); | ||
if (claimFeeParameters.feeAmount > 0) { | ||
SafeERC20.safeTransfer( | ||
REWARD_TOKEN, claimFeeParameters.feeCollector, claimFeeParameters.feeAmount | ||
); | ||
} | ||
return _payout; | ||
} |
Check warning
Code scanning / Slither
Divide before multiply Medium
- _reward = deposit.scaledUnclaimedRewardCheckpoint / SCALE_FACTOR
- deposit.scaledUnclaimedRewardCheckpoint = deposit.scaledUnclaimedRewardCheckpoint - (_reward * SCALE_FACTOR)
function notifyRewardAmount(uint256 _amount) external virtual { | ||
if (!isRewardNotifier[msg.sender]) revert Staker__Unauthorized("not notifier", msg.sender); | ||
|
||
// We checkpoint the accumulator without updating the timestamp at which it was updated, | ||
// because that second operation will be done after updating the reward rate. | ||
rewardPerTokenAccumulatedCheckpoint = rewardPerTokenAccumulated(); | ||
|
||
if (block.timestamp >= rewardEndTime) { | ||
scaledRewardRate = (_amount * SCALE_FACTOR) / REWARD_DURATION; | ||
} else { | ||
uint256 _remainingReward = scaledRewardRate * (rewardEndTime - block.timestamp); | ||
scaledRewardRate = (_remainingReward + _amount * SCALE_FACTOR) / REWARD_DURATION; | ||
} | ||
|
||
rewardEndTime = block.timestamp + REWARD_DURATION; | ||
lastCheckpointTime = block.timestamp; | ||
|
||
if ((scaledRewardRate / SCALE_FACTOR) == 0) revert Staker__InvalidRewardRate(); | ||
|
||
// This check cannot _guarantee_ sufficient rewards have been transferred to the contract, | ||
// because it cannot isolate the unclaimed rewards owed to stakers left in the balance. While | ||
// this check is useful for preventing degenerate cases, it is not sufficient. Therefore, it is | ||
// critical that only safe reward notifier contracts are approved to call this method by the | ||
// admin. | ||
if ( | ||
(scaledRewardRate * REWARD_DURATION) > (REWARD_TOKEN.balanceOf(address(this)) * SCALE_FACTOR) | ||
) revert Staker__InsufficientRewardBalance(); | ||
|
||
emit RewardNotified(_amount, msg.sender); | ||
} |
Check warning
Code scanning / Slither
Divide before multiply Medium
- scaledRewardRate = (_remainingReward + _amount * SCALE_FACTOR) / REWARD_DURATION
- (scaledRewardRate * REWARD_DURATION) > (REWARD_TOKEN.balanceOf(address(this)) * SCALE_FACTOR)
function getEarningPower(uint256, /* _amountStaked */ address, /* _staker */ address _delegatee) | ||
external | ||
view | ||
virtual | ||
override | ||
returns (uint256) | ||
{ | ||
uint48 _votingPowerTimepoint = _getVotingPowerTimepoint(); | ||
uint256 _votingPower = | ||
IVotes(VOTING_POWER_TOKEN).getPastVotes(_delegatee, _votingPowerTimepoint); | ||
|
||
if (_isOracleStale() || isOraclePaused) return _votingPower; | ||
return _isDelegateeEligible(_delegatee) ? _votingPower : 0; | ||
} |
Check notice
Code scanning / Slither
Block timestamp Low
Dangerous comparisons:
- _isOracleStale() || eligibilityModule.isOraclePaused()
function getNewEarningPower( | ||
uint256, /* _amountStaked */ | ||
address, /* _staker */ | ||
address _delegatee, | ||
uint256 /* _oldEarningPower */ | ||
) external view virtual override returns (uint256, bool) { | ||
uint48 _votingPowerTimepoint = _getVotingPowerTimepoint(); | ||
uint256 _votingPower = | ||
IVotes(VOTING_POWER_TOKEN).getPastVotes(_delegatee, _votingPowerTimepoint); | ||
|
||
// TODO: Do we want the same fallback behavior. | ||
// Should we instead accept the stale values if paused? | ||
if (_isOracleStale() || isOraclePaused) return (_votingPower, true); | ||
|
||
if (!_isDelegateeEligible(_delegatee)) { | ||
bool _isUpdateDelayElapsed = | ||
(timeOfIneligibility[_delegatee] + updateEligibilityDelay) <= block.timestamp; | ||
return (0, _isUpdateDelayElapsed); | ||
} | ||
|
||
return (_votingPower, true); | ||
} |
Check notice
Code scanning / Slither
Block timestamp Low
Dangerous comparisons:
- _isOracleStale() || eligibilityModule.isOraclePaused()
uint256 _votingPower = | ||
IVotes(VOTING_POWER_TOKEN).getPastVotes(_delegatee, _votingPowerTimepoint); | ||
|
||
if (_isOracleStale() || isOraclePaused) return _votingPower; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep this but read other epc, from staker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the address we'd need to look at, the address which represents the delegate in this case, is the one passed as the _staker
to the EPC, i.e. the owner of the "deposit" in the DelegateCompensationStaker, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good question. I updated the owner and delegate to be the same address for both spikes so it shouldn't matter, but it may matter for future implementations. Which would be better? Happy to choose either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make the EPC methods public on BinaryEligibilityEPC, add to existing pr, add tests for new public methods
function _fetchOrDeploySurrogate(address) internal pure override returns (DelegationSurrogate) { | ||
revert(); | ||
} |
Check warning
Code scanning / Slither
Dead-code Warning
src/DelegateCompensationStaker.sol
Outdated
depositorTotalEarningPower[_delegate] += _earningPower; | ||
deposits[_depositId] = Deposit({ | ||
balance: 0, | ||
delegatee: address(0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think delegating to address(0) would revert in Staker. The easiest way to handle it is probably just to pick a non-zero, non "real" address to set as the delegate for every "deposit"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this will revert unless we have to create surrogates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, I think it is probably better to make this the provided delegate address to avoid issues in the earning power calculator
return _depositId; | ||
} | ||
|
||
function _fetchOrDeploySurrogate(address) internal pure override returns (DelegationSurrogate) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we want this to revert? Wouldn't we need to do this once for whatever the fake "delegatee" is for every deposti?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I am missing something, do we still have the concept of a delegatee for deposit? I was under the impression delegates were simply accumulating rewards and not delegating them. I think we can revert if my understanding is correct as this is only used in _stake
and _alterDelegatee
which we no longer use.
uint48 public votingPowerUpdateFrequency; | ||
uint48 public immutable UPDATE_START_TIME; | ||
address public immutable VOTING_POWER_TOKEN; | ||
BinaryEligibilityOracleEarningPowerCalculator public eligibilityModule; |
Check warning
Code scanning / Slither
State variables that could be declared immutable Warning
BinaryEligibilityOracleEarningPowerCalculator public eligibilityModule; | ||
|
||
constructor( | ||
address _owner, |
Check notice
Code scanning / Slither
Local variable shadowing Low
- Ownable._owner (state variable)
constructor( | ||
address _owner, | ||
address _eligibilityAddress, | ||
address _votingPowerToken, |
Check notice
Code scanning / Slither
Missing zero address validation Low
function _isOracleStale() internal view returns (bool) { | ||
return block.timestamp - eligibilityModule.lastOracleUpdateTime() | ||
> eligibilityModule.STALE_ORACLE_WINDOW(); | ||
} |
Check notice
Code scanning / Slither
Block timestamp Low
Dangerous comparisons:
- block.timestamp - eligibilityModule.lastOracleUpdateTime() > eligibilityModule.STALE_ORACLE_WINDOW()
VOTING_POWER_TOKEN = _votingPowerToken; | ||
} | ||
|
||
function getEarningPower(uint256, /* _amountStaked */ address, /* _staker */ address _delegatee) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's switch to use _staker
returns (uint256) | ||
{ | ||
uint48 _votingPowerTimepoint = _getVotingPowerTimepoint(); | ||
uint256 _votingPower = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switch to square root calculation
import {DelegationSurrogate} from "./DelegationSurrogate.sol"; | ||
import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol"; | ||
|
||
abstract contract DelegateCompensationStaker is Staker { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing strategy
- Use fakes for other EPC, and token
- Add some integration post phase 1
Staker
- Simple smoke stakes, and other methods revert. Where there is smoke there is fire testing.
- Minimum, tests that demonstrate the functionality is left still works and interacts properly with the new changes i.e. reward distribution and claiming.
- At least one test for every public methods to make sure the happy is working.
No description provided.