Closed shahthepro closed 5 months ago
@shahthepro I have added my comments regarding the changes.
The FixedRateRewardsSource works to provide a steady per block stream of incentives funds to the xOGN staking contract.
Rewards can be sent to the contract from any source (including incentives, as well as protocol buybacks), but will drip out at a rate that can be changed by the strategist.
In the event that not enough funds are in the dripper, what funds are there are given out, and the dripper is reset to start accruing max dripped funds again. In this way even if there are fewer funds than expected coming in, the contract will still hand out whatever funds it does get.
There's a potential circular address dependency between this contract and the staking contract. Both need to know the address of the other. We've taken the simple route and made this contract us a mutable address for the staking contract, while the staking contract uses an immutable address for this.
If the contract is not initialized, collect rewards will revert because the message sender is not 0, preventing anything weird from happening.
This should be deployed into a proxy.
config.lastCollected
should never move downwards. This holds. It is always set to the current timestamp, which does not move down.
config.lastCollected
should be set any time rewards are sent from the contract. It is.
We should not have resolution issues on config.ratePerSecond
, since we can pay at a rate down to trillions of a penny's worth of yield per day.
Depending on how this contract is used, it could hold up to a month or two of incentives. This could be a substantial amount reward tokens. This contract does not hold any user funds.
Attackers could attempt to steal funds. However, Funds only transfer to the admin controlled rewards target.
Attackers could attempt to increase the number of rewards paid. However rewards paid can only be increased by donating to the contract (and only then if it does not have enough rewards)
A collectRewards should be called before changing the setRewardsPerSecond, if we care about accuracy, or if it has not been called in a long time. This ensures that the past is paid at the past rate.
Contract logic looks correct .
There are a few tiny flavor changes I see, but we can leave the contract as is.
_config
, code would be shorter, and more safely changed without the local variable.RewardCollected
event is not necessarily called every time lastCollect
is changed. This makes for a possible state changing function call that does not event, and prevents perfect tracking of what the future rewards will be based purely on events. That said this is only an issue when reward rate is set for some time to zero and then again raised.No for loops
selfdestruct
delegatecall
outside of proxying_No Crypto
If you made a contract change, make sure to complete the checklist below before merging it in master.
Contract change checklist: