From 0d952b25615d6778caf4a5fbd21f100afe4ec6f7 Mon Sep 17 00:00:00 2001 From: Schlagonia Date: Mon, 28 Oct 2024 09:45:44 -0600 Subject: [PATCH 1/8] fix: self report refund override --- contracts/VaultV3.vy | 10 +- package.json | 2 +- tests/unit/vault/test_strategy_accounting.py | 115 +++++++++++++++++++ yarn.lock | 8 +- 4 files changed, 127 insertions(+), 8 deletions(-) diff --git a/contracts/VaultV3.vy b/contracts/VaultV3.vy index d45ae5b7..142a325c 100644 --- a/contracts/VaultV3.vy +++ b/contracts/VaultV3.vy @@ -1257,8 +1257,10 @@ def _process_report(strategy: address) -> (uint256, uint256): self.strategies[strategy].current_debt = current_debt self.total_debt += gain else: - self.total_idle = total_assets - + # Add in any refunds since it is idle. + current_debt = unsafe_add(current_debt, total_refunds) + self.total_idle = current_debt + # Or record any reported loss elif loss > 0: current_debt = unsafe_sub(current_debt, loss) @@ -1266,7 +1268,9 @@ def _process_report(strategy: address) -> (uint256, uint256): self.strategies[strategy].current_debt = current_debt self.total_debt -= loss else: - self.total_idle = total_assets + # Add in any refunds since it is idle. + current_debt = unsafe_add(current_debt, total_refunds) + self.total_idle = current_debt # Issue shares for fees that were calculated above if applicable. if total_fees_shares > 0: diff --git a/package.json b/package.json index 0445d4ba..57824298 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "yearn-vaults-v3", "devDependencies": { - "@openzeppelin/contracts": "^4.9.0", + "@openzeppelin/contracts": "^4.9.5", "hardhat": "^2.12.2", "prettier": "^2.6.0", "prettier-plugin-solidity": "^1.0.0-beta.19", diff --git a/tests/unit/vault/test_strategy_accounting.py b/tests/unit/vault/test_strategy_accounting.py index 0b067e78..ee090dcb 100644 --- a/tests/unit/vault/test_strategy_accounting.py +++ b/tests/unit/vault/test_strategy_accounting.py @@ -651,3 +651,118 @@ def test_set_accountant__with_accountant(gov, vault, deploy_accountant): assert event[0].accountant == accountant.address assert vault.accountant() == accountant.address + + +def test_process_report_on_self__gain_and_refunds( + chain, + gov, + asset, + vault, + set_fees_for_strategy, + airdrop_asset, + deploy_flexible_accountant, +): + vault_balance = asset.balanceOf(vault) + gain = vault_balance // 10 + loss = 0 + management_fee = 0 + performance_fee = 0 + refund_ratio = 5_000 + refund = gain * refund_ratio // MAX_BPS_ACCOUNTANT + + accountant = deploy_flexible_accountant(vault) + # set up accountant + asset.mint(accountant, gain, sender=gov) + + set_fees_for_strategy( + gov, vault, accountant, management_fee, performance_fee, refund_ratio + ) + + initial_idle = vault.totalIdle() + + airdrop_asset(gov, asset, vault, gain) + + # Not yet recorded + assert vault.totalIdle() == initial_idle + assert asset.balanceOf(vault) == initial_idle + gain + pps_before = vault.pricePerShare() + assets_before = vault.totalAssets() + supply_before = vault.totalSupply() + tx = vault.process_report(vault.address, sender=gov) + event = list(tx.decode_logs(vault.StrategyReported)) + + assert len(event) == 1 + assert event[0].strategy == vault.address + assert event[0].gain == gain + assert event[0].loss == 0 + assert event[0].current_debt == vault_balance + gain + refund + assert event[0].total_fees == 0 + assert event[0].total_refunds == refund + + # Due to refunds, pps should have increased + assert vault.pricePerShare() == pps_before + assert vault.totalAssets() == vault_balance + gain + refund + assert vault.totalSupply() > supply_before + assert vault.totalDebt() == 0 + assert vault.totalIdle() == vault_balance + gain + refund + assert asset.balanceOf(vault) == vault_balance + gain + refund + + chain.pending_timestamp += DAY + chain.mine(timestamp=chain.pending_timestamp) + + assert vault.pricePerShare() > pps_before + + +def test_process_report_on_self__loss_and_refunds( + chain, + gov, + asset, + vault, + set_fees_for_strategy, + airdrop_asset, + deploy_flexible_accountant, +): + vault_balance = asset.balanceOf(vault) + gain = 0 + loss = vault_balance // 10 + management_fee = 0 + performance_fee = 0 + refund_ratio = 5_000 + refund = loss * refund_ratio // MAX_BPS_ACCOUNTANT + + accountant = deploy_flexible_accountant(vault) + # set up accountant + asset.mint(accountant, loss, sender=gov) + + set_fees_for_strategy( + gov, vault, accountant, management_fee, performance_fee, refund_ratio + ) + + initial_idle = vault.totalIdle() + + asset.transfer(gov, loss, sender=vault.address) + + # Not yet recorded + assert vault.totalIdle() == initial_idle + assert asset.balanceOf(vault) == initial_idle - loss + pps_before = vault.pricePerShare() + assets_before = vault.totalAssets() + supply_before = vault.totalSupply() + tx = vault.process_report(vault.address, sender=gov) + event = list(tx.decode_logs(vault.StrategyReported)) + + assert len(event) == 1 + assert event[0].strategy == vault.address + assert event[0].gain == gain + assert event[0].loss == loss + assert event[0].current_debt == vault_balance + refund - loss + assert event[0].total_fees == 0 + assert event[0].total_refunds == refund + + # Due to refunds, pps should have increased + assert vault.pricePerShare() < pps_before + assert vault.totalAssets() == vault_balance + refund - loss + assert vault.totalSupply() == supply_before + assert vault.totalDebt() == 0 + assert vault.totalIdle() == vault_balance + refund - loss + assert asset.balanceOf(vault) == vault_balance + refund - loss diff --git a/yarn.lock b/yarn.lock index bd02e075..3a4c4a17 100644 --- a/yarn.lock +++ b/yarn.lock @@ -419,10 +419,10 @@ "@nomicfoundation/solidity-analyzer-win32-ia32-msvc" "0.1.0" "@nomicfoundation/solidity-analyzer-win32-x64-msvc" "0.1.0" -"@openzeppelin/contracts@^4.7.3": - version "4.8.2" - resolved "https://registry.yarnpkg.com/@openzeppelin/contracts/-/contracts-4.8.2.tgz#d815ade0027b50beb9bcca67143c6bcc3e3923d6" - integrity sha512-kEUOgPQszC0fSYWpbh2kT94ltOJwj1qfT2DWo+zVttmGmf97JZ99LspePNaeeaLhCImaHVeBbjaQFZQn7+Zc5g== +"@openzeppelin/contracts@^4.9.5": + version "4.9.6" + resolved "https://registry.yarnpkg.com/@openzeppelin/contracts/-/contracts-4.9.6.tgz#2a880a24eb19b4f8b25adc2a5095f2aa27f39677" + integrity sha512-xSmezSupL+y9VkHZJGDoCBpmnB2ogM13ccaYDWqJTfS3dbuHkgjuwDFUmaFauBCboQMGB/S5UqUl2y54X99BmA== "@scure/base@~1.1.0": version "1.1.1" From 03a750b3222d1a342348c90228bcac7234def559 Mon Sep 17 00:00:00 2001 From: Schlagonia Date: Mon, 28 Oct 2024 10:48:19 -0600 Subject: [PATCH 2/8] chore: api version pure --- contracts/VaultV3.vy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/VaultV3.vy b/contracts/VaultV3.vy index 142a325c..d3736046 100644 --- a/contracts/VaultV3.vy +++ b/contracts/VaultV3.vy @@ -2103,7 +2103,7 @@ def FACTORY() -> address: """ return self.factory -@view +@pure @external def apiVersion() -> String[28]: """ From 08003116a524e61152179faf095702f247b47d28 Mon Sep 17 00:00:00 2001 From: Schlagonia Date: Mon, 28 Oct 2024 11:15:19 -0600 Subject: [PATCH 3/8] chore: gas savings --- contracts/VaultV3.vy | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/contracts/VaultV3.vy b/contracts/VaultV3.vy index d3736046..75a06012 100644 --- a/contracts/VaultV3.vy +++ b/contracts/VaultV3.vy @@ -861,7 +861,7 @@ def _redeem( # NOTE: strategy's debt decreases by the full amount but the total idle increases # by the actual amount only (as the difference is considered lost). - current_total_idle += (assets_to_withdraw - loss) + current_total_idle += (unsafe_sub(assets_to_withdraw, loss)) requested_assets -= loss current_total_debt -= assets_to_withdraw @@ -928,14 +928,11 @@ def _add_strategy(new_strategy: address, add_to_queue: bool): @internal def _revoke_strategy(strategy: address, force: bool=False): assert self.strategies[strategy].activation != 0, "strategy not active" - - # If force revoking a strategy, it will cause a loss. - loss: uint256 = 0 if self.strategies[strategy].current_debt != 0: assert force, "strategy has debt" # Vault realizes the full loss of outstanding debt. - loss = self.strategies[strategy].current_debt + loss: uint256 = self.strategies[strategy].current_debt # Adjust total vault debt. self.total_debt -= loss @@ -1031,7 +1028,7 @@ def _update_debt(strategy: address, target_debt: uint256, max_loss: uint256) -> # If we didn't get the amount we asked for and there is a max loss. if withdrawn < assets_to_withdraw and max_loss < MAX_BPS: # Make sure the loss is within the allowed range. - assert assets_to_withdraw - withdrawn <= assets_to_withdraw * max_loss / MAX_BPS, "too much loss" + assert unsafe_sub(assets_to_withdraw, withdrawn) <= assets_to_withdraw * max_loss / MAX_BPS, "too much loss" # If we got too much make sure not to increase PPS. elif withdrawn > assets_to_withdraw: @@ -1548,9 +1545,10 @@ def add_role(account: address, role: Roles): @param role The new role to add to account. """ assert msg.sender == self.role_manager - self.roles[account] = self.roles[account] | role + new_roles: Roles = self.roles[account] | role + self.roles[account] = new_roles - log RoleSet(account, self.roles[account]) + log RoleSet(account, new_roles) @external def remove_role(account: address, role: Roles): @@ -1562,9 +1560,10 @@ def remove_role(account: address, role: Roles): @param role The Role to remove. """ assert msg.sender == self.role_manager - self.roles[account] = self.roles[account] & ~role + new_roles: Roles = self.roles[account] & ~role + self.roles[account] = new_roles - log RoleSet(account, self.roles[account]) + log RoleSet(account, new_roles) @external def transfer_role_manager(role_manager: address): @@ -1675,15 +1674,16 @@ def buy_debt(strategy: address, amount: uint256): self._erc20_safe_transfer_from(self.asset, msg.sender, self, _amount) - # Lower strategy debt - self.strategies[strategy].current_debt -= _amount + # Lower strategy debt + new_debt: uint256 = unsafe_sub(current_debt, _amount) + self.strategies[strategy].current_debt = new_debt # lower total debt self.total_debt -= _amount # Increase total idle self.total_idle += _amount # log debt change - log DebtUpdated(strategy, current_debt, current_debt - _amount) + log DebtUpdated(strategy, current_debt, new_debt) # Transfer the strategies shares out. self._erc20_safe_transfer(strategy, msg.sender, shares) From 1bccce1fdc35ba270a959c80429b04a4e069e525 Mon Sep 17 00:00:00 2001 From: Schlagonia Date: Mon, 28 Oct 2024 11:17:11 -0600 Subject: [PATCH 4/8] chore: add role event --- contracts/VaultV3.vy | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/contracts/VaultV3.vy b/contracts/VaultV3.vy index 75a06012..c2c7f102 100644 --- a/contracts/VaultV3.vy +++ b/contracts/VaultV3.vy @@ -1776,7 +1776,10 @@ def shutdown_vault(): self.deposit_limit = 0 log UpdateDepositLimit(0) - self.roles[msg.sender] = self.roles[msg.sender] | Roles.DEBT_MANAGER + new_roles: Roles = self.roles[msg.sender] | Roles.DEBT_MANAGER + self.roles[msg.sender] = new_roles + log RoleSet(msg.sender, new_roles) + log Shutdown() From 1ff539b675fc93986e1359c6676c60dfcef89cea Mon Sep 17 00:00:00 2001 From: Schlagonia Date: Mon, 28 Oct 2024 11:30:37 -0600 Subject: [PATCH 5/8] chore: add events --- contracts/VaultV3.vy | 7 ++++++- tests/unit/vault/test_emergency_shutdown.py | 6 +++++- tests/unit/vault/test_roles.py | 20 ++++++++++++++++---- 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/contracts/VaultV3.vy b/contracts/VaultV3.vy index c2c7f102..92ff5063 100644 --- a/contracts/VaultV3.vy +++ b/contracts/VaultV3.vy @@ -112,6 +112,9 @@ event RoleSet: role: indexed(Roles) # STORAGE MANAGEMENT EVENTS +event UpdateFutureRoleManager: + future_role_manager: indexed(address) + event UpdateRoleManager: role_manager: indexed(address) @@ -1577,6 +1580,8 @@ def transfer_role_manager(role_manager: address): assert msg.sender == self.role_manager self.future_role_manager = role_manager + log UpdateFutureRoleManager(role_manager) + @external def accept_role_manager(): """ @@ -1779,7 +1784,7 @@ def shutdown_vault(): new_roles: Roles = self.roles[msg.sender] | Roles.DEBT_MANAGER self.roles[msg.sender] = new_roles log RoleSet(msg.sender, new_roles) - + log Shutdown() diff --git a/tests/unit/vault/test_emergency_shutdown.py b/tests/unit/vault/test_emergency_shutdown.py index 593cfe58..35b5ca06 100644 --- a/tests/unit/vault/test_emergency_shutdown.py +++ b/tests/unit/vault/test_emergency_shutdown.py @@ -25,7 +25,11 @@ def test_shutdown(gov, panda, vault): def test_shutdown_gives_debt_manager_role(gov, panda, vault): vault.set_role(panda.address, ROLES.EMERGENCY_MANAGER, sender=gov) assert ROLES.DEBT_MANAGER not in ROLES(vault.roles(panda)) - vault.shutdown_vault(sender=panda) + tx = vault.shutdown_vault(sender=panda) + event = list(tx.decode_logs(vault.RoleSet)) + assert len(event) == 1 + assert event[0].account == panda.address + assert event[0].role == ROLES.DEBT_MANAGER | ROLES.EMERGENCY_MANAGER assert ROLES.DEBT_MANAGER | ROLES.EMERGENCY_MANAGER == vault.roles(panda) diff --git a/tests/unit/vault/test_roles.py b/tests/unit/vault/test_roles.py index f77ee04c..75bca2d4 100644 --- a/tests/unit/vault/test_roles.py +++ b/tests/unit/vault/test_roles.py @@ -18,7 +18,10 @@ def test_transfers_role_manager(vault, gov, strategist): assert vault.role_manager() == gov assert vault.future_role_manager() == ZERO_ADDRESS - vault.transfer_role_manager(strategist, sender=gov) + tx = vault.transfer_role_manager(strategist, sender=gov) + event = list(tx.decode_logs(vault.UpdateFutureRoleManager)) + assert len(event) == 1 + assert event[0].future_role_manager == strategist assert vault.role_manager() == gov assert vault.future_role_manager() == strategist @@ -36,7 +39,10 @@ def test_gov_transfers_role_manager__gov_cant_accept(vault, gov, strategist): assert vault.role_manager() == gov assert vault.future_role_manager() == ZERO_ADDRESS - vault.transfer_role_manager(strategist, sender=gov) + tx = vault.transfer_role_manager(strategist, sender=gov) + event = list(tx.decode_logs(vault.UpdateFutureRoleManager)) + assert len(event) == 1 + assert event[0].future_role_manager == strategist assert vault.role_manager() == gov assert vault.future_role_manager() == strategist @@ -65,12 +71,18 @@ def test_gov_transfers_role_manager__can_change_future_manager( assert vault.role_manager() == gov assert vault.future_role_manager() == ZERO_ADDRESS - vault.transfer_role_manager(strategist, sender=gov) + tx = vault.transfer_role_manager(strategist, sender=gov) + event = list(tx.decode_logs(vault.UpdateFutureRoleManager)) + assert len(event) == 1 + assert event[0].future_role_manager == strategist assert vault.role_manager() == gov assert vault.future_role_manager() == strategist - vault.transfer_role_manager(bunny, sender=gov) + tx = vault.transfer_role_manager(bunny, sender=gov) + event = list(tx.decode_logs(vault.UpdateFutureRoleManager)) + assert len(event) == 1 + assert event[0].future_role_manager == bunny assert vault.role_manager() == gov assert vault.future_role_manager() == bunny From 8c3fdf61a4cf8ea04c64816cbd73cc8deedf9db9 Mon Sep 17 00:00:00 2001 From: Schlagonia Date: Mon, 28 Oct 2024 11:35:11 -0600 Subject: [PATCH 6/8] chore: fix comments --- contracts/VaultV3.vy | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/contracts/VaultV3.vy b/contracts/VaultV3.vy index 92ff5063..2f13337b 100644 --- a/contracts/VaultV3.vy +++ b/contracts/VaultV3.vy @@ -1541,11 +1541,11 @@ def set_role(account: address, role: Roles): @external def add_role(account: address, role: Roles): """ - @notice Add a new role to an address. - @dev This will add a new role to the account + @notice Add a new role/s to an address. + @dev This will add a new role/s to the account without effecting any of the previously held roles. @param account The account to add a role to. - @param role The new role to add to account. + @param role The new role/s to add to account. """ assert msg.sender == self.role_manager new_roles: Roles = self.roles[account] | role @@ -1556,11 +1556,11 @@ def add_role(account: address, role: Roles): @external def remove_role(account: address, role: Roles): """ - @notice Remove a single role from an account. + @notice Remove a role/s from an account. @dev This will leave all other roles for the account unchanged. - @param account The account to remove a Role from. - @param role The Role to remove. + @param account The account to remove a Role/s from. + @param role The Role/s to remove. """ assert msg.sender == self.role_manager new_roles: Roles = self.roles[account] & ~role @@ -1755,7 +1755,7 @@ def update_debt( @param strategy The strategy to update the debt for. @param target_debt The target debt for the strategy. @param max_loss Optional to check realized losses on debt decreases. - @return The amount of debt added or removed. + @return The new current debt of the strategy. """ self._enforce_role(msg.sender, Roles.DEBT_MANAGER) return self._update_debt(strategy, target_debt, max_loss) From ec1b8663b97df4de02012dd85ffa9922ab626d2b Mon Sep 17 00:00:00 2001 From: Schlagonia Date: Mon, 28 Oct 2024 13:23:47 -0600 Subject: [PATCH 7/8] test: fix invariant --- contracts/VaultV3.vy | 4 ++-- foundry_tests/handlers/VaultHandler.sol | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/contracts/VaultV3.vy b/contracts/VaultV3.vy index 2f13337b..239dbacd 100644 --- a/contracts/VaultV3.vy +++ b/contracts/VaultV3.vy @@ -1257,7 +1257,7 @@ def _process_report(strategy: address) -> (uint256, uint256): self.strategies[strategy].current_debt = current_debt self.total_debt += gain else: - # Add in any refunds since it is idle. + # Add in any refunds since it is now idle. current_debt = unsafe_add(current_debt, total_refunds) self.total_idle = current_debt @@ -1268,7 +1268,7 @@ def _process_report(strategy: address) -> (uint256, uint256): self.strategies[strategy].current_debt = current_debt self.total_debt -= loss else: - # Add in any refunds since it is idle. + # Add in any refunds since it is now idle. current_debt = unsafe_add(current_debt, total_refunds) self.total_idle = current_debt diff --git a/foundry_tests/handlers/VaultHandler.sol b/foundry_tests/handlers/VaultHandler.sol index 19e67513..f80991dc 100644 --- a/foundry_tests/handlers/VaultHandler.sol +++ b/foundry_tests/handlers/VaultHandler.sol @@ -242,7 +242,9 @@ contract VaultHandler is ExtendedTest { uint256 _amount ) public countCall("unreportedLoss") { _amount = bound(_amount, 0, strategy.totalAssets() / 10); - + + if(_amount == 0) return; + // Simulate losing money vm.prank(address(strategy)); asset.transfer(address(69), _amount); From 428895242706378fce878aea6733e824e266e2ac Mon Sep 17 00:00:00 2001 From: Schlagonia Date: Mon, 28 Oct 2024 16:01:30 -0600 Subject: [PATCH 8/8] test: fix invariants --- foundry_tests/handlers/VaultHandler.sol | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/foundry_tests/handlers/VaultHandler.sol b/foundry_tests/handlers/VaultHandler.sol index f80991dc..523fdebf 100644 --- a/foundry_tests/handlers/VaultHandler.sol +++ b/foundry_tests/handlers/VaultHandler.sol @@ -101,7 +101,7 @@ contract VaultHandler is ExtendedTest { deposit(_amount * 2); } } - _amount = bound(_amount, 0, vault.maxWithdraw(address(actor))); + _amount = bound(_amount, 0, vault.maxWithdraw(address(actor)) + 1); if (_amount == 0) ghost_zeroWithdrawals++; uint256 idle = vault.totalIdle(); @@ -122,7 +122,7 @@ contract VaultHandler is ExtendedTest { mint(_amount * 2); } } - _amount = bound(_amount, 0, vault.balanceOf(address(actor))); + _amount = bound(_amount, 0, vault.balanceOf(address(actor)) + 1); if (_amount == 0) ghost_zeroWithdrawals++; uint256 idle = vault.totalIdle(); @@ -242,9 +242,9 @@ contract VaultHandler is ExtendedTest { uint256 _amount ) public countCall("unreportedLoss") { _amount = bound(_amount, 0, strategy.totalAssets() / 10); - - if(_amount == 0) return; - + + if (_amount == 0) return; + // Simulate losing money vm.prank(address(strategy)); asset.transfer(address(69), _amount);