From 6aeee9c3e30759a2334eff2e29c8b912ffbecd70 Mon Sep 17 00:00:00 2001 From: gmridul Date: Fri, 21 Jan 2022 16:25:18 +0000 Subject: [PATCH 1/2] feat: add minShares to vault withdraw --- contracts/Vault.vy | 22 ++++--- tests/functional/vault/test_withdrawal.py | 80 +++++++++++++++++++++-- 2 files changed, 87 insertions(+), 15 deletions(-) diff --git a/contracts/Vault.vy b/contracts/Vault.vy index 38bf4800..f5a7c871 100644 --- a/contracts/Vault.vy +++ b/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,13 +1070,17 @@ 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] @@ -1083,8 +1088,8 @@ def withdraw( # 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 diff --git a/tests/functional/vault/test_withdrawal.py b/tests/functional/vault/test_withdrawal.py index def06ec4..96811d54 100644 --- a/tests/functional/vault/test_withdrawal.py +++ b/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}, + ) From e03fb7a1851c01fc6244582bcdfffbb02d8442b6 Mon Sep 17 00:00:00 2001 From: gmridul Date: Fri, 21 Jan 2022 18:16:43 +0000 Subject: [PATCH 2/2] fix: update README for lint instructions --- README.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index a824c8f5..730edcc7 100644 --- a/README.md +++ b/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//.brownie/packages/OpenZeppelin/openzeppelin-contracts@3.1.0" - ] + ] }``` - 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).