feat: minor fixes (#194)

* fix: redeem corrections

* chore: dont burn zero shares

* fix: use updated strategy storage

* fix: rebase

* chore: bump oz version

* fix: oz 4626 fix

* fix: lossy test

* fix: round down in max redeem

* fix: comment

ape-config.yaml

@@ -9,15 +9,15 @@ default_ecosystem: ethereum
 dependencies:
   - name: openzeppelin
     github: OpenZeppelin/openzeppelin-contracts
-    ref: 4.7.3
+    ref: 4.9.5
   - name: tokenized-strategy
     github: yearn/tokenized-strategy
     ref: dev_302
     contracts_folder: src
 
 solidity:
   import_remapping:
-    - "@openzeppelin/contracts=openzeppelin/v4.7.3"
+    - "@openzeppelin/contracts=openzeppelin/v4.9.5"
     - "@tokenized-strategy=tokenized-strategy/dev_302"
 
 ethereum:

contracts/VaultV3.vy

@@ -729,8 +729,11 @@ def _mint(sender: address, recipient: address, shares: uint256) -> uint256:
 def _assess_share_of_unrealised_losses(strategy: address, assets_needed: uint256) -> uint256:
     """
     Returns the share of losses that a user would take if withdrawing from this strategy
+    This accounts for losses that have been realized at the strategy level but not yet
+    realized at the vault level.
+
     e.g. if the strategy has unrealised losses for 10% of its current debt and the user 
-    wants to withdraw 1000 tokens, the losses that he will take are 100 token
+    wants to withdraw 1_000 tokens, the losses that they will take is 100 token
     """
     # Minimum of how much debt the debt should be worth.
     strategy_current_debt: uint256 = self.strategies[strategy].current_debt
@@ -746,12 +749,12 @@ def _assess_share_of_unrealised_losses(strategy: address, assets_needed: uint256
     # but will only receive assets_to_withdraw.
     # NOTE: If there are unrealised losses, the user will take his share.
     numerator: uint256 = assets_needed * strategy_assets
-    losses_user_share: uint256 = assets_needed - numerator / strategy_current_debt
+    users_share_of_loss: uint256 = assets_needed - numerator / strategy_current_debt
     # Always round up.
     if numerator % strategy_current_debt != 0:
-        losses_user_share += 1
+        users_share_of_loss += 1
 
-    return losses_user_share
+    return users_share_of_loss
 
 @internal
 def _withdraw_from_strategy(strategy: address, assets_to_withdraw: uint256):
@@ -777,38 +780,37 @@ def _redeem(
     receiver: address, 
     owner: address,
     assets: uint256,
-    shares_to_burn: uint256, 
+    shares: uint256, 
     max_loss: uint256,
     strategies: DynArray[address, MAX_QUEUE]
 ) -> uint256:
     """
     This will attempt to free up the full amount of assets equivalent to
-    `shares_to_burn` and transfer them to the `receiver`. If the vault does
+    `shares` and transfer them to the `receiver`. If the vault does
     not have enough idle funds it will go through any strategies provided by
-    either the withdrawer or the queue_manager to free up enough funds to 
+    either the withdrawer or the default_queue to free up enough funds to 
     service the request.
 
     The vault will attempt to account for any unrealized losses taken on from
     strategies since their respective last reports.
 
     Any losses realized during the withdraw from a strategy will be passed on
-    to the user that is redeeming their vault shares.
+    to the user that is redeeming their vault shares unless it exceeds the given
+    `max_loss`.
     """
     assert receiver != empty(address), "ZERO ADDRESS"
+    assert shares > 0, "no shares to redeem"
+    assert assets > 0, "no assets to withdraw"
     assert max_loss <= MAX_BPS, "max loss"
-
+    
     # If there is a withdraw limit module, check the max.
     if self.withdraw_limit_module != empty(address):
         assert assets <= self._max_withdraw(owner, max_loss, strategies), "exceed withdraw limit"
 
-    shares: uint256 = shares_to_burn
-    shares_balance: uint256 = self.balance_of[owner]
-
-    assert shares > 0, "no shares to redeem"
-    assert shares_balance >= shares, "insufficient shares to redeem"
+    assert self.balance_of[owner] >= shares, "insufficient shares to redeem"
 
     if sender != owner:
-        self._spend_allowance(owner, sender, shares_to_burn)
+        self._spend_allowance(owner, sender, shares)
 
     # The amount of the underlying token to withdraw.
     requested_assets: uint256 = assets
@@ -852,7 +854,7 @@ def _redeem(
             assets_to_withdraw = min(assets_needed, current_debt)
 
             # Cache max_withdraw now for use if unrealized loss > 0
-            # Use maxRedeem and convert since we use redeem.
+            # Use maxRedeem and convert it since we use redeem.
             max_withdraw: uint256 = IStrategy(strategy).convertToAssets(
                 IStrategy(strategy).maxRedeem(self)
             )
@@ -905,7 +907,7 @@ def _redeem(
             self._withdraw_from_strategy(strategy, assets_to_withdraw)
             post_balance: uint256 = ERC20(_asset).balanceOf(self)
 
-            # Always check withdrawn against the real amounts.
+            # Always check against the real amounts.
             withdrawn: uint256 = post_balance - previous_balance
             loss: uint256 = 0
             # Check if we redeemed too much.
@@ -1461,8 +1463,12 @@ def setProfitMaxUnlockTime(new_profit_max_unlock_time: uint256):
 
     # If setting to 0 we need to reset any locked values.
     if (new_profit_max_unlock_time == 0):
-        # Burn any shares the vault still has.
-        self._burn_shares(self.balance_of[self], self)
+
+        share_balance: uint256 = self.balance_of[self]
+        if share_balance > 0:
+            # Burn any shares the vault still has.
+            self._burn_shares(share_balance, self)
+
         # Reset unlocking variables to 0.
         self.profit_unlocking_rate = 0
         self.full_profit_unlock_date = 0
@@ -2006,8 +2012,8 @@ def maxRedeem(
     @return The maximum amount of shares that can be redeemed.
     """
     return min(
-        # Convert to shares is rounding up so we check against the full balance.
-        self._convert_to_shares(self._max_withdraw(owner, max_loss, strategies), Rounding.ROUND_UP),
+        # Min of the shares equivalent of max_withdraw or the full balance
+        self._convert_to_shares(self._max_withdraw(owner, max_loss, strategies), Rounding.ROUND_DOWN),
         self.balance_of[owner]
     )
 

contracts/test/ERC4626BaseStrategy.sol

@@ -20,7 +20,7 @@ abstract contract ERC4626BaseStrategy is ERC4626 {
     constructor(
         address _vault,
         address _asset
-    ) ERC4626(IERC20Metadata(address(_asset))) {
+    ) ERC4626(IERC20(address(_asset))) {
         _initialize(_vault, _asset);
     }
 
@@ -30,13 +30,7 @@ abstract contract ERC4626BaseStrategy is ERC4626 {
         vault = _vault;
     }
 
-    function decimals()
-        public
-        view
-        virtual
-        override(ERC20, IERC20Metadata)
-        returns (uint8)
-    {
+    function decimals() public view virtual override returns (uint8) {
         return _decimals;
     }
 

package.json

@@ -1,7 +1,7 @@
 {
     "name": "yearn-vaults-v3",
     "devDependencies": {
-        "@openzeppelin/contracts": "^4.7.3",
+        "@openzeppelin/contracts": "^4.9.0",
         "hardhat": "^2.12.2",
         "prettier": "^2.6.0",
         "prettier-plugin-solidity": "^1.0.0-beta.19",

tests/unit/vault/test_strategy_withdraw.py

@@ -1807,7 +1807,7 @@ def test_redeem__half_of_strategy_assets_from_locked_lossy_strategy_with_unreali
     asset,
     create_vault,
     create_strategy,
-    create_locked_strategy,
+    create_lossy_strategy,
     user_deposit,
     add_strategy_to_vault,
     add_debt_to_strategy,
@@ -1822,7 +1822,7 @@ def test_redeem__half_of_strategy_assets_from_locked_lossy_strategy_with_unreali
     )  # withdraw a quarter deposit (half of strategy debt)
     shares = amount
     liquid_strategy = create_strategy(vault)
-    lossy_strategy = create_locked_strategy(vault)
+    lossy_strategy = create_lossy_strategy(vault)
     strategies = [lossy_strategy, liquid_strategy]
     max_loss = 10_000
 
@@ -1840,9 +1840,9 @@ def test_redeem__half_of_strategy_assets_from_locked_lossy_strategy_with_unreali
         add_debt_to_strategy(gov, strategy, vault, amount_per_strategy)
 
     # lose half of assets in lossy strategy
-    asset.transfer(gov, amount_to_lose, sender=lossy_strategy)
+    lossy_strategy.setLoss(gov, amount_to_lose, sender=lossy_strategy)
     # Lock half the remaining funds.
-    lossy_strategy.setLockedFunds(amount_to_lock, DAY, sender=gov)
+    lossy_strategy.setLockedFunds(amount_to_lock, sender=gov)
 
     tx = vault.redeem(
         amount_to_withdraw,
@@ -1861,7 +1861,7 @@ def test_redeem__half_of_strategy_assets_from_locked_lossy_strategy_with_unreali
 
     event = list(tx.decode_logs(vault.Withdraw))
 
-    assert len(event) >= 1
+    assert len(event) > 1
     n = len(event) - 1
     assert event[n].sender == fish
     assert event[n].receiver == fish
@@ -1889,7 +1889,7 @@ def test_redeem__half_of_strategy_assets_from_locked_lossy_strategy_with_unreali
     assert asset.balanceOf(vault) == 0
     assert asset.balanceOf(liquid_strategy) == amount_per_strategy - expected_liquid_out
     assert (
-        asset.balanceOf(lossy_strategy)
+        asset.balanceOf(lossy_strategy.yieldSource())
         == amount_per_strategy - amount_to_lose - expected_locked_out
     )  # withdrawn from strategy
     assert (