From 44a16e6287caf96adf01335c87521185b23e8616 Mon Sep 17 00:00:00 2001 From: Schlagonia Date: Thu, 31 Aug 2023 15:00:53 -0600 Subject: [PATCH 1/4] fix: withdraw limits --- contracts/VaultV3.vy | 92 ++++++++++++++++++++++++++++---- tests/unit/vault/test_erc4626.py | 4 +- 2 files changed, 84 insertions(+), 12 deletions(-) diff --git a/contracts/VaultV3.vy b/contracts/VaultV3.vy index b429e4c3..e915adf0 100644 --- a/contracts/VaultV3.vy +++ b/contracts/VaultV3.vy @@ -551,13 +551,71 @@ def _max_deposit(receiver: address) -> uint256: @view @internal -def _max_redeem(owner: address) -> uint256: - return self.balance_of[owner] +def _max_withdraw( + owner: address, + max_loss: uint256 = 0, + strategies: DynArray[address, MAX_QUEUE] = [] +) -> uint256: + """ + @dev Returns the max amount of `asset` and `owner` can withdraw. -@view -@internal -def _max_withdraw(owner: address) -> uint256: - return self._convert_to_assets(self.balance_of[owner], Rounding.ROUND_DOWN) + This will do a full simulation of the withdraw in order to determine + how much is currently liquid and if the `max_loss` would allow for the + tx to not revert. + """ + # Get the max amount for the owner if fully liquid. + max_assets: uint256 = self._convert_to_assets(self.balance_of[owner], Rounding.ROUND_DOWN) + + # See if we have enough idle to service the withdraw. + current_idle: uint256 = self.total_idle + if max_assets > current_idle: + # Amount left that we need. + needed: uint256 = max_assets - current_idle + # Track how much we can pull. + have: uint256 = current_idle + loss: uint256 = 0 + + # If no queue was passed use the default one. + _strategies: DynArray[address, MAX_QUEUE] = strategies + if len(_strategies) == 0: + _strategies = self.default_queue + + for strategy in _strategies: + # Can't use an invalid strategy. + assert self.strategies[strategy].activation != 0, "inactive strategy" + + # Get the maximum amount the vault can withdraw from the strategy. + max_withdraw: uint256 = IStrategy(strategy).maxWithdraw(self) + + # If 0 move on to the next strategy. + if max_withdraw == 0: + continue + + # If we more than what we need, adjust it down. + if max_withdraw > needed: + max_withdraw = needed + + # Add to what we can pull. + have += max_withdraw + # Reduce how much we have left. + needed -= max_withdraw + # Track how much loss would be passed on to the owner. + loss += self._assess_share_of_unrealised_losses(strategy, max_withdraw) + + if needed == 0: + break + + # Update the max based after going through the queue. + max_assets = have + + # Check if there is a loss and a non-default value was set. + if loss > 0 and max_loss < MAX_BPS: + # If the loss is not within the allowed range. + if loss > max_assets * max_loss / MAX_BPS: + # The tx will revert. Can only withdraw idle. + max_assets = current_idle + + return max_assets @internal def _deposit(sender: address, recipient: address, assets: uint256) -> uint256: @@ -1798,23 +1856,37 @@ def maxMint(receiver: address) -> uint256: @view @external -def maxWithdraw(owner: address) -> uint256: +def maxWithdraw( + owner: address, + max_loss: uint256 = 0, + strategies: DynArray[address, MAX_QUEUE] = [] +) -> uint256: """ @notice Get the maximum amount of assets that can be withdrawn. + @dev Complies to normal 4626 interface and takes custom params. @param owner The address that owns the shares. + @param max_loss Custom max_loss if any. + @param strategies Custom strategies queue if any. @return The maximum amount of assets that can be withdrawn. """ - return self._max_withdraw(owner) + return self._max_withdraw(owner, max_loss, strategies) @view @external -def maxRedeem(owner: address) -> uint256: +def maxRedeem( + owner: address, + max_loss: uint256 = 0, + strategies: DynArray[address, MAX_QUEUE] = [] +) -> uint256: """ @notice Get the maximum amount of shares that can be redeemed. + @dev Complies to normal 4626 interface and takes custom params. @param owner The address that owns the shares. + @param max_loss Custom max_loss if any. + @param strategies Custom strategies queue if any. @return The maximum amount of shares that can be redeemed. """ - return self._max_redeem(owner) + return self._convert_to_shares(self._max_withdraw(owner, max_loss, strategies), Rounding.ROUND_DOWN) @view @external diff --git a/tests/unit/vault/test_erc4626.py b/tests/unit/vault/test_erc4626.py index c7a59017..5c4b57fb 100644 --- a/tests/unit/vault/test_erc4626.py +++ b/tests/unit/vault/test_erc4626.py @@ -193,7 +193,7 @@ def test_max_redeem__with_balance_greater_than_total_idle__returns_balance( add_strategy_to_vault(gov, strategy, vault) add_debt_to_strategy(gov, strategy, vault, strategy_deposit) - assert vault.maxWithdraw(fish.address) == assets + assert vault.maxRedeem(fish.address) == assets def test_max_redeem__with_balance_less_than_or_equal_to_total_idle__returns_balance( @@ -205,4 +205,4 @@ def test_max_redeem__with_balance_less_than_or_equal_to_total_idle__returns_bala user_deposit(fish, vault, asset, shares) - assert vault.maxWithdraw(fish.address) == assets + assert vault.maxRedeem(fish.address) == assets From 9c8e360778805d9978cbe14e087589e6e294a7e7 Mon Sep 17 00:00:00 2001 From: Schlagonia Date: Thu, 31 Aug 2023 16:34:31 -0600 Subject: [PATCH 2/4] test: max withdraw --- contracts/VaultV3.vy | 12 +- tests/unit/vault/test_erc4626.py | 201 ++++++++++++++++++++++++++++++- 2 files changed, 207 insertions(+), 6 deletions(-) diff --git a/contracts/VaultV3.vy b/contracts/VaultV3.vy index e915adf0..b7e85863 100644 --- a/contracts/VaultV3.vy +++ b/contracts/VaultV3.vy @@ -553,8 +553,8 @@ def _max_deposit(receiver: address) -> uint256: @internal def _max_withdraw( owner: address, - max_loss: uint256 = 0, - strategies: DynArray[address, MAX_QUEUE] = [] + max_loss: uint256, + strategies: DynArray[address, MAX_QUEUE] ) -> uint256: """ @dev Returns the max amount of `asset` and `owner` can withdraw. @@ -1875,7 +1875,7 @@ def maxWithdraw( @external def maxRedeem( owner: address, - max_loss: uint256 = 0, + max_loss: uint256 = MAX_BPS, strategies: DynArray[address, MAX_QUEUE] = [] ) -> uint256: """ @@ -1886,7 +1886,11 @@ def maxRedeem( @param strategies Custom strategies queue if any. @return The maximum amount of shares that can be redeemed. """ - return self._convert_to_shares(self._max_withdraw(owner, max_loss, strategies), Rounding.ROUND_DOWN) + return min( + # Max witdraw is rounding so we check against the full balance. + self._convert_to_shares(self._max_withdraw(owner, max_loss, strategies), Rounding.ROUND_UP), + self.balance_of[owner] + ) @view @external diff --git a/tests/unit/vault/test_erc4626.py b/tests/unit/vault/test_erc4626.py index 5c4b57fb..7f7126e9 100644 --- a/tests/unit/vault/test_erc4626.py +++ b/tests/unit/vault/test_erc4626.py @@ -1,6 +1,6 @@ import ape import pytest -from utils.constants import ROLES +from utils.constants import ROLES, DAY def test_total_assets(asset, fish, fish_amount, create_vault, user_deposit): @@ -155,6 +155,104 @@ def test_max_withdraw__with_balance_less_than_or_equal_to_total_idle__returns_ba assert vault.maxWithdraw(fish.address) == assets +def test_max_withdraw__with_custom_params( + asset, + fish, + fish_amount, + gov, + create_vault, + create_strategy, + add_debt_to_strategy, + add_strategy_to_vault, + user_deposit, +): + vault = create_vault(asset) + shares = fish_amount + assets = shares + strategy = create_strategy(vault) + strategy_deposit = assets // 2 + total_idle = assets - strategy_deposit + + vault.set_role( + gov.address, + ROLES.ADD_STRATEGY_MANAGER | ROLES.DEBT_MANAGER | ROLES.MAX_DEBT_MANAGER, + sender=gov, + ) + user_deposit(fish, vault, asset, assets) + add_strategy_to_vault(gov, strategy, vault) + add_debt_to_strategy(gov, strategy, vault, strategy_deposit) + + assert vault.maxWithdraw(fish.address, 22, [strategy]) == assets + + +def test_max_withdraw__with_lossy_strategy( + asset, + fish, + fish_amount, + gov, + create_vault, + create_lossy_strategy, + add_debt_to_strategy, + add_strategy_to_vault, + user_deposit, +): + vault = create_vault(asset) + shares = fish_amount + assets = shares + strategy = create_lossy_strategy(vault) + strategy_deposit = assets // 2 + loss = strategy_deposit // 2 + total_idle = assets - strategy_deposit + + vault.set_role( + gov.address, + ROLES.ADD_STRATEGY_MANAGER | ROLES.DEBT_MANAGER | ROLES.MAX_DEBT_MANAGER, + sender=gov, + ) + user_deposit(fish, vault, asset, assets) + add_strategy_to_vault(gov, strategy, vault) + add_debt_to_strategy(gov, strategy, vault, strategy_deposit) + + strategy.setLoss(gov.address, loss, sender=gov) + + # Should not effect the default returned value + assert vault.maxWithdraw(fish.address, 10_000) == assets + + # Should return just idle if max_loss == 0. + assert vault.maxWithdraw(fish.address) == total_idle + + +def test_max_withdraw__with_locked_strategy( + asset, + fish, + fish_amount, + gov, + create_vault, + create_locked_strategy, + add_debt_to_strategy, + add_strategy_to_vault, + user_deposit, +): + vault = create_vault(asset) + shares = fish_amount + assets = shares + strategy = create_locked_strategy(vault) + strategy_deposit = assets // 2 + locked = strategy_deposit // 2 + total_idle = assets - strategy_deposit + + vault.set_role( + gov.address, + ROLES.ADD_STRATEGY_MANAGER | ROLES.DEBT_MANAGER | ROLES.MAX_DEBT_MANAGER, + sender=gov, + ) + user_deposit(fish, vault, asset, assets) + add_strategy_to_vault(gov, strategy, vault) + add_debt_to_strategy(gov, strategy, vault, strategy_deposit) + + strategy.setLockedFunds(locked, DAY, sender=gov) + + assert vault.maxWithdraw(fish.address) == assets - locked def test_preview_redeem(asset, fish, fish_amount, create_vault, user_deposit): vault = create_vault(asset) @@ -165,7 +263,6 @@ def test_preview_redeem(asset, fish, fish_amount, create_vault, user_deposit): assert vault.previewRedeem(shares) == assets - def test_max_redeem__with_balance_greater_than_total_idle__returns_balance( asset, fish, @@ -206,3 +303,103 @@ def test_max_redeem__with_balance_less_than_or_equal_to_total_idle__returns_bala user_deposit(fish, vault, asset, shares) assert vault.maxRedeem(fish.address) == assets + + +def test_max_redeem__with_custom_params( + asset, + fish, + fish_amount, + gov, + create_vault, + create_strategy, + add_debt_to_strategy, + add_strategy_to_vault, + user_deposit, +): + vault = create_vault(asset) + shares = fish_amount + assets = shares + strategy = create_strategy(vault) + strategy_deposit = assets // 2 + total_idle = assets - strategy_deposit + + vault.set_role( + gov.address, + ROLES.ADD_STRATEGY_MANAGER | ROLES.DEBT_MANAGER | ROLES.MAX_DEBT_MANAGER, + sender=gov, + ) + user_deposit(fish, vault, asset, assets) + add_strategy_to_vault(gov, strategy, vault) + add_debt_to_strategy(gov, strategy, vault, strategy_deposit) + + assert vault.maxRedeem(fish.address, 0, [strategy]) == assets + + +def test_max_redeem__with_lossy_strategy( + asset, + fish, + fish_amount, + gov, + create_vault, + create_lossy_strategy, + add_debt_to_strategy, + add_strategy_to_vault, + user_deposit, +): + vault = create_vault(asset) + shares = fish_amount + assets = shares + strategy = create_lossy_strategy(vault) + strategy_deposit = assets // 2 + loss = strategy_deposit // 2 + total_idle = assets - strategy_deposit + + vault.set_role( + gov.address, + ROLES.ADD_STRATEGY_MANAGER | ROLES.DEBT_MANAGER | ROLES.MAX_DEBT_MANAGER, + sender=gov, + ) + user_deposit(fish, vault, asset, assets) + add_strategy_to_vault(gov, strategy, vault) + add_debt_to_strategy(gov, strategy, vault, strategy_deposit) + + strategy.setLoss(gov.address, loss, sender=gov) + + # Should not effect the default returned value + assert vault.maxRedeem(fish.address) == assets + + # Should return just idle if max_loss == 0. + assert vault.maxRedeem(fish.address, 0) == total_idle + + +def test_max_redeem__with_locked_strategy( + asset, + fish, + fish_amount, + gov, + create_vault, + create_locked_strategy, + add_debt_to_strategy, + add_strategy_to_vault, + user_deposit, +): + vault = create_vault(asset) + shares = fish_amount + assets = shares + strategy = create_locked_strategy(vault) + strategy_deposit = assets // 2 + locked = strategy_deposit // 2 + total_idle = assets - strategy_deposit + + vault.set_role( + gov.address, + ROLES.ADD_STRATEGY_MANAGER | ROLES.DEBT_MANAGER | ROLES.MAX_DEBT_MANAGER, + sender=gov, + ) + user_deposit(fish, vault, asset, assets) + add_strategy_to_vault(gov, strategy, vault) + add_debt_to_strategy(gov, strategy, vault, strategy_deposit) + + strategy.setLockedFunds(locked, DAY, sender=gov) + + assert vault.maxRedeem(fish.address) == assets - locked From 3799b70c8af7109d520e1fe0af8406298bad2fe1 Mon Sep 17 00:00:00 2001 From: Schlagonia Date: Fri, 1 Sep 2023 09:54:58 -0600 Subject: [PATCH 3/4] fix: unrealised loss flow --- contracts/VaultV3.vy | 34 +++++++++++++++++++++++--------- tests/unit/vault/test_erc4626.py | 3 +++ 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/contracts/VaultV3.vy b/contracts/VaultV3.vy index b7e85863..8ef08669 100644 --- a/contracts/VaultV3.vy +++ b/contracts/VaultV3.vy @@ -557,11 +557,19 @@ def _max_withdraw( strategies: DynArray[address, MAX_QUEUE] ) -> uint256: """ - @dev Returns the max amount of `asset` and `owner` can withdraw. + @dev Returns the max amount of `asset` an `owner` can withdraw. This will do a full simulation of the withdraw in order to determine how much is currently liquid and if the `max_loss` would allow for the tx to not revert. + + This will track any expected loss to check if the tx will revert, but + not account for it in the amount returned since it is unrealised and + therefore will not be accounted for in the conversion rates. + + i.e. If we have 100 debt and 10 of unrealised loss, the max we can get + out is 90, but a user of the vault will need to call withdraw with 100 + in order to get the full 90 out. """ # Get the max amount for the owner if fully liquid. max_assets: uint256 = self._convert_to_assets(self.balance_of[owner], Rounding.ROUND_DOWN) @@ -570,7 +578,7 @@ def _max_withdraw( current_idle: uint256 = self.total_idle if max_assets > current_idle: # Amount left that we need. - needed: uint256 = max_assets - current_idle + needed: uint256 = unsafe_sub(max_assets, current_idle) # Track how much we can pull. have: uint256 = current_idle loss: uint256 = 0 @@ -585,22 +593,30 @@ def _max_withdraw( assert self.strategies[strategy].activation != 0, "inactive strategy" # Get the maximum amount the vault can withdraw from the strategy. - max_withdraw: uint256 = IStrategy(strategy).maxWithdraw(self) + max_withdraw: uint256 = min(needed, self.strategies[strategy].current_debt) + + # Get any unrealised loss for the strategy. + unrealised_loss: uint256 = self._assess_share_of_unrealised_losses(strategy, max_withdraw) + + # See if any limit is enforced by the strategy. + strategy_limit: uint256 = IStrategy(strategy).maxWithdraw(self) + + # Adjust accordingly if there is a max withdraw limit. + if strategy_limit < max_withdraw - unrealised_loss: + # lower unrealised loss to the proportion to the limit. + unrealised_loss = unrealised_loss * strategy_limit / max_withdraw + # Still count the unrealised loss as withdrawable. + max_withdraw = strategy_limit + unrealised_loss # If 0 move on to the next strategy. if max_withdraw == 0: continue - # If we more than what we need, adjust it down. - if max_withdraw > needed: - max_withdraw = needed - # Add to what we can pull. have += max_withdraw # Reduce how much we have left. needed -= max_withdraw - # Track how much loss would be passed on to the owner. - loss += self._assess_share_of_unrealised_losses(strategy, max_withdraw) + loss += unrealised_loss if needed == 0: break diff --git a/tests/unit/vault/test_erc4626.py b/tests/unit/vault/test_erc4626.py index 7f7126e9..37a05a5e 100644 --- a/tests/unit/vault/test_erc4626.py +++ b/tests/unit/vault/test_erc4626.py @@ -155,6 +155,7 @@ def test_max_withdraw__with_balance_less_than_or_equal_to_total_idle__returns_ba assert vault.maxWithdraw(fish.address) == assets + def test_max_withdraw__with_custom_params( asset, fish, @@ -254,6 +255,7 @@ def test_max_withdraw__with_locked_strategy( assert vault.maxWithdraw(fish.address) == assets - locked + def test_preview_redeem(asset, fish, fish_amount, create_vault, user_deposit): vault = create_vault(asset) shares = fish_amount @@ -263,6 +265,7 @@ def test_preview_redeem(asset, fish, fish_amount, create_vault, user_deposit): assert vault.previewRedeem(shares) == assets + def test_max_redeem__with_balance_greater_than_total_idle__returns_balance( asset, fish, From 16322281c0b6e5503792859d26f23fd3500d27f9 Mon Sep 17 00:00:00 2001 From: Schlagonia Date: Sat, 2 Sep 2023 11:51:47 -0600 Subject: [PATCH 4/4] fix: comments --- contracts/VaultV3.vy | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/VaultV3.vy b/contracts/VaultV3.vy index 8ef08669..e88b5c60 100644 --- a/contracts/VaultV3.vy +++ b/contracts/VaultV3.vy @@ -616,12 +616,13 @@ def _max_withdraw( have += max_withdraw # Reduce how much we have left. needed -= max_withdraw + # Add any unrealised loss to the total loss += unrealised_loss if needed == 0: break - # Update the max based after going through the queue. + # Update the max after going through the queue. max_assets = have # Check if there is a loss and a non-default value was set.