Add Simplified Harvester #2222
Add Simplified Harvester #2222
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2222 +/- ##
==========================================
- Coverage 54.32% 53.50% -0.83%
==========================================
Files 78 79 +1
Lines 4009 4071 +62
Branches 1047 816 -231
==========================================
Hits 2178 2178
- Misses 1828 1890 +62
Partials 3 3 ☔ View full report in Codecov by Sentry. |
Add Simplified Harvester
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
| * sends it to the Strategist multisig. | ||
| * Anyone can call it. | ||
| */ | ||
| function harvest() external { |
There was a problem hiding this comment.
Minor thing - anyone can call harvest.
If we are trying to operate by using harvestAndSwap(), harvest() being before called would cancel the swap or remove most of the areo from being swappable. Over any given time range we'd probably be operating either in auto harvest() mode, or in manual harvestAndSwap() mode, but overlapping both calls.
It's probably not worth adding any complications around locking out harvest though.
There was a problem hiding this comment.
Yeah, that's a good point. I was thinking if we need to schedule harvest on Defender, we need this method to be publicly callable.
Also, if the contract doesn't have enough AERO for the swap, the strategist can do transfer to this contract in the same multisig tx (and it will swap & refund any leftover AERO back to the strategist)
There was a problem hiding this comment.
I would argue that it is worth adding a "harvestEnabled" boolean settable by the strategist. This way we can disable the public callable harvest function when required. I know it is additional complexity - which I also don't like.
But just 1 malicious actor is required and they can disable the convenient harvestAndSwap function for us (by calling harvest()) and we need to perform swapping and distributing the fees manually. Granted there is little incentive for anyone to be doing such a thing.
There was a problem hiding this comment.
That's a valid concern. However I don't think calling harvest() before we do a harvestAndSwap disables us from using that function. We can always transfer AERO to that contract in the same tx as harvestAndSwap to use it.
I'm trying to avoid any sort of storage variables to not mess up the storage layout when we start inheriting AbstractHarvester in future. One alternative I can think of is:
- in
harvestAndSwap, when there isn't enough funds in the contracts, it can do atransferFrom(strategist, address(this))for the difference. This means that strategist has to approve the Harvester contract to move AERO on its behalf but that should be fine I guess.
What do you think?
There was a problem hiding this comment.
I have went ahead made the change to transferFrom, let me know if you have any further comments/suggestion on this one
There was a problem hiding this comment.
I just made the harvest() method permissioned in addition to the above change. This concern should no longer be there
| * sends it to the Strategist multisig. | ||
| * Anyone can call it. | ||
| */ | ||
| function harvest() external { |
There was a problem hiding this comment.
I would argue that it is worth adding a "harvestEnabled" boolean settable by the strategist. This way we can disable the public callable harvest function when required. I know it is additional complexity - which I also don't like.
But just 1 malicious actor is required and they can disable the convenient harvestAndSwap function for us (by calling harvest()) and we need to perform swapping and distributing the fees manually. Granted there is little incentive for anyone to be doing such a thing.
| if (aeroBalance < aeroToSwap) { | ||
| // Transfer in balance from the multisig as needed | ||
| // slither-disable-next-line unchecked-transfer arbitrary-send-erc20 | ||
| aero.safeTransferFrom( |
There was a problem hiding this comment.
I understand why you added this, but this seems a bit over-engineering to me. I would rather completely remove the public harvest() and keep only the ones that have the restricted call, to prevent front-running.
However, as I don't know what the project requirements are and this doesn't seem to be unsafe, I have no problem keeping it like this.
Has this been considered to create a new role, only for simple harvest, and grant this role to a OZ Relayer?
There was a problem hiding this comment.
Has this been considered to create a new role, only for simple harvest, and grant this role to a OZ Relayer?
Yep, that's an option. However, I didn't wanna complicate the storage layout. Sooner or later, we want the harvester to inherit from AbstractHarvester like on mainnet.
Thinking of it, I guess one option is to completely discard this contract and proxy when we plan to make that transition instead of trying to upgrade the implementation at that time
There was a problem hiding this comment.
This seems a good idea indeed!
shahthepro
dismissed stale reviews from sparrowDom and clement-ux
via
b6e40aa
* [OETHb] Deploy 015 - Harvester * Update slither DB
Code Change Checklist
To be completed before internal review begins:
Internal review:
Deploy checklist
Two reviewers complete the following checklist: