# Yearn V3 Strategies Review checklist and Gotchas

## Functions to override

### **_deployFunds(uint256 _amount)**

- `_amount` parameter is `S.totalIdle + assets` , where `S.totalIdle` is supposedly the idle balance of the contract and `assets` is the fresh taken user funds.

- **Check assumptions on trying to deploy the `_amount`. If in some cases strategy doesnt want to fully deploy the `_amount`, then override `availableDepositLimit` function such that `_amount` is always deployable**

- **IMPORTANT** Some strategies may not be suited for force `_deployFunds` on deposits since this could be an attack vector in some cases. e.g single siding into an LP pool that can be unbalanced or MEV'd, etc. *Careful* consideration must be employed and tests on these cases for strategies that deposit directly to a protocol and on what cases it can become dangerous to do so. Flags or other mechanism to limit rate of funds deployed may be reasonable in some cases.

### **_freeFunds(uint256 _amount)**

As stated in the natspec of the original code:

Any difference between `_amount` and what is actually freed will be
counted as a loss and passed on to the withdrawer. This means
care should be taken in times of illiquidity. It may be better to revert
if withdraws are simply illiquid so not to realize incorrect losses.

- `Math.min(_amount, asset.balanceOf(address(this)))` should be safe to use

- On normal operations it should ONLY free the `_amount` of funds

- **ALWAYS FREE THE `_amount` worth of position NOTHING MORE.
If let's say there is a withdrawal fee, do not account it.**

### availableWithdrawLimit ###

This function defaults to MAX uint 256, also by default there is no check wheter the vault is in emergency mode or not for withdrawals.

Since this function overrides the maximum amounts of withdrawals users can make [here](https://github.com/yearn/tokenized-strategy/blob/8f6992d566a0c62a212beba93d593c1641ad3913/src/TokenizedStrategy.sol#L671) , is prudent to request an override of the function to allow for pause mechanisms by management or governance on extreme scenarios where we may need time before enabling withdrawals again.

Also a limit may be set for withdrawals with cooldown in case of an emergencyShutdown to allow for response time on an incident but still be permissionless to enable withdrawals after a set time.

This may be done with a new method `startCoolDown()` in `BasedTokenizedstrategy` that is open to any holder of the vault and sets a time configurable setting to allow withdrawals again after a period of time that is prudent.

The intention is to mimic some of v2 safe guards around queue management to repair strategies while pausing withdrawals, this may not be an option in all cases but its prudent to leave the door open for emergency scenarios. Something to evaluate as option in a case by case basis based on strategy risk profile and projected TVL.

### **_emergencyWithdraw(uint256 _amount)**

- `_amount` parameter needs to be uint256.MAX amount to fully exit

This method is important to override in strategies that are intended for production in yearn vaults. Ideally regardless of the amount it should default to total balance and exit once.

The method needs to try to exit all funds from the protocol yield source if possible without even getting rewards, this must assume the protocol is in the worst state possible.

This method requires that the strategy method `shutdownStrategy` is called before calling emergencyWithdraw. ***So emergency exit is a two step process and it cannot be undone.***

1. `strategy.shutdownStrategy()`
2. `strategy.emergencyWithdraw(2**256-1)`

**TESTS should be require in reviews to ensure the strategy can exit position in emergency entirely.**

### **updateDebt**

It's important to check the `deposit` logic on tokenized strategies actually mints the correct shares since vault when updating debt to increase it, it doesnt check the final shares number increased only the asset balances side effects.

https://github.com/yearn/yearn-vaults-v3/blob/05fbd377a7778c660034f17c11b32e3767ff9166/contracts/VaultV3.vy#L799

Note: this case may apply more for external to yearn contracts that are erc4626 compliant but have different logic for minting shares or edge cases.

### **_mint**

The `_mint` function https://github.com/yearn/tokenized-strategy/blob/ba8e6e1ca74b21e566f81b92618bcf39fe354ca2/src/TokenizedStrategy.sol#L1707

is not meant to be called directly by Contracts inheriting BaseTokenizedStrategy, a strategy should **NEVER** need to be calling the `_mint` method directly since minting to `self or address(this)` breaks an important invariant of the tokenized strategies regarding profit unlocking.

### set_open_role open

in v3 vyper vault here there's an open role setter
https://github.com/yearn/yearn-vaults-v3/blob/1e6bc67cf70352e9567e67404d73856cc343b634/contracts/VaultV3.vy#L1286

to allow a specific role to be called by any msg.sender, careful checks need to be considered in security reviews around expectations and consequencies of opening this role up for certain operations and considering the overall use case flow for security implications.

### **availableDepositLimit**

- If limit is calculated as how much strategy can deploy up to, count the `S.totalIdle` too.

- **Deposit check is only applied to "assets" user puts to the strategy. However, actual amount deployed is S.totalIdle + assets. So consider this !**

### **harvestAndReport**

- V2 Harvest alike function.

- NO NEED TO UNWIND THE PROFIT FROM THE STRATEGY. Just report what strategy has, no need to free profit or anything.

- Sell rewards, deploy them to strategy, account the final balance, report loss or profit and REDEPOSIT. Even report loss, redeposit.

**IMPORTANT SECURITY CONSIDERATIONS of `harvestAndReport`**

* If the implemented method relies on protocol oracles, spot price, etc and the totalAssets return value can be manipulated or is badly calculated it could incorrectly report a profit or loss and leave the strategy in bad accounting state. Important checks of tests and logic are needed here.

* If the `asset.balanceOf(this)` is by some error not reported or incorrectly reported, the strategy will assume is idle by default and may revert here if newTotalAssets < newIdle.
https://github.com/yearn/tokenized-strategy/blob/8f6992d566a0c62a212beba93d593c1641ad3913/src/TokenizedStrategy.sol#L1059

* Because of reasons above it may be important to require healthcheck on `_harvestAndReport` implementations to avoid any miscalculations in reports.

## Linked Libraries on Contracts that extend BasedTokenizedStrategy

`The only time the onlySelf may be used is if you add a Linked Library to your strategy`

`which i would say should be discouraged`

Quote from Schlag

## GENERAL
- Any airdrop to strategy is not accounted as IDLE funds. It won't be accounted in `S.totalIdle`!!

- If yTrades sends `asset` token to strategy, the profit accounting will be held inside the `harvestAndReport` strategy. Untill then strategy has no idea of the airdropped amount.

- Strategy mints shares according to how much user provided "asset" tokens to strategy. Strategy doesn't care how much of it put into position. When withdrawing account for this too. Do not over withdraw if there is a withdrawal fee. Let the user incur losses.
-

Example:
Let's imagine we have a bridge strategy, which includes both a 10% deposit and a 10% withdrawal fee.

Now, let's say a user decides to deposit 100 DAI into the strategy. At this point, the strategy issues the user 100 shares. The strategy doesn't care about the deposit fee at this point. What's important is that the user has provided 100 DAI.

Next, the same user decides to withdraw, and they want to take out 50 DAI. This is where the _freeFunds function comes into play. It's set up to withdraw exactly 50 DAI. Of course, because of the 10% withdrawal fee, the user actually ends up with 45 DAI, and there's a loss of 5 DAI. But here's the thing: the _freeFunds function doesn't try to pull out 55 DAI to cover the withdrawal fee. It just reports the loss. And that's totally okay.

## Direct Storage updates with assembly

Note @storm0x : this is my impression from reading the code, it may be a general assumption to look out for and have very few exceptions if any.

Tokenized strategies have a specific storage pointer slot to update important data, careful review should be done if there are any direct storage updates in assembly from a specific implementation of BaseTokenizedStrategy since most if not all strategies shouldn't be writing directly to slots in storage pointer.

https://github.com/yearn/tokenized-strategy/blob/ba8e6e1ca74b21e566f81b92618bcf39fe354ca2/src/TokenizedStrategy.sol#L342

Most implementations should not need to directly update storages if at all, direct usage of assembly writing to storage location `keccak256("yearn.base.strategy.storage") - 1` should be disallowed.