-
Notifications
You must be signed in to change notification settings - Fork 2
fix: Add restrictions when depositing rewards. #17
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
base: main
Are you sure you want to change the base?
Conversation
src/Staking.sol
Outdated
| function depositReward(uint256 _amount) external nonReentrant { | ||
| rewardToken.safeTransferFrom(msg.sender, address(this), _amount); | ||
| uint256 epoch = currentEpoch(); | ||
| require(epoch > 0, "Cannot deposit reward in epoch 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.
This will break the BETH contract. what if we deposit all the BETH to the next epoch in case there is no staker in current epoch?
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.
Or imagine smth like this:
if(epochTotalLocked[epoch - 1] == 0) {
if(epochReward[epoch - 1] > 0) {
epochReward[epoch] += epochReward[epoch - 1];
epochReward[epoch - 1] = 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.
This will break the BETH contract.
I see.
Or imagine smth like this
Make sense.
But how about rewards that are deposited at epoch 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 guess this is fixed in your PR right?
| } | ||
|
|
||
| epochReward[epoch] += totalAmount; | ||
| emit RewardDeposited(msg.sender, epoch, totalAmount); |
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.
Should we report total-amount or _amount here?
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.
Yes, we can use _amount, it is clear match with what depositor actually sent.
And for the line 115:
epochReward[epoch - 1] = 0;
Should we add a separate event for accumulated rewards, like:
emit RewardAccumulated(epoch - 1, epoch, previousEpochReward);
|
There is another scenario: Rewards for Epoch n become permanently stuck because:
|
Fix #16