Idle-Finance / yearn-tranche-strategy

Idle PYT Yearn-strategy
GNU Affero General Public License v3.0
0 stars 0 forks source link

Idle Tranche Strategy #3

Open vany365 opened 2 years ago

vany365 commented 2 years ago

Description: Idle Perpetual Yield Tranches Strategy for Yearn

Commit (hash):

Tag:

Repo: https://github.com/Idle-Finance/yearn-tranche-strategy

Scope: (List link to files)

Due Diligence: (attach link to due diligence document, see template here)

Additional References or Repo (Link to underlying protocols or relevant documents):

Deployed Contract (etherscan link):

Review Ongoing By:

Review Completed By:

massun-onibakuchi commented 2 years ago

Estimated APY, taking into account the effect of auto-compounding interest and harvests

Junior Tranche

some of apy are incorrect.

Screen Shot 2022-04-21 at 15 31 12

Senior Tranche

Screen Shot 2022-04-21 at 15 32 51
dudesahn commented 2 years ago

Spamming all mainnet strategies in review—please please consider checking ethereum baseFee prices for harvestTrigger similar to what I do here; and just in general planning around automating as much as you can with harvestTrigger:

    // check if the current baseFee is below our external target
    function isBaseFeeAcceptable() internal view returns (bool) {
        return
            IBaseFee(0xb5e1CAcB567d98faaDB60a1fD4820720141f064F)
                .isCurrentBaseFeeAcceptable();
    }

Happy to discuss if you have any questions 🙂

saltyfacu commented 2 years ago

@wavey and @tonkers will help here :)

decaf-addict commented 1 year ago

already in BaseStrategy, can be removed https://github.com/Idle-Finance/yearn-tranche-strategy/blob/beb107efaa0f8c6090c102fa74e8878def9b51e3/contracts/Strategy.sol#L33-L34

decaf-addict commented 1 year ago

remove, not used https://github.com/Idle-Finance/yearn-tranche-strategy/blob/beb107efaa0f8c6090c102fa74e8878def9b51e3/contracts/Strategy.sol#L103 https://github.com/Idle-Finance/yearn-tranche-strategy/blob/beb107efaa0f8c6090c102fa74e8878def9b51e3/contracts/Strategy.sol#L116

decaf-addict commented 1 year ago

opinion: leave this blank to allow gov to sweep any token

https://github.com/Idle-Finance/yearn-tranche-strategy/blob/beb107efaa0f8c6090c102fa74e8878def9b51e3/contracts/Strategy.sol#L526-L544

massun-onibakuchi commented 1 year ago

@tonkers-kuma

remove, not used

https://github.com/Idle-Finance/yearn-tranche-strategy/blob/beb107efaa0f8c6090c102fa74e8878def9b51e3/contracts/Strategy.sol#L103

https://github.com/Idle-Finance/yearn-tranche-strategy/blob/beb107efaa0f8c6090c102fa74e8878def9b51e3/contracts/Strategy.sol#L116

router is used to fetch asset price in ethToWant()

massun-onibakuchi commented 1 year ago

23996c2fbc3bf1cf5f0a928c2a2a788f9695360d

decaf-addict commented 1 year ago

Grouping all these comments together bc I'm reviewing on the plane w/o internet.

General: is the idea of this strategy to deploy a set of 2 contracts (junior + senior) together and have yearn manage the weight distribution? Do you have any tools better inform us on what to adjust to? What's the benefit of this tranche strategy if the depositor has to manage the balance of the weights themselves?

General: What kind of operation are the tranches doing under the hood to create yield? What's the risk profile in term of strategy and also exposure to other protocols?

can leave blank, use gas oracle pattern instead. https://github.com/Idle-Finance/yearn-tranche-strategy/blob/beb107efaa0f8c6090c102fa74e8878def9b51e3/contracts/Strategy.sol#L561-L586

remove: https://github.com/Idle-Finance/yearn-tranche-strategy/blob/beb107efaa0f8c6090c102fa74e8878def9b51e3/contracts/Strategy.sol#L752

use safemath https://github.com/Idle-Finance/yearn-tranche-strategy/blob/beb107efaa0f8c6090c102fa74e8878def9b51e3/contracts/Strategy.sol#L662

is the tranche token interest bearing? What about the gauge token? Looks like the amount could be of either type of token. Just making sure that there doesn't need to be a conversion here https://github.com/Idle-Finance/yearn-tranche-strategy/blob/beb107efaa0f8c6090c102fa74e8878def9b51e3/contracts/Strategy.sol#L642

could be wise to also build in some defensive codes here like Math.max(toWithdraw, stakedBal) https://github.com/Idle-Finance/yearn-tranche-strategy/blob/beb107efaa0f8c6090c102fa74e8878def9b51e3/contracts/Strategy.sol#L646

Will this revert if _trancheAmount isn't available to withdraw? https://github.com/Idle-Finance/yearn-tranche-strategy/blob/beb107efaa0f8c6090c102fa74e8878def9b51e3/contracts/Strategy.sol#L651 If yes, add check for max safe withdrawable amount https://github.com/Idle-Finance/yearn-tranche-strategy/blob/beb107efaa0f8c6090c102fa74e8878def9b51e3/contracts/Strategy.sol#L698

nit: inconsistent use of _ in several methods https://github.com/Idle-Finance/yearn-tranche-strategy/blob/beb107efaa0f8c6090c102fa74e8878def9b51e3/contracts/Strategy.sol#L708

stakedBal.add() not needed https://github.com/Idle-Finance/yearn-tranche-strategy/blob/beb107efaa0f8c6090c102fa74e8878def9b51e3/contracts/Strategy.sol#L340

Specify junior or senior https://github.com/Idle-Finance/yearn-tranche-strategy/blob/beb107efaa0f8c6090c102fa74e8878def9b51e3/contracts/Strategy.sol#L290

don't leave reward tokens and idle tokens behind https://github.com/Idle-Finance/yearn-tranche-strategy/blob/beb107efaa0f8c6090c102fa74e8878def9b51e3/contracts/Strategy.sol#L485

bugduino commented 1 year ago

Thanks for the review @tonkers-kuma. For the General part: You should rather select only one of the 2 tranches, according to your risk profile and that's it, Idle automatically 'split' the interest and redirects it to the 2 tranches according to the, dynamic, APR split ratio. Of course potentially one can also deposit in both tranches, using 2 instances of this strategy, and manage the ratio between the 2 according to any logic.

Funds deposited via tranches will get then redeposited in one of the lending provider that we support (currently Lido, Convex, Euler, Clearpool) and rewards harvested and reinvested for the underlying. All senior tranches, approved by the governance via snapshots, can also get some IDLE via Idle Gauges. Some tranches, may have also additional rewards distributed, via gauges Multireward contracts, eg for Lido tranche we distribute LDO proportionally to the TVL. Of course in case of hacks or any loss, senior holders will be the first to receive funds.

All tranche tokens are interest bearing tokens. Senior tranches deposited in a gauge will give back to the user a gauge-deposit token, we use LiquidityGaugeV3 from Curve. If the gauge also has rewards, like for Lido senior tranche, then the tranche token deposited in the gauge will start getting also these additional rewards automatically

(Btw the dynamic split, vs a fixed split ratio, will be the default for all tranches on our side in the next few days)

For the implementation comments I'll let @massun-onibakuchi add some more info

massun-onibakuchi commented 1 year ago

use gas oracle pattern instead.

what's gas oracle pattern?

massun-onibakuchi commented 1 year ago

Specify junior or senior

https://github.com/Idle-Finance/yearn-tranche-strategy/blob/beb107efaa0f8c6090c102fa74e8878def9b51e3/contracts/Strategy.sol#L290

tranche.name() returns somthinIdleCDO AA Tranche - wstETH (AA_wstETH) ,which contains Junior or Senior. AA Tranche means ~Junior tranche~ senior one.

massun-onibakuchi commented 1 year ago

Will this revert if _trancheAmount isn't available to withdraw? yearn-tranche-strategy/contracts/Strategy.sol](https://github.com/Idle-Finance/yearn-tranche-strategy/blob/beb107efaa0f8c6090c102fa74e8878def9b51e3/contracts/Strategy.sol#L651

https://github.com/Idle-Finance/yearn-tranche-strategy/commit/beb107efaa0f8c6090c102fa74e8878def9b51e3

If yes, add check for max safe withdrawable amount

no. _withdrawTranche() will not revert unless the strategy try to withdraw amount greater than actual balance

massun-onibakuchi commented 1 year ago

is the tranche token interest bearing? What about the gauge token? Looks like the amount could be of either type of token. Just making sure that there doesn't need to be a conversion here

tranche token is interest bearing token. same amount of gauge tokens are minted/ burned when users depositing/withdrawing their tranche to/from Gauge. a conversion rate between gauge token and tranche is always 1.

massun-onibakuchi commented 1 year ago

opinion: leave this blank to allow gov to sweep any token https://github.com/Idle-Finance/yearn-tranche-strategy/issues/5#issuecomment-1192263148

bugduino commented 1 year ago

Specify junior or senior

https://github.com/Idle-Finance/yearn-tranche-strategy/blob/beb107efaa0f8c6090c102fa74e8878def9b51e3/contracts/Strategy.sol#L290

tranche.name() returns somthinIdleCDO AA Tranche - wstETH (AA_wstETH) ,which contains Junior or Senior. AA Tranche means Junior tranche

To be clear, AA is the Senior tranche while BB is the junior one

massun-onibakuchi commented 1 year ago

sorrry,I made a mistake

decaf-addict commented 1 year ago

use gas oracle pattern instead.

what's gas oracle pattern?

one of the comments above by Dudesahn https://github.com/Idle-Finance/yearn-tranche-strategy/issues/3#issuecomment-1138055945

decaf-addict commented 1 year ago

comments left in PR https://github.com/Idle-Finance/yearn-tranche-strategy/pull/6#pullrequestreview-1067778406

tapired commented 1 year ago

did the reviews in other issues, LGTM

0xValJohn commented 1 year ago

Placing this issue on hold until we determine a way forward.

bugduino commented 1 year ago

@0xValJohn if you need more info let me know, we removed also gauges in the meantime so there are less steps involved in general. Not sure what was the blocker here