fix: debt withdraw (#175)

* fix: debt withdraw

* feat: use during redeem too

* fix: typos

* chore: use max redeem

* chore: comments

* test: unrealized loss tests

* feat: realize losses during debt updates

README.md

@@ -6,6 +6,8 @@ This repository contains the Smart Contracts for Yearns V3 vault implementation.
 
 [Vault.vy](contracts/VaultV3.vy) - The ERC4626 compliant Vault that will handle all logic associated with deposits, withdraws, strategy management, profit reporting etc.
 
+For the V3 strategy implementation see the [Tokenized Strategy](https://github.com/yearn/tokenized-strategy) repo.
+
 ## Requirements
 
 This repository runs on [ApeWorx](https://www.apeworx.io/). A python based development tool kit.

contracts/VaultV3.vy

@@ -41,13 +41,13 @@ interface IStrategy:
     def balanceOf(owner: address) -> uint256: view
     def maxDeposit(receiver: address) -> uint256: view
     def maxWithdraw(owner: address) -> uint256: view
-    def withdraw(amount: uint256, receiver: address, owner: address) -> uint256: nonpayable
     def redeem(shares: uint256, receiver: address, owner: address) -> uint256: nonpayable
     def deposit(assets: uint256, receiver: address) -> uint256: nonpayable
     def totalAssets() -> (uint256): view
     def convertToAssets(shares: uint256) -> uint256: view
     def convertToShares(assets: uint256) -> uint256: view
     def previewWithdraw(assets: uint256) -> uint256: view
+    def maxRedeem(owner: address) -> uint256: view
 
 interface IAccountant:
     def report(strategy: address, gain: uint256, loss: uint256) -> (uint256, uint256): nonpayable
@@ -638,6 +638,24 @@ def _assess_share_of_unrealised_losses(strategy: address, assets_needed: uint256
 
     return losses_user_share
 
+@internal
+def _withdraw_from_strategy(strategy: address, assets_to_withdraw: uint256):
+    """
+    This takes the amount denominated in asset and performs a {redeem}
+    with the corresponding amount of shares.
+
+    We use {redeem} to natively take on losses without aditional non-4626 standard parameters.
+    """
+    # Need to get shares since we use redeem to be able to take on losses.
+    shares_to_redeem: uint256 = min(
+        # Use previewWithdraw since it should round up.
+        IStrategy(strategy).previewWithdraw(assets_to_withdraw), 
+        # And check against the max we can pull.
+        IStrategy(strategy).maxRedeem(self)
+    )
+    # Redeem the shares.
+    IStrategy(strategy).redeem(shares_to_redeem, self, self)
+
 @internal
 def _redeem(
     sender: address, 
@@ -760,20 +778,13 @@ def _redeem(
                 continue
 
             # WITHDRAW FROM STRATEGY
-            # Need to get shares since we use redeem to be able to take on losses.
-            shares_to_withdraw: uint256 = min(
-                # Use previewWithdraw since it should round up.
-                IStrategy(strategy).previewWithdraw(assets_to_withdraw), 
-                # And check against our actual balance.
-                IStrategy(strategy).balanceOf(self)
-            )
-            IStrategy(strategy).redeem(shares_to_withdraw, self, self)
+            self._withdraw_from_strategy(strategy, assets_to_withdraw)
             post_balance: uint256 = ASSET.balanceOf(self)
 
             # Always check withdrawn against the real amounts.
             withdrawn: uint256 = post_balance - previous_balance
             loss: uint256 = 0
-            # Check if we redeemed to much.
+            # Check if we redeemed too much.
             if withdrawn > assets_to_withdraw:
                 # Make sure we don't underlfow in debt updates.
                 if withdrawn > current_debt:
@@ -819,8 +830,8 @@ def _redeem(
 
     # Check if there is a loss and a non-default value was set.
     if assets > requested_assets and max_loss < MAX_BPS:
-        # The loss is within the allowed range.
-        assert assets - requested_assets <= assets * max_loss / MAX_BPS, "to much loss"
+        # Assure the loss is within the allowed range.
+        assert assets - requested_assets <= assets * max_loss / MAX_BPS, "too much loss"
 
     # First burn the corresponding shares from the redeemer.
     self._burn_shares(shares, owner)
@@ -941,18 +952,22 @@ def _update_debt(strategy: address, target_debt: uint256) -> uint256:
 
         # Always check the actual amount withdrawn.
         pre_balance: uint256 = ASSET.balanceOf(self)
-        IStrategy(strategy).withdraw(assets_to_withdraw, self, self)
+        self._withdraw_from_strategy(strategy, assets_to_withdraw)
         post_balance: uint256 = ASSET.balanceOf(self)
 
-        # making sure we are changing according to the real result no matter what. 
-        # This will spend more gas but makes it more robust. Also prevents issues
-        # from a faulty strategy that either under or over delievers 'assets_to_withdraw'
-        assets_to_withdraw = min(post_balance - pre_balance, current_debt)
+        # making sure we are changing idle according to the real result no matter what. 
+        # We pull funds with {redeem} so there can be losses or rounding differences.
+        withdrawn: uint256 = min(post_balance - pre_balance, current_debt)
+
+        # If we got too much make sure not to increase PPS.
+        if withdrawn > assets_to_withdraw:
+            assets_to_withdraw = withdrawn
 
         # Update storage.
-        self.total_idle += assets_to_withdraw
-        self.total_debt -= assets_to_withdraw
-
+        self.total_idle += withdrawn # actual amount we got.
+        # Amount we tried to withdraw in case of losses
+        self.total_debt -= assets_to_withdraw 
+
         new_debt = current_debt - assets_to_withdraw
     else: 
         # We are increasing the strategies debt

contracts/test/mocks/ERC4626/BaseStrategyMock.sol

@@ -38,7 +38,7 @@ abstract contract ERC4626BaseStrategyMock is ERC4626BaseStrategy {
         return _maxDebt > _totalAssets ? _maxDebt - _totalAssets : 0;
     }
 
-    function totalAssets() public view override returns (uint256) {
+    function totalAssets() public view virtual override returns (uint256) {
         return IERC20(asset()).balanceOf(address(this));
     }
 

contracts/test/mocks/ERC4626/FaultyStrategy.sol

@@ -58,4 +58,22 @@ contract ERC4626FaultyStrategy is ERC4626BaseStrategyMock {
 
         return shares;
     }
+
+    function redeem(
+        uint256 _shares,
+        address _receiver,
+        address _owner
+    ) public override returns (uint256) {
+        require(
+            _shares <= maxRedeem(_owner),
+            "ERC4626: withdraw more than max"
+        );
+
+        // this will simulate withdrawing less than the vault wanted to
+        uint256 toRedeem = _shares / 2;
+        uint256 assets = previewRedeem(toRedeem);
+        _withdraw(_msgSender(), _receiver, _owner, toRedeem, assets);
+
+        return assets;
+    }
 }

contracts/test/mocks/ERC4626/LockedStrategy.sol

@@ -49,6 +49,16 @@ contract ERC4626LockedStrategy is ERC4626BaseStrategyMock {
         }
     }
 
+    function maxRedeem(address) public view override returns (uint256) {
+        uint256 balance = IERC20(asset()).balanceOf(address(this));
+        if (block.timestamp < lockedUntil) {
+            return convertToShares(balance - lockedBalance);
+        } else {
+            // no locked assets, withdraw all
+            return convertToShares(balance);
+        }
+    }
+
     function migrate(address _newStrategy) external override {
         require(lockedBalance == 0, "strat not liquid");
         IERC20(asset()).safeTransfer(

contracts/test/mocks/ERC4626/LossyStrategy.sol

@@ -7,7 +7,8 @@ import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol
 contract ERC4626LossyStrategy is ERC4626BaseStrategyMock {
     using SafeERC20 for IERC20;
 
-    uint256 public withdrawingLoss;
+    int256 public withdrawingLoss;
+    uint256 public lockedFunds;
 
     constructor(
         address _vault,
@@ -19,10 +20,22 @@ contract ERC4626LossyStrategy is ERC4626BaseStrategyMock {
         IERC20(asset()).safeTransfer(_target, _loss);
     }
 
-    function setWithdrawingLoss(uint256 _loss) external {
+    function setWithdrawingLoss(int256 _loss) external {
         withdrawingLoss = _loss;
     }
 
+    function setLockedFunds(uint256 _lockedFunds) external {
+        lockedFunds = _lockedFunds;
+    }
+
+    function totalAssets() public view override returns (uint256) {
+        if (withdrawingLoss < 0) {
+            return uint256(int256(IERC20(asset()).balanceOf(address(this))) + withdrawingLoss);
+        } else {
+            return super.totalAssets();
+        }
+    }
+
     function _withdraw(
         address _caller,
         address _receiver,
@@ -34,17 +47,20 @@ contract ERC4626LossyStrategy is ERC4626BaseStrategyMock {
             _spendAllowance(_owner, _caller, _shares);
         }
 
+        uint256 toWithdraw = uint256(int256(_assets) - withdrawingLoss);
         _burn(_owner, _shares);
         // Withdrawing loss simulates a loss while withdrawing
-        IERC20(asset()).safeTransfer(_receiver, _assets - withdrawingLoss);
-        // burns (to simulate loss while withdrawing)
-        IERC20(asset()).safeTransfer(asset(), withdrawingLoss);
+        IERC20(asset()).safeTransfer(_receiver, toWithdraw);
+        if(withdrawingLoss > 0) {
+            // burns (to simulate loss while withdrawing)
+            IERC20(asset()).safeTransfer(asset(), uint256(withdrawingLoss));
+        }
 
         emit Withdraw(
             _caller,
             _receiver,
             _owner,
-            _assets - withdrawingLoss,
+            toWithdraw,
             _shares
         );
     }
@@ -54,7 +70,7 @@ contract ERC4626LossyStrategy is ERC4626BaseStrategyMock {
     ) internal override returns (uint256 _amountFreed) {}
 
     function maxWithdraw(address) public view override returns (uint256) {
-        return IERC20(asset()).balanceOf(address(this));
+        return IERC20(asset()).balanceOf(address(this)) - lockedFunds;
     }
 
     function migrate(address _newStrategy) external override {