feat: add minShares to vault withdraw

README.md

@@ -93,7 +93,7 @@ Any command `in code blocks` is meant to be executed from a Mac/Linux terminal o
       - You may have to install with `--ignore-engines` (try this if you get an error)
 14. Compile the Smart Contracts:
     - `brownie compile`
-15. `brownie test tests/functional/ -s -n auto` \* If everything worked, you'll see something like the following:
+15. `brownie test tests/functional/ -s -n auto` If everything worked, you'll see something like the following:
     ![Console](https://i.imgur.com/wGSmCrY.png)
 16. Launch VSCode
     - If you're in Windows using WSL, type `code .` to launch VSCode
@@ -115,7 +115,7 @@ Any command `in code blocks` is meant to be executed from a Mac/Linux terminal o
           //...prev configs...,
           "solidity.remappings": [
             "@openzeppelin=/home/<username>/.brownie/packages/OpenZeppelin/[email protected]"
-          ] 
+          ]
         }```
     - Set Black as the linter.
       - You'll see a toast notification the bottom right asking about linting, choose _black_
@@ -164,13 +164,13 @@ A brief explanation of flags:
 Check linter rules for `*.json` and `*.sol` files:
 
 ```bash
-yarn lint:check
+yarn format:check
 ```
 
 Fix linter errors for `*.json` and `*.sol` files:
 
 ```bash
-yarn lint:fix
+yarn format:fix
 ```
 
 Check linter rules for `*.py` files:
@@ -194,5 +194,5 @@ For security concerns, please visit [Bug Bounty](https://github.com/yearn/yearn-
 You can read more about Yearn Finance on our documentation [webpage](https://docs.yearn.finance).
 
 ## Discussion
- 
+
 For questions not covered in the docs, please visit [our Discord server](http://discord.yearn.finance).

contracts/Vault.vy

@@ -467,7 +467,7 @@ def setLockedProfitDegradation(degradation: uint256):
     # Since "degradation" is of type uint256 it can never be less than zero
     assert degradation <= DEGRADATION_COEFFICIENT
     self.lockedProfitDegradation = degradation
-    log LockedProfitDegradationUpdated(degradation) 
+    log LockedProfitDegradationUpdated(degradation)
 
 
 @external
@@ -926,7 +926,7 @@ def deposit(_amount: uint256 = MAX_UINT256, recipient: address = msg.sender) ->
 
     # Tokens are transferred from msg.sender (may be different from _recipient)
     self.erc20_safe_transferFrom(self.token.address, msg.sender, self, amount)
-    
+
     log Deposit(recipient, shares, amount)
 
     return shares  # Just in case someone wants them
@@ -960,7 +960,7 @@ def _sharesForAmount(amount: uint256) -> uint256:
         return  (
             amount
             * self.totalSupply
-            / _freeFunds 
+            / _freeFunds
         )
     else:
         return 0
@@ -1026,6 +1026,7 @@ def withdraw(
     maxShares: uint256 = MAX_UINT256,
     recipient: address = msg.sender,
     maxLoss: uint256 = 1,  # 0.01% [BPS]
+    minShares: uint256 = 1,
 ) -> uint256:
     """
     @notice
@@ -1069,22 +1070,26 @@ def withdraw(
     @param maxLoss
         The maximum acceptable loss to sustain on withdrawal. Defaults to 0.01%.
         If a loss is specified, up to that amount of shares may be burnt to cover losses on withdrawal.
+    @param minShares
+        Minimum number of shares to try and redeem for tokens, defaults to 1.
+        Revert if the final shares being redeemed is less than `minShares`.
+        If 0 is passed, the function will succeed even if no shares are redeemed.
     @return The quantity of tokens redeemed for `_shares`.
     """
-    shares: uint256 = maxShares  # May reduce this number below
-
     # Max Loss is <=100%, revert otherwise
     assert maxLoss <= MAX_BPS
 
+    shares: uint256 = maxShares  # May reduce this number below
+
     # If _shares not specified, transfer full share balance
     if shares == MAX_UINT256:
         shares = self.balanceOf[msg.sender]
 
     # Limit to only the shares they own
     assert shares <= self.balanceOf[msg.sender]
 
-    # Ensure we are withdrawing something
-    assert shares > 0
+    # Ensure we are withdrawing at least `minShares`
+    assert shares >= minShares
 
     # See @dev note, above.
     value: uint256 = self._shareValue(shares)
@@ -1141,6 +1146,7 @@ def withdraw(
             # 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)
+            assert shares >= minShares
 
         # NOTE: This loss protection is put in place to revert if losses from
         #       withdrawing are more than what is considered acceptable.
@@ -1154,7 +1160,7 @@ def withdraw(
     # Withdraw remaining balance to _recipient (may be different to msg.sender) (minus fee)
     self.erc20_safe_transfer(self.token.address, recipient, value)
     log Withdraw(recipient, shares, value)
-    
+
     return value
 
 

tests/functional/vault/test_withdrawal.py

@@ -92,7 +92,7 @@ def test_forced_withdrawal(
     assert token.balanceOf(vault) == 0
 
     # One of our strategies suffers a loss
-    total_assets = vault.totalAssets()
+    vault.totalAssets()
     loss = token.balanceOf(strategies[0]) // 2  # 10% of total
     common_health_check.setStrategyLimits(strategies[0], 5000, 5000, {"from": gov})
     strategies[0]._takeFunds(loss, {"from": gov})
@@ -247,12 +247,12 @@ def test_withdrawal_with_empty_queue(
     assert vault.balanceOf(gov) == strategy_balance
     assert token.balanceOf(gov) == free_balance
 
-    # Calling it a second time with strategy_balance should be a no-op
-    vault.withdraw(
-        strategy_balance * vault.pricePerShare() // 10 ** vault.decimals(),
-        {"from": gov},
-    )
-    assert token.balanceOf(gov) == free_balance
+    # Calling it a second time with strategy_balance should revert as we cannot withdraw more shares
+    with brownie.reverts():
+        vault.withdraw(
+            strategy_balance * vault.pricePerShare() // 10 ** vault.decimals(),
+            {"from": gov},
+        )
 
     # Re-establish the withdrawal queue
     vault.addStrategyToQueue(strategy, {"from": gov})
@@ -462,3 +462,69 @@ def test_withdraw_not_enough_funds_with_gains(
     priceAfter = vault.pricePerShare()
 
     assert priceBefore <= priceAfter  # with decimals=2 price remains the same.
+
+
+def test_withdraw_with_less_than_min_shares(
+    chain, token, gov, Vault, guardian, rewards, common_health_check, TestStrategy
+):
+    vault = guardian.deploy(Vault)
+    vault.initialize(
+        token,
+        gov,
+        rewards,
+        token.symbol() + " yVault",
+        "yv" + token.symbol(),
+        guardian,
+        common_health_check,
+    )
+    vault.setDepositLimit(2 ** 256 - 1, {"from": gov})
+
+    strategy = gov.deploy(TestStrategy, vault)
+    vault.addStrategy(strategy, 1000, 0, 10, 1000, {"from": gov})
+
+    # Withdraw at least 1 share, when total supply of shares is 0
+    with brownie.reverts():
+        vault.withdraw(1, {"from": gov})
+
+    # If `minShares` is explicitly set to 0, let the txn succeed.
+    shares = vault.withdraw(1, gov, 1, 0, {"from": gov})
+    assert shares == 0
+
+    token.approve(vault, 2 ** 256 - 1, {"from": gov})
+    vault.deposit(1000, {"from": gov})
+
+    # Withdraw when minShares > maxShares
+    with brownie.reverts():
+        vault.deposit(10, gov, 1, 20)
+
+    # Remove all tokens from gov to make asserts easier
+    token.approve(gov, 2 ** 256 - 1, {"from": gov})
+    token.transferFrom(gov, guardian, token.balanceOf(gov), {"from": gov})
+
+    chain.sleep(8640)
+    common_health_check.setDisabledCheck(strategy, True, {"from": gov})
+    strategy.harvest({"from": gov})
+
+    free_balance = token.balanceOf(vault)
+    strategy_balance = token.balanceOf(strategy)
+
+    # A few tokens have been transferred from vault to strategy
+    assert free_balance == 990
+    assert strategy_balance == 10
+
+    # Remove strategy so that the vault cannot withdraw from it
+    vault.removeStrategyFromQueue(strategy, {"from": gov})
+
+    assert (
+        vault.balanceOf(gov) == 1000 * (10 ** vault.decimals()) // vault.pricePerShare()
+    )
+
+    # Withdraw 991 tokens, but since only 990 tokens can be withdrawn, this reverts
+    with brownie.reverts():
+        vault.withdraw(
+            1000 * (10 ** vault.decimals()) // vault.pricePerShare(),
+            gov,
+            1,
+            991 * (10 ** vault.decimals()) // vault.pricePerShare(),
+            {"from": gov},
+        )