New Idle.Finance Strategy

[vany365]

Description: Updated Idle.Finance strategy by Antonio

Commit (hash):

Tag:

Repo: https://github.com/Idle-Finance/yIdleStrategies/tree/feat-0.4.3

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:

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

Review Completed By:

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

[wavey0x]

Seems this is used in several places even on a single harvest call. Can user actions (e.g. deposit/withdraw) directly impact the idle token virtual price?

https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L54

[wavey0x]

I would expand permissions to allow Manangement to update this: https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L149

[wavey0x]

What are some situations where we would not want to update virtual price?

Possibly if there is a bug or issue on Idle side, we don't want to harvest into a loss? We have healthchecks which address this already. But maybe there's another reason you have in mind.

https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L131

[wavey0x]

Set healthcheck in the constructor:

0xDDCea799fF1699e98EDF118e0629A974Df7DF012

[wavey0x]

checking: is it possible for an idle tokens to not have 1e18 denominated virtual price? (e.g. USDT, etc)

https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L338

[wavey0x]

not necessary. but for your gas golfing: could add want as an input parameter here.

[decaf-addict]

Looks like this could be remnants of Emiliano's swapper. I'd recommend using ySwaps so that this won't have to be separately maintained https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/Converter.sol#L19

ySwap's idea is to give a yearn maintained swapper permissions to pull reward tokens from the strategy. The selling of tokens will be outsourced to ySwaps and the want will be airdropped back to the strategy asynchronously

strategy just needs to give it permission example: https://github.com/flashfish0x/StrategyConvexTemplate/blob/a4148923fa229441b8d1f20ccf7a1128853d83be/contracts/StrategyConvexFactoryClonable.sol#L287-L301

[decaf-addict]

I believe you can achieve the similar access restrictions with onlyVaultManagers without creating your own modifier

https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L66

[decaf-addict]

weth address won't change. Should be hardcoded as constant and public if you need the getter

remove from param: https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L74

can be removed: https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L511

[decaf-addict]

question: why add 1 here? https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L212

[decaf-addict]

Is there particular bytecode size savings with this pattern? Also the param for the internal method doesn't seem necessary

https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L432-L438

[decaf-addict]

looks like you can reuse liquidatePosition() here https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L265-L271

[decaf-addict]

I think you can do this calculation at the end and just combine it with the profit recalc. This way, the profit can be used to repay any potential non-zero loss

https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L273-L279

[decaf-addict]

If I'm reading this correctly, this seems like an odd way to enforce minAmounts returned from a swap. Wouldn't this require you to custom set an amount every harvest? A better way would be to use a price quote from IConverter and reduce that amount by x% or whatever price impact/slippage is tolerable

https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L465

[decaf-addict]

question: what could cause this?

https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L58

[decaf-addict]

set these to onlyVaultManagers

https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L131 https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L139 https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L144

[decaf-addict]

Redundant. If you don't override the method at all, it'll just use the parent function, which is what calling super. is doing

https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L385-L387

[massun-onibakuchi]

Seems this is used in several places even on a single harvest call. Can user actions (e.g. deposit/withdraw) directly impact the idle token virtual price?

https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L54

Virtual price would be changed based on net asset value and idle token totalSupply. Possibly I can remove this modifier.

[massun-onibakuchi]

question: what could cause this?

https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L58

lastVirtualPrice <= currentTokenPrice will be true when

question: why add 1 here?

https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L212

My guess as to the intent of the original Emiliano's code is to cover rounding errors. According to BaseStrategy#estimatedTotalAsset

This value should be higher than its expected value to be "safe".

[massun-onibakuchi]

Is there particular bytecode size savings with this pattern? Also the param for the internal method doesn't seem necessary

https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L432-L438

I added this internal function to avoid reading repeatedly want by using SLOAD. I would like to remove the external function rather than the internal function

[massun-onibakuchi]

If I'm reading this correctly, this seems like an odd way to enforce minAmounts returned from a swap. Wouldn't this require you to custom set an amount every harvest? A better way would be to use a price quote from IConverter and reduce that amount by x% or whatever price impact/slippage is tolerable

https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L465

yeah I know what you mean. When I have asked about the handling of slippage, I have gotten the following response

How do you know what the price should be? When you do it manually on uniswap you set the slippage based on the current price which uniswap then checks at the block you get mined.

That means you can’t use normal slippage checks on the same block. You need to find some other way to check what the price should be. Stuff like steth accumulator assumes a 1-1 price and calculates slippage off that. Another option is to use an oracle service like chain link. But they all have their issues

ref:yearn school tg https://t.me/c/1447010539/13038

so I made a setter for amount of output instead of x%.

because harvest transaction will be sent via flashbot RPC, I recognize that slippage is not much of a problem. actually not a few strategies allows 100% slippage (minimum output 0). In this case this can be accomplished by not setting a minimum amount. (initial mapping value is 0).

[bugduino]

Seems this is used in several places even on a single harvest call. Can user actions (e.g. deposit/withdraw) directly impact the idle token virtual price? https://github.com/Idle-Finance/yIdleStrategies/blob/be01957270d39674ed204c0f2b8b51621946ab00/contracts/StrategyIdle.sol#L54

Virtual price would be changed based on net asset value and idle token totalSupply. Possibly I can remove this modifier.

I think that this could be useful in case of hacks on Idle side, so to avoid depositing in a potentially hacked pool

[vany365]

Repo has been migrated. Please see above. Per idle team it is ready for review. Once its established we will close this ticket. Thanks