# PWC Yearn Finance SC Audit Issues Tracking

NOTE: see PWC report for details and reference
Dev can assign themselves to each issue with () to own it and work in PR or response

## Medium Issues

### 5.1 Adding Strategies in Emergency Shutdown Mode (Doug)
- [x] Fixed [#145](https://github.com/iearn-finance/yearn-vaults/pull/145)
- [ ] Won't Fix/Acknowledged

### 5.2 Assumptions About Connected Contracts Not Specified Sufficiently
- [ ] Fixed
- [x] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: It is indeed the case that touch points such as the governance and attached strategies are not a part of this review, as well as some of the other components such as the Registry (completed after this review had started). However, the most important interactions are between the Vault and the BaseStrategy template, which is intended to organize the primary security-critical interactions between Strategies (which inherent from BaseStrategy) and the Vault code (which is not extendable). It is noted that this review can only consider the interactions between these two components, and that any departures from the standard implementations must be carefully reviewed by the team internally before deployment, eventually obtaining an external review as well.
- **@fubuloubu**: Functions that begin with `estimated*` or `expected*` are not to be used in a security-critical context, and are only there as a guide to be used in an off-chain context.

### 5.3 Front Running Significant Losses
- [ ] Fixed
- [x] Won't Fix/Acknowledged

Notes:
- (Doug) Two possible mitigations that come to mind is calling harvest through private mining for transactions with possible smaller losses. As an additional mitigation in an extreme case of a hack on a strat, we can disable withdrawals with shutdown like we do in deposits to avoid early withdraws that can damage remaining users until harvest report for affected strategy is done. wdyt?
- **@fubuloubu**: We are investing in private mempool infrastructure for our keepers (using Keep3r) such that we minimize the impact of mempool monitoring and attacks that can occur due to large reports from connected strategies.
- **@fubuloubu**: There is a known issue that users may be able to frontrun other depositors and avoid losses if a strategy were to suffer from a hack or other event that affects the price of deposits. We have several mitigations in place, including reorganization of withdrawl queue and the recently added withdrawal loss bookkeeping (see [#140](https://github.com/iearn-finance/yearn-vaults/pull/140) and [#143](https://github.com/iearn-finance/yearn-vaults/pull/143)) which should reduce the impact of these issues. A proper resolution of this issue is very difficult, and we are working on better ways to mitigate these scenarios at present.

### 5.4 Incorrect Definition of `BaseStrategy.minReportDelay`
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Agree, should change to `maxReportDelay`

### 5.5 Interaction between `Vault.withdraw()` and `Strategy.liquidatePosition()`
- [ ] Fixed
- [x] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: (1) It is up to the Strategy implementation to decide if additional actions need to be taken based on the current balance the strategy has available to itself, as well as the investment of it's current positions. The `BaseStrategy` template does not have sufficient context to make this determination; for example, a debt-based Strategy may need a certain level of buffer available to itself outside of it's core positions to avoid liquidation penalties.
- **@fubuloubu**: (2) See the reasoning for (1)

### 5.6 Missing Sanity Check for Fees
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Agree, should ensure that the setting for fees is bounded by the maximum setting during occasions when that value could change.

### 5.7 Missing Sanity Check in `Vault.addStrategyToQueue()`
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Agree, assert that strategy argument is not 0x0

### 5.8 No Role can Exit Strategy Exit Shutdown
- [ ] Fixed
- [x] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: This is by design. The intention behind "Emergency Exit" mode for a strategy vs. a less severe "revoke" is that the strategy has some level of catastrophic loss that needs to be exited as quickly as possible. Simply revoking the strategy is enough to sunset it during normal operations, allowing it to wind down gracefully without liquidating it's positions for potentially enormous penalties or slippage.The intention here is to cover scenarios where an external protocol has a hack in progress, and there is a chance to recover assets by reacting quickly to that scenario. In these scenarios, there didn't seem to be a good reason why undoing this condition seemed useful/necessary. It is meant as an option of last resort.
- **@fubuloubu**: Note that there is also a third mitigation available to strategies in that they can be fully migrated. Migration is really intended for when there is a bug in the Strategy contract itself, and is intended to be the least invasive of the 3 possible mitigations we have.

### 5.9 Remaining To-Dos (very top of the contract in line 40.)
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Will resolve these TODOs

### 5.10 Roles That Can Trigger Emergency Mode
- [ ] Fixed
- [x] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: This is by design, and seems like a mis-reading of the Specification. Governance or Guardian roles can enter `Vault.emergencyShutdown`, but only Governance can exit it.

### 5.11 Strategy Can Revoke Itself
- [ ] Fixed
- [x] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: This is by design as an implementation shortcut for how the "Emergency Exit" mode works. The `BaseStrategy` template only allows that method to be called from `BaseStrategy.setEmergencyExit` to complete that scenario and trigger a full divestment of the strategy. No other methods should allow this.

## Low Issues

### 5.12 Call Fails Late in `Vault.addStrategy()`
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: minor optimization, will consider further

### 5.13 Fee Calculation Specification
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Agree, will update Specification

### 5.14 Frontrunning Profit Reports
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: See answer to (5.3). In summary, we are exploring private mempool for our keeper infrastructure to mitigate this potential exploit. There is no known better mitigation at this time.

### 5.15 Governance Parameter Changes
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Update Specification

### 5.16 Imprecise Specification for Strategy Migration
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Update Specification

### 5.17 In `Vault.removeStrategyFromQueue()` No Error Message Is Present
- [ ] Fixed
- [x] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Error message add deploy and execution cost overhead. Reverting seems like natural behavior in this scenario.

### 5.18 Incorrect Code Comments
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Update comments

### 5.19 Inefficient State Variable Order
- [ ] Fixed
- [x] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Slot size optimization users a part of the Solidity compiler that has had issues in the past. Would rather avoid this type of optimization, even if it comes at the cost of several thousand gas.

### 5.20 Limited Effectiveness of `Vault.guestlist`
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: The intent of the function is not for "regulatory purposes" but as a temporary limiter on user deposits when the contract is deployed. We employ several of these types of mitigations (including the deposit limit) to make sure that Vault AUM remains "reasonably sized" and only competent individuals or individuals who can afford the losses are able to partipicate at first. Due to how Ethereum works, these mitigations are poor at best, and we don't want assumptions to be made that they can be used as effective regulatory compliance measures when they can't be considered fit for that purpose.
- **@fubuloubu**: We will explore if adding the guestlist check on transfer is desirable.

### 5.21 Mismatched Interface
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: This is leftover from a previous iteration where migrations occured from the other direction, and thus should be removed

### 5.22 Missing Sanity Checks in `BaseStrategy`
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Agree, add sensible input validation

### 5.23 Missing Sanity Checks in `Vault.setWithdrawalQueue()`
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Explore a data structure that could enable constant-time membership checks as well as linear time insertion/removal/iteration (see 5.28 and 7.1)

### 5.24 No Hard Enforcement of Wrong Loss Reports
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Perform the check and remove the `min` instead.

### 5.25 Non-Vault Specification
- [ ] Fixed
- [x] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: The note is correct, during Vault's "Emergency Shutdown" all *Strategies* are forced into a "normal shutdown" state (similar to revoke). This is transparent to the Strategy, they only see that their debt limit is suddenly 0.

### 5.26 Possible Reentrancy
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Add `@nonreentrant("withdraw")` decorator to prevent reentrancy attack possibilities in `Vault.withdraw()`

### 5.27 Redundant Contract Calls
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Explore if caching makes sense

### 5.28 Redundant Loop to Organize the Withdrawal Queue
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Explore a data structure that could enable constant-time membership checks as well as linear time insertion/removal/iteration (see 5.23 and 7.1)

### 5.29 Redundant Read Accesses to Storage
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Explore gas cost optimizations

### 5.30 Reporting Gain and Loss at Once Is Possible
- [ ] Fixed
- [x] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: This is by design. A fairly common use case would be a CRV farming strategy that farmed a realized gain for reward tokens but realized some slippage loss on the deposit due to pool imbalance. The independence of these variables has utility.

### 5.31 Specification Regarding User Deposits
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Update the Specification

### 5.32 Speed During Emergency Exit
- [ ] Fixed
- [x] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: This is by design, as the action should cause the harvest trigger to almost immediately execute. We wanted to avoid the scenario where we are forcing the strategy to do something in that scenario it might not normally do, or have an errant revert being triggered which rejects our call to emergency exit

### 5.33 Strategist Fee During Emergency Exit
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: The `Vault.report()` function takes the tactic of repaying debt first before reporting a gain under the scenario that a shutdown is occuring. It could perhaps be more accurately stated in the Specification

### 5.34 Strategy Specification Belongs to Vault
- [ ] Fixed
- [x] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: This is correctly stated, the implementation detail is that `BaseStrategy.setEmergencyExit()` calls `Vault.revokeStrategy(address(this))` in order to block itself from pulling more debt.

### 5.35 Strategy Struct Optimization
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Premature optimization may be dangerous here, considering potential optimization incompatibilties between Solidity and Vyper.

### 5.36 Superfluous `min` Check
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Update line to remove the `min` operation, and set directly to the rate-limited value

### 5.37 Unclear Assumptions Regarding Withdrawal From Strategy
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Rectify the comments and code with [#140](https://github.com/iearn-finance/yearn-vaults/pull/140) and [#143](https://github.com/iearn-finance/yearn-vaults/pull/143)

### 5.38 User Transfers Documentation Discrepancy
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Update Specification

### 5.39 User Withdraws
- [ ] Fixed
- [ ] Won't Fix/Acknowledged

Notes:
- **@fubuloubu**: Update Specification

## Audit Open Questions

### 6.1 Direct Migration of Strategy
A Strategy contract can be migrated by the connected vault or by the governance. However, it is unclear why it should be directly migrated by the governance. It seems that such a migration would create an inconsistent state where the vault suddenly refers to an old, fundless contract. Can you provide an example to motivate the functionality?

Answers:
- **@fubuloubu**: If the Vault was severely broken, than Governance could recover assets from all the connected Strategies via this function. Note that `BaseStrategy.sweep()` cannot be used as it protects against sweeping `want` tokens.

### 6.2 Event for Emergency Exit
The `BaseStrategy` contract emits many helpful events. However, it currently does not emit an event when the emergency exit status is being set which marks a significant event. Have you considered adding such an event?

Answers:
- **@fubuloubu**: Add event

### 6.3 Fee Deduction When Withdrawing
The code comments imply a fee deduction when forcefully withdrawing from strategies to cover a user's withdraw request that exceeds the funds held by the vault:

# |no|: This performs forced withdrawals from each Strategy. There is
# a 0.5% withdrawal fee assessed on each forced withdrawal (<= 0.5% total)

The code does not have any fee deduction logic implemented but there might be a hidden logic in the strategy contract when calling `Strategy(strategy).withdraw(amountNeeded)`. Could you please elaborate on the planned fee deduction in the scenario described above?

Answers:
- **@fubuloubu**: that is a mistake, will fix

### 6.4 Interface Consistency
In case of further development it seems beneficial to check the consistency of the two interfaces `VaultAPI` and `StrategyAPI` to the respective implementations. Have you considered some kind of automated consistency check?

Answers:
- **@fubuloubu**: Add test for `Vault` adherence to `VaultAPI` and `TestStrategy` adherence to `StrategyAPI`

### 6.5 Unclear Comment in `Vault.setRewards()`
The `setRewards` function of the `BaseStrategy` has the following comment:

* Used to change `rewards`. Any distributed rewards will cease flowing
* to the old address and begin flowing to this address once the change
* is in effect.
*
* This will not change any Strategy reports in progress, only
* new reports made after this change goes into effect.

However, the implementation immediately set the new rewards value and any subsequent call to `distributeRewards` would directly use the new value. What does the comment refer to?

Answers:
- **@fubuloubu**: Remove the second line of the comment, it doesn't make sense.

### 6.6 Why Is `debtPayment` Not Explicitly Included in `StrategyReported`?
In the report function the event StrategyReported is emitted to summarize the actions. However, it does not log debtPayment explicitly. Is this intentional?

Answers:
- **@fubuloubu**: `debtPayment` was added recently, should add this to `StrategyReported` event after `gain` and `loss`

### 6.7 What Are the Specifications for a Strategy Regarding the Vault It Can Belong To?
A Vault can have multiple strategies. Can a strategy also belong to multiple vaults? If not, how would this be prevented? If yes, how are migrations handled?

Answers:
- **@fubuloubu**: yes, it should be possible. The migration function in BaseStrategy would have to be overridden or capable of handling a 2-phase upgrade process

## Audit Notes

From report:
We leverage this section to highlight further findings that are not necessarily issues.

### 7.1 Data Structure for Withdraw Queue
Depending on the expected, average number of strategies managed per vault it could be beneficial to implement the withdraw queue as a linked list. Hence, insertions and removals would become significantly more efficient. Linked lists generally have to disadvantage of a necessary traversal, but every access to the withdraw queue is currently happening as form of a traversal. In case of very few strategies per vault the impact would be minor, but in case the limit of 20 is almost reached, then significant gas savings would be possible.

Comments:
- **@fubuloubu**: Explore a data structure that could enable constant-time membership checks as well as linear time insertion/removal/iteration (see 5.23 and 5.28)
- NOTE: research adding this directly to Vyper as an additional data type available e.g. `OrderedSet` (see [orderedset](https://pypi.org/project/orderedset/))

### 7.2 Harvest Trigger Return Value
The description for the prepareReturn function of the strategy which decides about reported gains, losses and the _debtPayment (the amount send back to reduce the outstanding debt) says:

Note: `_debtPayment` should be less than or equal to `_debtOutstanding`. It is okay for it to be less than `_debtOutstanding`, as that should only used as a guide for how much is left to pay back.

If this advice is followed, then the strategy will continue to have outstanding debt. Hence, inside `harvestTrigger`:

uint256 outstanding = vault.debtOutstanding();
if (outstanding > 0) return true;

`true` will always be returned. It seems slightly contradictory that outstanding debt is important enough to constantly trigger harvest, but not important enough to pay it back during the execution of harvest.

Comments:
- **@fubuloubu**: Have to unpack this one more. May need clarification to the logical inconsistency.

### 7.3 Solidity Compiler Version
For the chosen solidity compiler version 0.6.12 there are two known bugs: "DynamicArrayCleanup" and "EmptyByteArrayCopy". Both of these vulnerabilities have medium severity. Currently, both of these do not apply, however concrete implementations of the contract could be affected. Hence, a compiler version upgrade could be considered.

Comments:
- **@fubuloubu**: Consider upgrading to 0.8.x series (0.7.x was avoided due to several inconsistencies)

### 7.4 Total Gain Could Be Calculated Off-Chain
The vault keeps track of the strategies' total gains in a state variable. This state variable is only used in _expectedReturn. The internal function is only used in expectedReturn. expectedReturn is an external function. Hence, the state variable could be also calculated off-chain by checking the events and doing the calculation. yearn.finance could re-evaluate whether the on-chain storage is worth the additional gas.

Comments:
- **@fubuloubu**: We were exploring expanding this feature in [#69](https://github.com/iearn-finance/yearn-vaults/pull/69). The ultimate intention here is to have an on-chain yield oracle, so the Vault can more effectively be used as a money lego for more interesting ideas.