fix: chainsec audit fixes (#215)

* fix: self report refund override

* chore: api version pure

* chore: gas savings

* chore: add role event

* chore: add events

* chore: fix comments

* test: fix invariant

* test: fix invariants

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)
 
@@ -861,7 +864,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 +931,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 +1031,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:
@@ -1257,16 +1257,20 @@ 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 now 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)
         if strategy != self:
             self.strategies[strategy].current_debt = current_debt
             self.total_debt -= loss
         else:
-            self.total_idle = total_assets
+            # Add in any refunds since it is now 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:
@@ -1537,30 +1541,32 @@ 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
-    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):
     """
-    @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
-    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):
@@ -1574,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():
     """
@@ -1671,15 +1679,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)
@@ -1746,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)
@@ -1772,7 +1781,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()
 
 
@@ -2099,7 +2111,7 @@ def FACTORY() -> address:
     """
     return self.factory
 
-@view
+@pure
 @external
 def apiVersion() -> String[28]:
     """

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();
@@ -243,6 +243,8 @@ contract VaultHandler is ExtendedTest {
     ) 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);

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",

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)
 
 

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

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

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"