fix: audit comments yearn#3 (yearn#168)

* fix: too few shares are burned in case of loss w/ adjusted withdrawal Audit Response yearn#2 - 5.1 * fix: check loss protection after adjusting for withdrawal capacity Audit Response yearn#2 - 5.2 * docs: clarify documentation around revoking during emergency exit mode * fix: make expected return 0 if strategy is declaring itself inactive Audit Response yearn#2 - 8.4 * fix: use strategy's lastReport value on migration, not block.timestamp Audit Reponse yearn#2 - 8.5 * fix: increase precision on price calc by 3 places Audit Reponse yearn#2 - 8.7 * refactor: small updates to comments and StrategyAPI functions Audit Response yearn#2 - 5.5 * refactor: improve the accuracy of `expectedReturn` Audit Reponse yearn#2 - 5.6 * docs: clarified requirement for Emergency Shutdown * docs: updated requirement Audit Reponse yearn#2 - 5.12 * fix: add reentrancy guard (using same key as "withdraw") to "deposit" Audit Reponse yearn#2 - 5.19
orbxball · Jan 28, 2021 · 9d90ef3 · 9d90ef3
1 parent cfb37d6
commit 9d90ef3
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 28 deletions.
diff --git a/SPECIFICATION.md b/SPECIFICATION.md
@@ -22,6 +22,8 @@ NOTE: Users should only withdraw "free" assets as some strategies need to divest
 
 NOTE: "free" assets in the Vault means the amount the Vault has freely available (e.g. not invested in Strategies)
 
+NOTE: The strategy can also revoke itself during normal operation of the Vault, but this behavior only occurs during "Emergency Exit" mode.
+
 1. A User is able to deposit an amount of the single asset the Vault accepts in exchange for shares of the Vault's underlying assets, depending on the Vault's deposit configuration.
 1. A User is able to withdraw an amount of tokens represented by their shares up to the total available amount that can be withdrawn from the combination of the Vault's overhead, and what can be forcibly withdrawn from all the strategies with debt to the Vault that have been pre-authorized for withdrawals in the Vault's withdrawal queue.
 1. A User is able to transfer any amount of their Vault shares to anyone else.
@@ -46,7 +48,7 @@ NOTE: During Emergency Shutdown mode of the Vault, the intention is for the Vaul
 
 1. During Emergency Shutdown, no Users may deposit into the Vault.
 1. During Emergency Shutdown, Governance cannot add new Strategies.
-1. During Emergency Shutdown, each Strategy must pay back their debt as quickly as reasonable, with minimally impact to their positions.
+1. During Emergency Shutdown, the Vault should communicate to each Strategy to return their entire position as quickly as reasonable, with minimal impact to their positions (Normal Operation).
 1. Only Governance can exit Emergency Shutdown Mode.
 
 ## Strategy Specification
@@ -74,7 +76,7 @@ NOTE: In this mode, the Strategy defines a reversionary set of actions that seek
 
 1. During Emergency Exit, the Strategy cannot take new debt from the connected Vault.
 1. During Emergency Exit, the Strategy can still interact with any external system, but must be able to handle any failure(s) of each of those system(s) as well as possible.
-1. Only Governance can exit Emergency Exit Mode.
+1. There is no condition that can revert Emergency Exit Mode once it is entered.
 
 ## Registry Specification
 

diff --git a/contracts/BaseStrategy.sol b/contracts/BaseStrategy.sol
@@ -82,17 +82,21 @@ interface VaultAPI is IERC20 {
  * This interface is here for the keeper bot to use.
  */
 interface StrategyAPI {
+    function name() external view returns (string memory);
+
+    function vault() external view returns (address);
+
+    function want() external view returns (address);
+
     function apiVersion() external pure returns (string memory);
 
+    function keeper() external view returns (address);
+
     function isActive() external view returns (bool);
 
     function delegatedAssets() external virtual view returns (uint256);
 
-    function name() external view returns (string memory);
-
-    function vault() external view returns (address);
-
-    function keeper() external view returns (address);
+    function estimatedTotalAssets() external virtual view returns (uint256);
 
     function tendTrigger(uint256 callCost) external view returns (bool);
 
@@ -615,7 +619,7 @@ abstract contract BaseStrategy {
      */
     function withdraw(uint256 _amountNeeded) external returns (uint256 _loss) {
         require(msg.sender == address(vault), "!vault");
-        // Liquidate as much as possible to `want`, up to `_amount`
+        // Liquidate as much as possible to `want`, up to `_amountNeeded`
         uint256 amountFreed;
         (amountFreed, _loss) = liquidatePosition(_amountNeeded);
         // Send it directly back (NOTE: Using `msg.sender` saves some gas here)

diff --git a/contracts/Vault.vy b/contracts/Vault.vy
@@ -51,6 +51,7 @@ interface DetailedERC20:
 interface Strategy:
     def want() -> address: view
     def vault() -> address: view
+    def isActive() -> bool: view
     def estimatedTotalAssets() -> uint256: view
     def withdraw(_amount: uint256) -> uint256: nonpayable
     def migrate(_newStrategy: address): nonpayable
@@ -742,6 +743,7 @@ def _issueSharesForAmount(to: address, amount: uint256) -> uint256:
 
 
 @external
+@nonreentrant("withdraw")
 def deposit(_amount: uint256 = MAX_UINT256, recipient: address = msg.sender) -> uint256:
     """
     @notice
@@ -811,8 +813,9 @@ def deposit(_amount: uint256 = MAX_UINT256, recipient: address = msg.sender) ->
 @internal
 def _shareValue(shares: uint256) -> uint256:
     # Determines the current value of `shares`.
-        # NOTE: if sqrt(Vault.totalAssets()) >>> 1e39, this could potentially revert
-    return (shares * (self._totalAssets())) / self.totalSupply
+    # NOTE: if sqrt(Vault.totalAssets()) > 1e37, this could potentially revert
+    # NOTE: using 1e3 for extra precision here, when decimals is low
+    return ((10 ** 3 * (shares * self._totalAssets())) / self.totalSupply) / 10 ** 3
 
 
 @view
@@ -821,8 +824,8 @@ def _sharesForAmount(amount: uint256) -> uint256:
     # Determines how many shares `amount` of token would receive.
     # See dev note on `deposit`.
     if self._totalAssets() > 0:
-        # NOTE: if sqrt(token.totalSupply()) > 1e39, this could potentially revert
-        return (amount * self.totalSupply) / self._totalAssets()
+        # NOTE: if sqrt(token.totalSupply()) > 1e37, this could potentially revert
+        return ((10 ** 3 * (amount * self.totalSupply)) / self._totalAssets()) / 10 ** 3
     else:
         return 0
 
@@ -898,7 +901,7 @@ def withdraw(
         The address to issue the shares in this Vault to. Defaults to the
         caller's address.
     @param maxLoss
-        The maximum acceptable loss to sustain on withdrawal. Defaults to 0%.
+        The maximum acceptable loss to sustain on withdrawal. Defaults to 0.01%.
     @return The quantity of tokens redeemed for `_shares`.
     """
     shares: uint256 = maxShares  # May reduce this number below
@@ -913,6 +916,7 @@ def withdraw(
     # See @dev note, above.
     value: uint256 = self._shareValue(shares)
 
+    totalLoss: uint256 = 0
     if value > self.token.balanceOf(self):
         # We need to go get some from our strategies in the withdrawal queue
         # NOTE: This performs forced withdrawals from each Strategy. During
@@ -922,7 +926,6 @@ def withdraw(
         #       can optionally specify the maximum acceptable loss (in BPS)
         #       to prevent excessive losses on their withdrawals (which may
         #       happen in certain edge cases where Strategies realize a loss)
-        totalLoss: uint256 = 0
         for strategy in self.withdrawalQueue:
             if strategy == ZERO_ADDRESS:
                 break  # We've exhausted the queue
@@ -956,17 +959,19 @@ def withdraw(
             self.strategies[strategy].totalDebt -= withdrawn + loss
             self.totalDebt -= withdrawn + loss
 
-        # NOTE: This loss protection is put in place to revert if losses from
-        #       withdrawing are more than what is considered acceptable.
-        assert totalLoss <= maxLoss * (value + totalLoss) / MAX_BPS
-
     # NOTE: We have withdrawn everything possible out of the withdrawal queue
     #       but we still don't have enough to fully pay them back, so adjust
     #       to the total amount we've freed up through forced withdrawals
     vault_balance: uint256 = self.token.balanceOf(self)
     if value > vault_balance:
         value = vault_balance
-        shares = self._sharesForAmount(value)
+        # NOTE: Burn # of shares that corresponds to what Vault has on-hand,
+        #       including the losses that were incurred above during withdrawals
+        shares = self._sharesForAmount(value + totalLoss)
+
+    # NOTE: This loss protection is put in place to revert if losses from
+    #       withdrawing are more than what is considered acceptable.
+    assert totalLoss <= maxLoss * (value + totalLoss) / MAX_BPS
 
     # Burn shares (full value of what is being withdrawn)
     self.totalSupply -= shares
@@ -1025,7 +1030,8 @@ def addStrategy(
         The Strategy will be appended to `withdrawalQueue`, call
         `setWithdrawalQueue` to change the order.
     @param strategy The address of the Strategy to add.
-    @param debtRatio The ratio of the total assets in the `vault that the `strategy` can manage.
+    @param debtRatio
+        The share of the total assets in the `vault that the `strategy` has access to.
     @param rateLimit
         Limit on the increase of debt per unit time since last harvest
     @param performanceFee
@@ -1171,10 +1177,11 @@ def migrateStrategy(oldVersion: address, newVersion: address):
 
     self.strategies[newVersion] = StrategyParams({
         performanceFee: strategy.performanceFee,
-        activation: block.timestamp,
+        # NOTE: use last report for activation time, so E[R] calc works
+        activation: strategy.lastReport,
         debtRatio: strategy.debtRatio,
         rateLimit: strategy.rateLimit,
-        lastReport: block.timestamp,
+        lastReport: strategy.lastReport,
         totalDebt: strategy.totalDebt,
         totalGain: 0,
         totalLoss: 0,
@@ -1350,13 +1357,15 @@ def creditAvailable(strategy: address = msg.sender) -> uint256:
 @internal
 def _expectedReturn(strategy: address) -> uint256:
     # See note on `expectedReturn()`.
-    delta: uint256 = block.timestamp - self.strategies[strategy].lastReport
-    if delta > 0:
+    strategy_lastReport: uint256 = self.strategies[strategy].lastReport
+    timeSinceLastHarvest: uint256 = block.timestamp - strategy_lastReport
+    totalHarvestTime: uint256 = strategy_lastReport - self.strategies[strategy].activation
+
+    # NOTE: If either `timeSinceLastHarvest` or `totalHarvestTime` is 0, we can short-circuit to `0`
+    if timeSinceLastHarvest > 0 and totalHarvestTime > 0 and Strategy(strategy).isActive():
         # NOTE: Unlikely to throw unless strategy accumalates >1e68 returns
-        # NOTE: Will not throw for DIV/0 because activation <= lastReport
-        return (self.strategies[strategy].totalGain * delta) / (
-            block.timestamp - self.strategies[strategy].activation
-        )
+        # NOTE: Calculate average over period of time where harvests have occured in the past
+        return (self.strategies[strategy].totalGain * timeSinceLastHarvest) / totalHarvestTime
     else:
         return 0  # Covers the scenario when block.timestamp == activation