Idle.finance update strategy

[vany365]

Description: Update to old idle.finance strategy. This is an updated based on this old issue: https://github.com/Idle-Finance/yIdleStrategies/issues/1

Commit (hash):

Tag:

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

Scope: (List link to files)

• File.sol

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:

• [ ] @wavey0x
• [x] @tonkers-kuma

Review Completed By:

• [ ] @wavey0x
• [x] @tonkers-kuma

[decaf-addict]

ability to exit during migration shouldn't depend on enabledStake, this could lead to accidentally leaving idle tokens behind

https://github.com/Idle-Finance/best-yield-yearn-strategy/blob/f5a6221347286538c5fda84484f5cb5c9b0dfb31/contracts/StrategyIdle.sol#L415-L416

[decaf-addict]

address injection needs higher access rights. Please elevate to onlyGovernance https://github.com/Idle-Finance/best-yield-yearn-strategy/blob/f5a6221347286538c5fda84484f5cb5c9b0dfb31/contracts/StrategyIdle.sol#L122

[decaf-addict]

opinion: loss should only reported when the strategy is unable to withdraw the requested amount, not any time eta is lower than debt, wdyt @wavey0x? https://github.com/Idle-Finance/best-yield-yearn-strategy/blob/f5a6221347286538c5fda84484f5cb5c9b0dfb31/contracts/StrategyIdle.sol#L330

[decaf-addict]

accounting logic can be simplified and made more concise https://github.com/Idle-Finance/best-yield-yearn-strategy/blob/f5a6221347286538c5fda84484f5cb5c9b0dfb31/contracts/StrategyIdle.sol#L337-L356

example: https://github.com/tonkers-kuma/strategy-88mph/blob/b10e3a177decdccd83872235665e545c908b1899/contracts/Strategy.sol#L139-L148

[decaf-addict]

opinion: With yswaps, reward selling is now done asynchronously. Claiming could be moved to adjustPosition, which is a cheaper operation that can be called more frequently https://github.com/Idle-Finance/best-yield-yearn-strategy/blob/f5a6221347286538c5fda84484f5cb5c9b0dfb31/contracts/StrategyIdle.sol#L318-L320

[decaf-addict]

suggestion: add external onlyVaultManagers versions of these functions (_divest, _invest, _claim) to allow more flexibility during emergencies https://github.com/Idle-Finance/best-yield-yearn-strategy/blob/f5a6221347286538c5fda84484f5cb5c9b0dfb31/contracts/StrategyIdle.sol#L459

[decaf-addict]

questions: What are the possible multi-reward reward tokens and do you have an estimated total apy for this strat?

[massun-onibakuchi]

questions: What are the possible multi-reward reward tokens and do you have an estimated total apy for this strat?

@tonkers-kuma

atm Best Yield will have no rewards. Right now we only distribute LDO for the stETH tranche, but we will also add MTA distribution for musd3crv and PNT for pbtccrv pool.

we have estimated total apy of Best Yield as follow.

[decaf-addict]

Latest changes LGTM!

[massun-onibakuchi]

whoops, i accidentally merged the PR before @wavey0x review. will open again if necessary.

[dudesahn]

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 🙂

[wavey0x]

LGTM

[storming0x]

@massun-onibakuchi

i will start doing the sec review for this on yearn's side.

A few initial questions, is this an update from the previous IDLE strategy version? if that's the case can you provide a link and paste here through https://www.diffchecker.com/.

What tokens/vaults is this strategy targeting?

is ProxyFactoryInitializable in scope?

[storming0x]

Question:

Can you walk me thru this scenario, how likely are we to have a loss here on a user withdrawal?

https://github.com/Idle-Finance/best-yield-yearn-strategy/blob/fa920508a32eaf40716ad67bf6a5e5eb40f88235/contracts/StrategyIdle.sol#L398

If a loss on withdrawal under specific conditions is very likely given fees or slippage, etc, we try to minimize it by setting a check on protection here, if (lossProtectionEnable) revert if loss > 0.

Altho, in this case the user would take the loss not the vault, (which is what we usually want) just want to get a sense how likely is this scenario here.

[storming0x]

Comment:

Can you share the code for multiRewards contracts here specifically

https://github.com/Idle-Finance/best-yield-yearn-strategy/blob/fa920508a32eaf40716ad67bf6a5e5eb40f88235/contracts/StrategyIdle.sol#L414

We may need a DD doc based on this contract, since it was not reviewed before as part of the first IDLE strategy and the strategy max approves their entire balance to it.

This DD document contains but not limited to:

1. Audit status and bounties.
2. Multi sig situation or upgradability options.

Example here:

# Protocol Due Diligence: [ Wild Credit ]

## Overview + Links
- **[Site](https://wild.credit)**
- **Team** 
- Anon
- **[Docs](https://wild-credit.gitbook.io/wild-credit/)**
- **Audits and due dilligence disclosures** None so far

## Rug-ability
**Multi-sig:**
None, Controller is owned by a timelock contract with 12 hours delay

**Number of Multi-sig signers / threshold:**
    NA, admin of timelock is a 1 EOA acc which is the wild deployer
**Upgradable Contracts:**
    None,but core factors like price oracle,collateral factors can be changed on controller 
**Decentralization:**
    Currently centralized with controller ownership being a centrally owned timelock contract

## Misc Risks

[List any risks pertaining to things like: how yield is generated, governance, multisig, etc]

- Price source for tokens is currently a UNIv3 oracle which fetches twap eth price for token * chainlink ethusd price for price data.
- Wildcredit previously was whitehacked when they had a unprotected initialize function which allowed anyone to drain the lending pair funds.
- There is a set borrow fee which will need to be covered overtime from the yield.
- Rewarded WILD for borrowing would be routed through UniV3 to want upon harvest. In the wip strategy case i will be using 1Inch to swap the rewards for want.
- The gas costs to lend or borrow are very high , there is a possibility tend and harvest will cost > 1k usd in gas costs based on the number of loops on a leverage/devleverage tx.
- 

### Audit Reports / Key Findings

[List links and any key findings]

### Anything else

[Is the protocol a great fit with yearn for a specific reason? Do Yearn and the other protocol mutually benefit?]

# Path to Prod

## Strategy Details
- **Description:**
- **Strategy current APR:**
- **Does Strategy delegate assets?:**
- **Target Prod Vault:**
- **BaseStrategy Version #:**
- **Target Prod Vault Version #:**

## Testing Plan
### Ape.tax
- **Will Ape.tax be used?:**
- **Will Ape.tax vault be same version # as prod vault?:**
- **What conditions are needed to graduate? (e.g. number of harvest cycles, min funds, etc):**

## Prod Deployment Plan
- **Suggested position in withdrawQueue?:**
- **Does strategy have any deposit/withdraw fees?:**
- **Suggested debtRatio?:**
- **Suggested max debtRatio to scale to?:**

## Emergency Plan
- **Shutdown Plan:**
- **Things to know:**
- **Scripts / steps needed:**
- **Is it safe to...**
    - call EmergencyShutdown
    - remove from withdrawQueue
    - call revoke and then harvest

[storming0x]

Question:

https://github.com/Idle-Finance/best-yield-yearn-strategy/blob/fa920508a32eaf40716ad67bf6a5e5eb40f88235/contracts/StrategyIdle.sol#L460

when would this be 0? is there a check we need to do in case we sent want but tokensMinted where 0 based on this line after:

https://github.com/Idle-Finance/best-yield-yearn-strategy/blob/fa920508a32eaf40716ad67bf6a5e5eb40f88235/contracts/StrategyIdle.sol#L462

[storming0x]

Question:

How much is the fee for IDle deposits?

https://github.com/Idle-Finance/best-yield-yearn-strategy/blob/fa920508a32eaf40716ad67bf6a5e5eb40f88235/contracts/StrategyIdle.sol#L516

[storming0x]

Suggestion:

Ideally we should not add another exit condition to strategy during a migration. Migration is designed to assume the underlying protocol is in the worst case scenario and assume contracts revert or just don't work.

If this line fails the rest of want or funds are trapped in this contract without being able to migrate.

https://github.com/Idle-Finance/best-yield-yearn-strategy/blob/fa920508a32eaf40716ad67bf6a5e5eb40f88235/contracts/StrategyIdle.sol#L414

A workaround we can use since we have manual methods to unstake is to add a flag on migration like: checkStakedBeforeMigrating and that is turn true by default but in case of emergency we have the option to turn it off and not interact with multiRewards during migration if we have to.

This also means we may need to remove protectedTokens here to be able to rescue idleTokens after with sweep

https://github.com/Idle-Finance/best-yield-yearn-strategy/blob/fa920508a32eaf40716ad67bf6a5e5eb40f88235/contracts/StrategyIdle.sol#L298

This is doomsday scenario design to build as many escape hatches as we can during an emergency.

[massun-onibakuchi]

Thank you for your reviewing @storming0x

@massun-onibakuchi

i will start doing the sec review for this on yearn's side.

A few initial questions, is this an update from the previous IDLE strategy version? if that's the case can you provide a link and paste here through https://www.diffchecker.com/.

What tokens/vaults is this strategy targeting?

is ProxyFactoryInitializable in scope?

There are many updates from the previous strategy version which is complicated. Almost all of the codebase has been rewritten.

Main changes

• refactoring liquidatePosition and prepareReturn function
• staking tokens to multireward staking contract.
• removing swap function and outsourcing to ySwaps
• setter of TradeFactory and multireward

ProxyFactoryInitializable is in scope, which is the exactly same code as the previous strategy. i reused one deployed in the previous Idle Strategy version when i deployed Idle BY strategy for apetax. https://www.diffchecker.com/xzGuauM5

this strategy targets mainly dai, usdc and usdt

[massun-onibakuchi]

Question:

Can you walk me thru this scenario, how likely are we to have a loss here on a user withdrawal?

https://github.com/Idle-Finance/best-yield-yearn-strategy/blob/fa920508a32eaf40716ad67bf6a5e5eb40f88235/contracts/StrategyIdle.sol#L398

If a loss on withdrawal under specific conditions is very likely given fees or slippage, etc, we try to minimize it by setting a check on protection here, if (lossProtectionEnable) revert if loss > 0.

Altho, in this case the user would take the loss not the vault, (which is what we usually want) just want to get a sense how likely is this scenario here.

Idle BY charges a fee on profits. and there is no swapping when a use withdraw. basically a loss on withdrawal would be zero.

[massun-onibakuchi]

Comment:

Can you share the code for multiRewards contracts here specifically

https://github.com/Idle-Finance/best-yield-yearn-strategy/blob/fa920508a32eaf40716ad67bf6a5e5eb40f88235/contracts/StrategyIdle.sol#L414

We may need a DD doc based on this contract, since it was not reviewed before as part of the first IDLE strategy and the strategy max approves their entire balance to it.

This DD document contains but not limited to:

1. Audit status and bounties.
2. Multi sig situation or upgradability options.

Example here:

# Protocol Due Diligence: [ Wild Credit ]

## Overview + Links
- **[Site](https://wild.credit)**
- **Team** 
- Anon
- **[Docs](https://wild-credit.gitbook.io/wild-credit/)**
- **Audits and due dilligence disclosures** None so far

## Rug-ability
**Multi-sig:**
None, Controller is owned by a timelock contract with 12 hours delay

**Number of Multi-sig signers / threshold:**
    NA, admin of timelock is a 1 EOA acc which is the wild deployer
**Upgradable Contracts:**
    None,but core factors like price oracle,collateral factors can be changed on controller 
**Decentralization:**
    Currently centralized with controller ownership being a centrally owned timelock contract

## Misc Risks

[List any risks pertaining to things like: how yield is generated, governance, multisig, etc]

- Price source for tokens is currently a UNIv3 oracle which fetches twap eth price for token * chainlink ethusd price for price data.
- Wildcredit previously was whitehacked when they had a unprotected initialize function which allowed anyone to drain the lending pair funds.
- There is a set borrow fee which will need to be covered overtime from the yield.
- Rewarded WILD for borrowing would be routed through UniV3 to want upon harvest. In the wip strategy case i will be using 1Inch to swap the rewards for want.
- The gas costs to lend or borrow are very high , there is a possibility tend and harvest will cost > 1k usd in gas costs based on the number of loops on a leverage/devleverage tx.
- 

### Audit Reports / Key Findings

[List links and any key findings]

### Anything else

[Is the protocol a great fit with yearn for a specific reason? Do Yearn and the other protocol mutually benefit?]

# Path to Prod

## Strategy Details
- **Description:**
- **Strategy current APR:**
- **Does Strategy delegate assets?:**
- **Target Prod Vault:**
- **BaseStrategy Version #:**
- **Target Prod Vault Version #:**

## Testing Plan
### Ape.tax
- **Will Ape.tax be used?:**
- **Will Ape.tax vault be same version # as prod vault?:**
- **What conditions are needed to graduate? (e.g. number of harvest cycles, min funds, etc):**

## Prod Deployment Plan
- **Suggested position in withdrawQueue?:**
- **Does strategy have any deposit/withdraw fees?:**
- **Suggested debtRatio?:**
- **Suggested max debtRatio to scale to?:**

## Emergency Plan
- **Shutdown Plan:**
- **Things to know:**
- **Scripts / steps needed:**
- **Is it safe to...**
    - call EmergencyShutdown
    - remove from withdrawQueue
    - call revoke and then harvest

Multirewards contract is a contract in Idle Gauge system https://github.com/Idle-Finance/idle-gauges/blob/main/contracts/Multirewards.sol

might need DD on multirewards and LiquidityGauge used in tranche strategy. cc @bugduino

[massun-onibakuchi]

Question:

https://github.com/Idle-Finance/best-yield-yearn-strategy/blob/fa920508a32eaf40716ad67bf6a5e5eb40f88235/contracts/StrategyIdle.sol#L460

when would this be 0? is there a check we need to do in case we sent want but tokensMinted where 0 based on this line after:

https://github.com/Idle-Finance/best-yield-yearn-strategy/blob/fa920508a32eaf40716ad67bf6a5e5eb40f88235/contracts/StrategyIdle.sol#L462

basically this would be not zero. because of this lines https://github.com/Idle-Finance/best-yield-yearn-strategy/blob/fa920508a32eaf40716ad67bf6a5e5eb40f88235/contracts/StrategyIdle.sol#L453-L455

but i added if (enabledStake && tokensMinted != 0) {...} because multirewards.stake(tokensMinted) reverts if tokensMinted is zero.

[massun-onibakuchi]

Question:

How much is the fee for IDle deposits?

https://github.com/Idle-Finance/best-yield-yearn-strategy/blob/fa920508a32eaf40716ad67bf6a5e5eb40f88235/contracts/StrategyIdle.sol#L516

For the Best Yield strategies, there's a 10% performance fee deducted from the yield earned every time a user withdraws funds from a strategy.
details are here : https://docs.idle.finance/products/fees

[massun-onibakuchi]

Suggestion:

Ideally we should not add another exit condition to strategy during a migration. Migration is designed to assume the underlying protocol is in the worst case scenario and assume contracts revert or just don't work.

If this line fails the rest of want or funds are trapped in this contract without being able to migrate.

https://github.com/Idle-Finance/best-yield-yearn-strategy/blob/fa920508a32eaf40716ad67bf6a5e5eb40f88235/contracts/StrategyIdle.sol#L414

A workaround we can use since we have manual methods to unstake is to add a flag on migration like: checkStakedBeforeMigrating and that is turn true by default but in case of emergency we have the option to turn it off and not interact with multiRewards during migration if we have to.

This also means we may need to remove protectedTokens here to be able to rescue idleTokens after with sweep

https://github.com/Idle-Finance/best-yield-yearn-strategy/blob/fa920508a32eaf40716ad67bf6a5e5eb40f88235/contracts/StrategyIdle.sol#L298

This is doomsday scenario design to build as many escape hatches as we can during an emergency.

ok. i will create the flag

[bugduino]

Multirewards contract is a contract in Idle Gauge system https://github.com/Idle-Finance/idle-gauges/blob/main/contracts/Multirewards.sol

might need DD on multirewards and LiquidityGauge used in tranche strategy. cc @bugduino

Actually gauges are not used for the Best Yield strategy, but only for the Perpertual Yield tranches (which is the next strategy queued for review). We can simply remove the multiRewards part from this strategy @massun-onibakuchi

[storming0x]

@massun-onibakuchi pls paste here the updated commit w changes i can review them and comment on them as well. Thx.

[massun-onibakuchi]

@storming0x

commit with changes https://github.com/Idle-Finance/best-yield-yearn-strategy/pull/9/commits/84cd7b2e47cbd30a3dc8053c0055108bb3be2231

changes :

• remove multireward part
• remove enabledStake flag

thx

[storming0x]

Question:

after the new changes is there a need to use rewardTokens as well? i don't see it being a factor or used in prepareReturn or anywhere else?

Or they are being pulled by tradeFactory?

[massun-onibakuchi]

Question:

after the new changes is there a need to use rewardTokens as well? i don't see it being a factor or used in prepareReturn or anywhere else?

Or they are being pulled by tradeFactory?

when the strategy calls IdleToken.redeemIdleToken method, the strategy would receive governance tokens such as COMP. tokens set in rewardTokens are pulled by tradeFactory.

[massun-onibakuchi]

I updated claimRewards methods. https://github.com/Idle-Finance/best-yield-yearn-strategy/commit/d2f8efa1ac8ebbb060ee2708cdd6228e486acaf0

[storming0x]

Latest changes look good @massun-onibakuchi pls, coordinate w @wavey0x to handover a deployment address from this reviewed commit for further verification of the code.

[decaf-addict]

• remove multireward part
• remove enabledStake flag

Had the same question of why they weren't used when I was looking at the deployed strat. Thx for removing. LGTM!

[saltyfacu]

@massun-onibakuchi any news?