yearn / yearn-vaults Goto Github PK

Parameter changes would be very helpful to have the history logged so they can be searched in history

It's a bad idea to run multiple keepers with the same underlying key (you'll get nonce conflicts), so redesign the keeper script to be able to manage several strategies at the same time (during startup, and add/remove during runtime maybe? Perhaps by watching the Vault event stream?)

Add a view function that provides the following:

return self.depositLimit - self._totalAssets() if self.depositLimit >= self._totalAssets() else 0

Compute a management fee as issued inflation of Vault shares to rewards based on AUM (in return for proper management).

Remove withdrawal fees entirely, making withdrawals free (up to the withdrawal depth based on debt in strategies)

Add a transfer-like function which allows transactions denominated in vault's token.
https://github.com/dapphub/chai/blob/master/src/chai.sol#L108-L113

One step further might be a function that does withdraw + token.transfer, so the recipient receives the exact value in token with the required amount of sender's vault shares being burned.

Audit Issues:

• use safemath in harvestTrigger multiplication
• use modifiers for auth to avoid duplication
• add more events to setter functions (L156, 169, 174, 184, 189, 382)

Vault.vy issues

• Potential withdrawal lock -> Code

Auditor's Note:

In case if as result of withdrawals in previous loop iteration self.token.balanceOf(self) become more that value that will cause transaction revert. That scenario is possible because only for first iteration we can exactly consider that value more than self.token.balanceOf(self), but for next iterations according withdrawal logic there is no check that real withdrawn amount(after Strategy(strategy).withdraw(amountNeeded) call) less or equal than desired amountNeeded.

It seems in normal/optimistic flow Strategy(strategy).withdraw(amountNeeded) never withdraw tokens more than requested, but anyway we recommend properly handling pessimistic case because strategy is external contract and can be broken. Due to withdrawal queue can be changed only by governance(usually governance is msig or smth like that) unexpected strategy behavior can lock withdrawals for undefined period that might be fatal in some cases.

I am wondering if yearn will provide a service to get the best borrowing interest rate.

Analyze coverage gaps for Vault/BaseStrategy/Registry (using brownie gui after brownie test tests/functional --coverage) and look for ways to increase coverage of calling conditions the test suite is currently missing.

Examples:

• token.transfer()/transferFrom() return False
• Vault.report() w/ 0% fees & profit
• Vault.report() w/ profit/debt payment but no tokens
• token.transfer() w/ no return value
• Strategy.harvestTrigger() w/ debt outstanding
• Strategy.harvestTrigger() w/ realized loss
• Strategy.sweep() w/ protected tokens
• Strategy.setKeeper()/setReward()/setMinReportDelay()/setProfitFactor()/setDebtThreshold() w/ governance

Brownie needs this: eth-brownie/brownie#411

This is an unlikely scenario, but could be good to assert in a test case the behavior for the vault to handle when a strategy is sending weird numbers for some reason like report(gain=100K, loss=100K).

A nice to have more than anything, don't see it as critical on top of other issues.

It should be safe to convert both the mgmt fee calculation, as well as the rate limit logic (both primarily used in Vault.report()) to be time-based instead of block number-based. This should be validated and commented on, to make sure that assumption is well-communicated to external integrations of strategy harvests.

This change would make both calculations more accurate, as well as make testing of their associated logic way easier.

Having strategy assert caller for governance-only functions is Vault.governance() is more efficient from an upgrade perspective than setting state variables in both

As discussed would be useful for harvest() calls which should be called even with 500 gwei.

If liquidatePosition(uint256 _amountNeeded)(uint256 _amountFreed) frees more than expected, it causes underflow down the line. This should be handled at BaseStrategy level and not cause an error.

Use this Strategy fix for now
banteg/strategy-uni-lp-pickle@fc8da9e

Currently, the deposits are unlimited on deployment. This is potentially problematic, as we shouldn't allow Vaults to be immediately available during the setup/configuration phase. Due to how deployments are handled, it would be safer to have to set this manually in order to "open up" the Vault for deposits once deployed to mainnet, which can use multicall in order to set all the parameters needed at once, including Strategy approvals, Guest List, etc.

From Audit Report:

The vault contract manages a withdraw queue. In some locations duplicate checks are being performed. However, in the function Vault.setWithdrawalQueue(_newQueue) there are protections against adding the address zero but there is no protection against adding the same address multiple times to the queue.

Additionally, the function Vault._organizeWithdrawalQueue() reorganizes the withdrawal queue such that all zero addresses are at the end of the array and none is in-between valid addresses. When adding a strategy to the queue by calling Vault,addStrategyToQueue(_strategy, *params) the loop ensures that the new strategy to be added is not in the queue already. We should evaluate to use the loop to put the new strategy into the correct free slot already. This would save the reorganization of the queue done in the next step with Vautl._organizeWithdrawalQueue.()

Explore a data structure that could enable constant-time membership checks as well as linear time insertion/removal/iteration.

NOTE: research adding this directly to Vyper as an additional data type available e.g. OrderedSet

Gnosis Safe uses VERSION idempotent storage variable

Also, figure out if there's a way we can use proxies smartly?

Need to see when OpenZeppelin upgrades

see: #502

Currently, total Vault debt limit doesn't really do very much (no winding back or balancing wind downs). Redesign this a bit so that total Vault Debt Limit = sum(Strategy debt limits), using ratios for the % of total investible that the Vaults get. Also consider changing the Vault debt limit to be a read only method, and instead specify % overhead to leave of total assets. The deposit limit forms part of the solution here.

Which it's usually advisible to have the Vault wrap the description and symbol of the token it is wrapping, in some circumstances this might not be advisible. For example, the yUSD Vault is actually y"yDAI+yUSDC+yUSDT+yTUSD" which makes it really difficult for certain integrations to list.

Add an override that lets someone declare a different symbol and/or description for the Vault

migrateStrategy shouldn't empty the replaced strategy params, it should do the same thing as revoke.

Include a way to configure a cap on deposits (configured at deployment), with MAX_UINT256 being uncapped

If the strategy doesn't have more want than debtOutstanding there is no need to call adjustPosition.

Currently have linting for Python and Solidity files, but not Vyper.

Add linting support + hook up to Github workflows.

Because withdrawals leave some underdefined behavior to strategies to implement them as "try to liquidate up to amountNeeded (see below)

https://github.com/iearn-finance/yearn-vaults/blob/22d0607291980bb19b5110c46440843b2d9e3938/contracts/BaseStrategy.sol#L276-L286

We end up in a scenario where large slippage could be caused by that action, but the withdrawer would not realize that loss and instead socialize it to the other depositors (see below)

https://github.com/iearn-finance/yearn-vaults/blob/22d0607291980bb19b5110c46440843b2d9e3938/contracts/Vault.vy#L432-L440

We currently mitigate this through Strategy review, but this is not ideal

Deployment on Ropsten was failing to let me withdraw more than the "free" amount in Vault, meaning the withdrawal queue is broken in some way under this condition. I was attempting to force a partial withdrawal from a strategy.

https://ethereum.stackexchange.com/questions/84775/is-there-a-vyper-equivalent-to-openzeppelins-safeerc20-safetransfer/84776#84776

Example with StrategyUniswapPairPickle migration. Strategy has incurred a 0.5% loss caused by Pickle withdrawal fee, but the pricePerShare has remained the same, meaning the vault is actually short that amount.

pricePerShare 1.000001852497160145
estimatedTotalAssets 15.728413834807870538
Transaction sent: 0x8f5150e727b70961708548cad44ef527f48d446ec1f18813f2f2c6e786b22cd9
  Gas price: 0.0 gwei   Gas limit: 10000000
  Vault.migrateStrategy confirmed - Block: 11152421   Gas used: 251525 (2.52%)

pricePerShare 1.000001852497160145
estimatedTotalAssets 15.648645586618978664

• Add a strategy (by address, using pre-defined params w/ overrides available)
• Update params for strategy (single parameter or multiple params at once)
• Revoke a strategy
• Manage withdrawal queue (reordering, adding, removing)
• Allow "batching" multiple actions together using MultiCall
• Add a method to "delegate to" another account for execution (such as Gnosis Safe)

As a reference:
https://github.com/dapphub/chai/blob/master/src/chai.sol#L134-L153

We want to have more granularity on gov only methods.
The idea is to keep the following methods gov only

setName
setSymbol
setGovernance
acceptGovernance
setGuestList
setRewards
sweep
setPerformanceFees
setManagementFees
addStrategy
updateStrategyPerformanceFee
migrateStrategy
setDepositLimit

and make the following manager +gov available:

setWithdrawalQueue
updateStrategyDebtLimit
updateStrategyRateLimit
addStrategyToQueue
removeStrategyFromQueue

These APIs are underused, and incorrect given BaseStrategy.delegatedAssets() is missing from them. No sense in keeping them, this calculation is better done externally anyways.

Remove:

• Vault.balanceSheetOfStrategy(strategy) and associated tests
• Vault.totalBalanceSheet(strategies) and associated tests

Also not sure if Abstracts can be released via EthPM, but the Strategy interface should be possible.

Allow depositing tokens with permit(), so users don't need to send a second approve() call

Current subgraph is ugly cause there is no standartized way to retrieve parameters.

Filter which parameters are used/useful and introduce corresponding events.

This logic will cause a failing txn if self.token.balanceOf(msg.sender) > self.depositLimit - self._totalAssets():
https://github.com/iearn-finance/yearn-vaults/blob/958d380c61c993abbfaa5d3c4191feb204c14de2/contracts/Vault.vy#L608-L609

Management fee is calculated during report. It takes the current total assets and calculated by taking the difference between current timestamp and the last reported time. This is grossly unfair for the initial depositors if there is a big wait time between vault creation and first harvest.

Let's look at Hegic for example. Hegic vault was created 15 days ago. first harvest was 3 days ago.

So hegic vault created. Over the next two weeks it sits empty. Then money ($450k) is deposited and harvest is run. That deposit is charges management fee as if it had been in the vault for two weeks.

As we can see in the tx report users are charged about 743 HEGIC on their deposits of 1_146_733 HEGIC (0.06%) even though nothing has happened yet.

Suggest that we use total assets at last report. Not current total assets.

Create two new fields to be standard in strategy contracts.

1. Add a boolean field that indicates whether a strategy is delegating its funds to another strategy, which would greatly simplify TVL calculations. Currently, calculations for TVL to avoid double-counting needs to be curated manually. Addition of a simple boolean field to indicate whether the strategy were delegating its funds could move all of these calculations on-chain.

Conceptually, the calculation could work as: all added strategies -> update migrations -> filter out non-TVL -> perform estimated asset count

As a name for this boolean field, I was thinking isDelegated, but if that would lead to confusion with the "Delegated Vaults" then we could just call it includeInTVL.

2. Add a boolean field to signify whether a strategy is active or not, isActive. This would essentially allow "hiding" deprecated or inactive strategies without having to actually remove them from vaults.

If desired, this could also be automated with some kind of check on value locked in the strategy.

It might make sense to refactor the Vault to allow deploying shallow "forwarder-style" proxies for each released version of the Vault, to reduce the overall gas usage from deploying new copies.

Remove the pure modifier to be able to customize the name() of the strategy.

In 0.2.0, exitPosition was modified to be able to return a tuple with (uint256 _loss, uint256 _debtPayment).
There are two issues which were not addressed:

a) exitPosition might be able to return a profit
b) exitPosition is also called from setEmergencyExit() but the return values are not read by the vault.

Since after setEmergencyExit(), the strategist needs to call harvest(), I propose:

a) Remove the call to exitPosition() in setEmergencyExit()
b) Change the logic in harvest to handle to do the following:

if (emergencyExit) {
  (profit, loss, debtPayment) = exitPosition(vault.debtOutstanding());
} else {
  (profit, loss, debtPayment) = prepareReturn(vault.debtOutstanding());
}

Simple strategies which can withdraw immediately can call prepareReturn inside exitPosition and more complex strategies can try to unwind faster.

Somewhat related to #1 (fully support overrides)

I think we could probably do a better job of naming products across the yearn ecosystem, but starting with Vaults:

Current constructor for V2 vaults set name and symbol as follows:

self.name = concat("yearn ", DetailedERC20(_token).name())
self.symbol = concat("y", DetailedERC20(_token).symbol())

I suggest better differentiation between yearn products. For example, the name of a Vault could default to yearn Vault {token.name} or similar.

We may even want to consider making that differentiation in the symbol. Maybe a prefix of yv (e.g. yvUSD)? I say this with faint memory of an aToken style yUSD being discussed that could function as a yield-bearing stablecoin for the ecosystem. In this instance, yvUSD would effectively accrue yield as capital gains whereas yUSD would generate income. Not sure where we are with that discussion.

That being said, the y suffix is simple, has existing brand traction, and is easy to say!

Topics of discussion:

1. Introducing Vault into the name of the Vault token
2. Stronger symbol differentiation between products in the yearn ecosystem
3. Product naming schemes/preferences in general

Thoughts?

The Specification dictates that only governance should be able to undo a Vault shutdown, but the current setEmergencyShutdown also allows a guardian to undo a shutdown.

Work by submitting a PR to the feat/registry branch.

Contract is here: Registry.sol

Let's continue this discussion on this issue.

Debt Limits should be %-based, with the invariant that only <= 100% of the Vault's total assets can be lent out to Strategies

Debt limits should adjust downwards based on realized losses in report, and credit available should be computed dynamically via totalAssets and Strategy.debtLimit [%]

Also consider renaming Strategy.debtLimit to Strategy.creditLimit

• Emergency exit from False to Truegives the rate limit from the last block reported (which is earlier than when the reset happens)
• reseting debt limit on a strategy back to nonzero value (following revokeStrategy) uses the last block reported (which is earlier than when the reset happens)

We might want to let Vaults to be able to define "guest lists", which would be another contract that dictates who is allowed to participate in a Vault (and transfer shares). Potential use case here might be for more "experimental" style Vaults, or for "high risk" Vaults where we might want to define a limit based on something like $ value of YFI staked in governance, etc.

This would look like the following:

interface GuestList:
    def authorized(guest: address) -> bool: view

guestList: public(GuestList)


... # In deposit()
    if self.guestList.address != ZERO_ADDRESS:
         assert self.guestList.authorized(msg.sender)
...

In base strategy is: function name() external virtual pure returns (string memory);

It should be view and not pure. As pure we can't read from state so can't store a name.

Make a detection to disable if deltaDebt / rateLimit < blockDelta so it's safer for a wider range (doesn't run into overflow issues with big blockDelta * rateLimit)