Yearn Vault V3 review by panda

===

## Scope
[Code Repo](https://github.com/jmonteer/yearn-vaults-v3)
[Commit](https://github.com/jmonteer/yearn-vaults-v3/commit/90ad39d8f8b8929383feac9b4533b220593d5471)

The commit reviewed was 90ad39d8f8b8929383feac9b4533b220593d5471. The review covered the entire repository with a focus on the vyper contracts.

## Critical Findings

No critical findings were found.

## High Findings

### 1. High - There is no warranty the strategies uses all the funds it was approved to use

A strategy might only transfer some of the funds it was offered to use. It's assumed in the contract that the allowance might be spent less than 100%. This could result in wrong debt accounting, with more debt on the strategy than it was supposed to.

## Proof of concept

self.erc20_safe_approve(ASSET.address, strategy, assets_to_transfer)
IStrategy(strategy).deposit(assets_to_transfer, self)
self.erc20_safe_approve(ASSET.address, strategy, 0)
self.total_idle -= assets_to_transfer
self.total_debt += assets_to_transfer

The `erc20_safe_approve(ASSET.address, strategy, 0)` is only useful if the strategy hasn't used its entire allowance.
https://github.com/jmonteer/yearn-vaults-v3/blob/90ad39d8f8b8929383feac9b4533b220593d5471/contracts/VaultV3.vy#L684

## Impact

Non-expected debt on the strategy.

## Recommendation

Ensure the strategy spends all the allowance or check for unspent allowance and remove it from `assets_to_transfer`.

### 2. High - Strategy frontrunning

The vault has an anti-front-run mechanism. Strategies can get deposits from multiple addresses that aren't limited to yearn vaults. It's possible for anyone to front-run/sandwich the strategy profits on strategies such as selling a token to make a profit.

## Impact

Vaults using the strategy will only benefit from a smaller portion of the profit sharing.

## Recomendation

Make strategy `deposit` a gate kept function to only vaults.

### 3. High - Accountant funds can be drained

Account report isn't protected; with a malicious vault contract, it's possible to drain the funds out of the contract.
The issue is also present on Accountant and FlexibleAccountant contracts.

## Proof of concept

By creating contracts that behave like a vault, it's possible to drain funds using strategies that have a `refund_ratios` not set to zero by sending a high loss.

https://github.com/jmonteer/yearn-vaults-v3/blob/90ad39d8f8b8929383feac9b4533b220593d5471/contracts/periphery/Accountant.vy#L71

## Impact

Funds will be lost to the attacker.

## Recommendation

Control the access to the report function.

## Medium Findings

### 1. Medium - Vault shares can be transferred to vault or zero address

The transfer function does not prevent shares from being sent to the vault or the zero address.

## Proof of concept

@internal
def _transfer(sender: address, receiver: address, amount: uint256):
    # Protect people from accidentally sending their shares to bad places
    assert self.balance_of[sender] >= amount, "insufficient funds"
    self.balance_of[sender] -= amount
    self.balance_of[receiver] += amount
    log Transfer(sender, receiver, amount)

https://github.com/jmonteer/yearn-vaults-v3/blob/90ad39d8f8b8929383feac9b4533b220593d5471/contracts/VaultV3.vy#L191

## Impact

Funds can be lost for a user; I do not see a PPS manipulation possible when transferring funds to the contract.

## Recommendation

Prevent transfers to self and zero address.

### 2. Medium - Withdrawing can be performed without loss by manipulating the array of strategies.

In the case of a strategy that hasn't reported a loss yet, a user can withdraw his funds without loss by using strategies that aren't lossy before the call to report the loss is made.

## Low findings

### 1. Low - Calling non-standard ERC20 contracts should use the new vyper default response functionality

In multiple places in the vyper contracts, the call to non-standard ERC20 is handled with a `raw_call` method. It should be replaced by adding the new `default_return_value` kwarg to the function when calling it.

## Proof of concept

response: Bytes[32] = raw_call(
    token,
    concat(
        method_id("approve(address,uint256)"),
        convert(spender, bytes32),
        convert(amount, bytes32),
    ),
    max_outsize=32,
)
if len(response) > 0:
    assert convert(response, bool), "Transfer failed!"

https://github.com/jmonteer/yearn-vaults-v3/blob/90ad39d8f8b8929383feac9b4533b220593d5471/contracts/VaultV3.vy#L337

https://github.com/jmonteer/yearn-vaults-v3/blob/90ad39d8f8b8929383feac9b4533b220593d5471/contracts/VaultV3.vy#L354

https://github.com/jmonteer/yearn-vaults-v3/blob/90ad39d8f8b8929383feac9b4533b220593d5471/contracts/VaultV3.vy#L372

https://github.com/jmonteer/yearn-vaults-v3/blob/90ad39d8f8b8929383feac9b4533b220593d5471/contracts/periphery/Accountant.vy#L108

## Impact

Higher gas usage to deploy and copy the contracts.

## Recommendation

Here is one example of how I suggest doing it

sucess: bool = ERC20(token).approve(spender, amount, default_return_value=True)
assert sucess, "Approval failled."

### 2. Low - FlexibleAccountant doesn't handle non-standard ERC20

Unlike the other contracts, the FlexibleAccountant doesn't use a safe `approve` method

## Proof of concept

ERC20(self.asset).approve(msg.sender, total_refunds)

https://github.com/jmonteer/yearn-vaults-v3/blob/90ad39d8f8b8929383feac9b4533b220593d5471/contracts/periphery/FlexibleAccountant.vy#L109

## Recommendation

Use the following method.

sucess: bool = ERC20(token).approve(spender, amount, default_return_value=True)
assert sucess, "Approval failled."

## Gas findings

### 1. Gas - unnecessary assert condition

https://github.com/jmonteer/yearn-vaults-v3/blob/90ad39d8f8b8929383feac9b4533b220593d5471/contracts/VaultV3.vy#L468

if shares == max_value(uint256):
    shares = shares_balance

assert shares_balance >= shares, "insufficient shares to redeem"

## Recommendation

if shares == max_value(uint256):
    shares = shares_balance
else:
    assert shares_balance >= shares, "insufficient shares to redeem"

## Question findings

### 1. Question - Should it be possible to add strategies to a blacklist to prevent withdrawal from it.

On v2, it's possible to remove a strategy from the withdrawal list or to put it at the bottom of the list to make sure funds aren't withdrawn from it. I wonder if a mechanism to block/prevent from withdrawing from specific strategies is required?

### 2. Question - Funds might not be withdrawable without loss

I can see a case where a vault has multiple strategies, some that are lossy if funds are withdrawn and some that are not. It seems to me some will be able to empty the non-lossy strategies when possible and prevent others from withdrawing without a loss. This could be an issue for projects building on top of the vault that might not be able to withdraw any amount of tokens without a loss.

### 3. Question - How does a strategy estimate the unrealized loss

I wonder how possible it is for all the strategies to estimate the unrealized loss without triggering it.

### 4. Question - Should the minimum_total_idle be expressed in percentage

When minimum_total_idle expressed in assets isn't zero, the amount of funds left is most likely too little for big vaults or too big for small vaults. With some of the vaults funds moving rapidly, it will require more monitoring and actions than a value expressed in percentage.

## Naming

On the line https://github.com/jmonteer/yearn-vaults-v3/blob/90ad39d8f8b8929383feac9b4533b220593d5471/contracts/VaultV3.vy#L496 using share is confusing since it's not referring to shares but to the shared loss.