Add Sushi ETH/Alcx, Curve alusd-3crv strategy #45
Conversation
Adding unit test
midaseth
changed the title
- Update controller - Update strategybasesymbiotic - Update picklejarsymbiotic
| _setImplementation(_logic); | ||
| if(_data.length > 0) { | ||
| (bool success,) = _logic.delegatecall(_data); | ||
| require(success); |
There was a problem hiding this comment.
you can also bubble up the original revert reason like here:
There was a problem hiding this comment.
Hmm, actually, this contract is just copied from openzeppelin.
|
|
||
| // **** Setters **** | ||
|
|
||
| function deposit() public override { |
There was a problem hiding this comment.
Not sure if pickle has moved away from this, but the public earn interaction is something we moved away in v1 before sunsetting some of the vaults, we realized there are several problems that arise when user can force the strategy to invest, this may not be the case here depending of the state of stakingPool, but is smth that pickle may want to explore
See:
https://github.com/yearn/yearn-protocol/blob/develop/contracts/strategies/StrategyDAI3poolV2.sol
The other option is to pull funds during harvest instead of public earn investing all the way to staking pool
There was a problem hiding this comment.
well actually, calling earn function is not that important because it's called every time you deposit. So I think I can make earn function an internal function.
There was a problem hiding this comment.
@dougstorm Thanks for sharing your concern with the forced earn. We've removed the automatic earn call everytime the user deposits. It was originally there becaue we thought that accRewardPerShare would accrue unfairly to users who had deposited but where the funds were not staked yet. However, this was no different than the case of a user depositing just before a harvest, so it was really just an overoptimization.
We can also consider using harvest to simultaneously perform the function of earn as we explore moving to something closer to the v2 Vaults :)
There was a problem hiding this comment.
Sorry.... I neglected to hit "submit review" and the above message has been stuck in limbo for a week.
| /** | ||
| * @dev Collection of functions related to the address type | ||
| */ | ||
| library AddressUpgradeable { |
There was a problem hiding this comment.
Is there a reason why all the open zepellin libraries and contracts are not pulled from OZ import directly?
There was a problem hiding this comment.
It's because of the solc version mismatch.
| convenienceFee = _convenienceFee; | ||
| } | ||
|
|
||
| function setStrategy(address _token, address _strategy) public { |
There was a problem hiding this comment.
if strategist is an EOA, what happens in the case his keys are compromised? is there time to react from the pickle team in case compromised strategist calls this with a full rug strategy?
Our approach is to limit new code deployed and attached to governance role only
There was a problem hiding this comment.
Strategies can only be set where they have first been approved in approveStrategy. approveStrategy can only be called by the timelock, of which the multisig governance address is the sole owner. So there's no opportunity for the strategist to switch to a rug strategy, only a previously sanctioned strategy :)
There was a problem hiding this comment.
strategist is a multisig, not EOA. So I think it's not the case.
src/controller-v5.sol
Outdated
|
|
||
| // Function to swap between jars | ||
| function swapExactJarForJar( | ||
| address _fromJar, // From which Jar |
There was a problem hiding this comment.
I remember one of the root cause of the incident with the controller was not checking that these provided Jars are known in the Pickle Universe (validated) is that check done somewhere else?
Or how do we handle someone sending malicious jar? i don't remember the fix for that incident.
There was a problem hiding this comment.
Thanks, I will remove it.
Update ALCX/ETH strategy to accomodate dual rewards
closes #43