From 2d5b43d5dde01c3511c3e985d3477a4c16436284 Mon Sep 17 00:00:00 2001
From: Schlagonia <schlagonia@gmail.com>
Date: Mon, 5 Feb 2024 09:10:51 -0700
Subject: [PATCH 01/17] fix: use safe erc

---
 contracts/accountants/HealthCheckAccountant.sol | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contracts/accountants/HealthCheckAccountant.sol b/contracts/accountants/HealthCheckAccountant.sol
index 4ce37f9..f55b476 100644
--- a/contracts/accountants/HealthCheckAccountant.sol
+++ b/contracts/accountants/HealthCheckAccountant.sol
@@ -583,8 +583,8 @@ contract HealthCheckAccountant {
         uint256 _amount
     ) internal {
         if (ERC20(_token).allowance(address(this), _contract) < _amount) {
-            ERC20(_token).approve(_contract, 0);
-            ERC20(_token).approve(_contract, _amount);
+            ERC20(_token).safeApprove(_contract, 0);
+            ERC20(_token).safeApprove(_contract, _amount);
         }
     }
 

From 1013172538eb85d2cc0c823b7b7c2361f0cbac0e Mon Sep 17 00:00:00 2001
From: Schlagonia <schlagonia@gmail.com>
Date: Mon, 5 Feb 2024 09:30:00 -0700
Subject: [PATCH 02/17] fix: combine accountant functions

---
 .../accountants/HealthCheckAccountant.sol     | 60 +++++++++++--------
 1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/contracts/accountants/HealthCheckAccountant.sol b/contracts/accountants/HealthCheckAccountant.sol
index f55b476..54f221c 100644
--- a/contracts/accountants/HealthCheckAccountant.sol
+++ b/contracts/accountants/HealthCheckAccountant.sol
@@ -151,29 +151,15 @@ contract HealthCheckAccountant {
     ) {
         require(_feeManager != address(0), "ZERO ADDRESS");
         require(_feeRecipient != address(0), "ZERO ADDRESS");
-        require(
-            defaultManagement <= MANAGEMENT_FEE_THRESHOLD,
-            "exceeds management fee threshold"
-        );
-        require(
-            defaultPerformance <= PERFORMANCE_FEE_THRESHOLD,
-            "exceeds performance fee threshold"
-        );
-        require(defaultMaxLoss <= MAX_BPS, "too high");
-
-        feeManager = _feeManager;
-        feeRecipient = _feeRecipient;
-
-        defaultConfig = Fee({
-            managementFee: defaultManagement,
-            performanceFee: defaultPerformance,
-            refundRatio: defaultRefund,
-            maxFee: defaultMaxFee,
-            maxGain: defaultMaxGain,
-            maxLoss: defaultMaxLoss
-        });
 
-        emit UpdateDefaultFeeConfig(defaultConfig);
+        _updateDefaultConfig(
+            defaultManagement,
+            defaultPerformance,
+            defaultRefund,
+            defaultMaxFee,
+            defaultMaxGain,
+            defaultMaxLoss
+        );
     }
 
     /**
@@ -323,14 +309,36 @@ contract HealthCheckAccountant {
         uint16 defaultMaxGain,
         uint16 defaultMaxLoss
     ) external virtual onlyFeeManager {
+        _updateDefaultConfig(
+            defaultManagement,
+            defaultPerformance,
+            defaultRefund,
+            defaultMaxFee,
+            defaultMaxGain,
+            defaultMaxLoss
+        );
+    }
+
+    /**
+     * @dev Updates the Accountant's default fee config.
+     *   Is used during deployment and during any future updates.
+     */
+    function _updateDefaultConfig(
+        uint16 defaultManagement,
+        uint16 defaultPerformance,
+        uint16 defaultRefund,
+        uint16 defaultMaxFee,
+        uint16 defaultMaxGain,
+        uint16 defaultMaxLoss
+    ) internal virtual {
         // Check for threshold and limit conditions.
         require(
             defaultManagement <= MANAGEMENT_FEE_THRESHOLD,
-            "exceeds management fee threshold"
+            "management fee threshold"
         );
         require(
             defaultPerformance <= PERFORMANCE_FEE_THRESHOLD,
-            "exceeds performance fee threshold"
+            "performance fee threshold"
         );
         require(defaultMaxLoss <= MAX_BPS, "too high");
 
@@ -373,11 +381,11 @@ contract HealthCheckAccountant {
         // Check for threshold and limit conditions.
         require(
             customManagement <= MANAGEMENT_FEE_THRESHOLD,
-            "exceeds management fee threshold"
+            "management fee threshold"
         );
         require(
             customPerformance <= PERFORMANCE_FEE_THRESHOLD,
-            "exceeds performance fee threshold"
+            "performance fee threshold"
         );
         require(customMaxLoss <= MAX_BPS, "too high");
 

From 773f63edcfe04b710d199a572341a7c8c048a1b2 Mon Sep 17 00:00:00 2001
From: Schlagonia <schlagonia@gmail.com>
Date: Mon, 5 Feb 2024 09:35:58 -0700
Subject: [PATCH 03/17] fix: typos

---
 contracts/Managers/RoleManager.sol                     | 2 +-
 contracts/accountants/HealthCheckAccountant.sol        | 8 +++-----
 contracts/accountants/RefundAccountant.sol             | 2 +-
 contracts/debtAllocators/DebtAllocator.sol             | 4 ++--
 contracts/debtAllocators/YieldManager/YieldManager.sol | 4 ++--
 5 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/contracts/Managers/RoleManager.sol b/contracts/Managers/RoleManager.sol
index 4ec4529..a2fd67e 100644
--- a/contracts/Managers/RoleManager.sol
+++ b/contracts/Managers/RoleManager.sol
@@ -153,7 +153,7 @@ contract RoleManager is Governance2Step {
             )
         });
 
-        // Security cna set the max debt for strategies to have.
+        // Security can set the max debt for strategies to have.
         _positions[SECURITY] = Position({
             holder: _security,
             roles: uint96(Roles.MAX_DEBT_MANAGER)
diff --git a/contracts/accountants/HealthCheckAccountant.sol b/contracts/accountants/HealthCheckAccountant.sol
index 54f221c..78952c3 100644
--- a/contracts/accountants/HealthCheckAccountant.sol
+++ b/contracts/accountants/HealthCheckAccountant.sol
@@ -75,7 +75,7 @@ contract HealthCheckAccountant {
         _;
     }
 
-    modifier onlyVaultOrFeeManagers() {
+    modifier onlyVaultOrFeeManager() {
         _checkVaultOrFeeManager();
         _;
     }
@@ -261,7 +261,7 @@ contract HealthCheckAccountant {
      * @dev This is not used to set any of the fees for the specific vault or strategy. Each fee will be set separately.
      * @param vault The address of a vault to allow to use this accountant.
      */
-    function addVault(address vault) external virtual onlyVaultOrFeeManagers {
+    function addVault(address vault) external virtual onlyVaultOrFeeManager {
         // Ensure the vault has not already been added.
         require(!vaults[vault], "already added");
 
@@ -274,9 +274,7 @@ contract HealthCheckAccountant {
      * @notice Function to remove a vault from this accountant's fee charging list.
      * @param vault The address of the vault to be removed from this accountant.
      */
-    function removeVault(
-        address vault
-    ) external virtual onlyVaultOrFeeManagers {
+    function removeVault(address vault) external virtual onlyVaultOrFeeManager {
         // Ensure the vault has been previously added.
         require(vaults[vault], "not added");
 
diff --git a/contracts/accountants/RefundAccountant.sol b/contracts/accountants/RefundAccountant.sol
index 8eb5738..a3faff5 100644
--- a/contracts/accountants/RefundAccountant.sol
+++ b/contracts/accountants/RefundAccountant.sol
@@ -92,7 +92,7 @@ contract RefundAccountant is HealthCheckAccountant {
 
     /**
      * @notice Set a strategy to use to refund a reward amount for
-     * aut compounding reward tokens.
+     * auto compounding reward tokens.
      *
      * @param _vault Address of the vault to refund.
      * @param _strategy Address of the strategy to refund during the report.
diff --git a/contracts/debtAllocators/DebtAllocator.sol b/contracts/debtAllocators/DebtAllocator.sol
index aad03d5..e0b41ae 100644
--- a/contracts/debtAllocators/DebtAllocator.sol
+++ b/contracts/debtAllocators/DebtAllocator.sol
@@ -293,7 +293,7 @@ contract DebtAllocator {
 
             // Check if it's over the threshold.
             if (toPull > minimumChange) {
-                // Can't lower debt if there is unrealised losses.
+                // Can't lower debt if there are unrealised losses.
                 if (
                     _vault.assess_share_of_unrealised_losses(
                         _strategy,
@@ -410,7 +410,7 @@ contract DebtAllocator {
 
     /**
      * @notice Set the minimum change variable for a strategy.
-     * @dev This is the amount of debt that will needed to be
+     * @dev This is the minimum amount of debt that will have to be
      * added or pulled for it to trigger an update.
      *
      * @param _minimumChange The new minimum to set for the strategy.
diff --git a/contracts/debtAllocators/YieldManager/YieldManager.sol b/contracts/debtAllocators/YieldManager/YieldManager.sol
index 954e9fb..332fdc7 100644
--- a/contracts/debtAllocators/YieldManager/YieldManager.sol
+++ b/contracts/debtAllocators/YieldManager/YieldManager.sol
@@ -152,7 +152,7 @@ contract YieldManager is Governance {
                     _accountedFor -= _loss;
                 }
 
-                // Make sure we the vault can withdraw that amount.
+                // Make sure the vault can withdraw that amount.
                 require(
                     _maxWithdraw(_vault, _strategy) >= _currentDebt - _newDebt,
                     "max withdraw"
@@ -358,7 +358,7 @@ contract YieldManager is Governance {
                     _totalAssets -= _loss;
                 }
 
-                // Make sure we the vault can withdraw that amount.
+                // Make sure the vault can withdraw that amount.
                 require(
                     _maxWithdraw(_vault, _strategy) >= _currentDebt - _newDebt,
                     "max withdraw"

From 8b19296363f3124e54d69b09a3d5430469d07082 Mon Sep 17 00:00:00 2001
From: Schlagonia <schlagonia@gmail.com>
Date: Mon, 5 Feb 2024 13:24:18 -0700
Subject: [PATCH 04/17] fix: check min idle

---
 contracts/accountants/HealthCheckAccountant.sol |  3 +++
 contracts/debtAllocators/DebtAllocator.sol      | 13 +++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/contracts/accountants/HealthCheckAccountant.sol b/contracts/accountants/HealthCheckAccountant.sol
index 78952c3..b6afab9 100644
--- a/contracts/accountants/HealthCheckAccountant.sol
+++ b/contracts/accountants/HealthCheckAccountant.sol
@@ -152,6 +152,9 @@ contract HealthCheckAccountant {
         require(_feeManager != address(0), "ZERO ADDRESS");
         require(_feeRecipient != address(0), "ZERO ADDRESS");
 
+        feeManager = _feeManager;
+        feeRecipient = _feeRecipient;
+
         _updateDefaultConfig(
             defaultManagement,
             defaultPerformance,
diff --git a/contracts/debtAllocators/DebtAllocator.sol b/contracts/debtAllocators/DebtAllocator.sol
index e0b41ae..7161e85 100644
--- a/contracts/debtAllocators/DebtAllocator.sol
+++ b/contracts/debtAllocators/DebtAllocator.sol
@@ -281,9 +281,18 @@ contract DebtAllocator {
             }
             // If current debt is greater than our max.
         } else if (maxDebt < params.current_debt) {
+            uint256 toPull = params.current_debt - targetDebt;
+
+            uint256 currentIdle = _vault.totalIdle();
+            uint256 minIdle = _vault.minimum_total_idle();
+            if (minIdle > currentIdle) {
+                // Pull at least the amount needed for minIdle.
+                toPull = Math.max(toPull, minIdle - currentIdle);
+            }
+
             // Find out by how much. Aim for the target.
-            uint256 toPull = Math.min(
-                params.current_debt - targetDebt,
+            toPull = Math.min(
+                toPull,
                 // Account for the current liquidity constraints.
                 // Use max redeem to match vault logic.
                 IVault(_strategy).convertToAssets(

From 720b4c77f34aa720f46785bd3e8d1734c280458b Mon Sep 17 00:00:00 2001
From: Schlagonia <schlagonia@gmail.com>
Date: Mon, 5 Feb 2024 13:48:03 -0700
Subject: [PATCH 05/17] fix: remove refund bool

---
 contracts/accountants/RefundAccountant.sol  | 31 ++++---------
 tests/accountants/test_refund_accountant.py | 48 +++++++++------------
 2 files changed, 28 insertions(+), 51 deletions(-)

diff --git a/contracts/accountants/RefundAccountant.sol b/contracts/accountants/RefundAccountant.sol
index a3faff5..399241e 100644
--- a/contracts/accountants/RefundAccountant.sol
+++ b/contracts/accountants/RefundAccountant.sol
@@ -14,20 +14,11 @@ contract RefundAccountant is HealthCheckAccountant {
     event UpdateRefund(
         address indexed vault,
         address indexed strategy,
-        bool refund,
         uint256 amount
     );
 
-    /// @notice Struct to hold refund info for a strategy.
-    struct Refund {
-        // If the accountant should refund on the report.
-        bool refund;
-        // The amount if any to refund.
-        uint248 amount;
-    }
-
-    /// @notice Mapping of vault => strategy => struct if there is a reward refund to give.
-    mapping(address => mapping(address => Refund)) public refund;
+    /// @notice Mapping of vault => strategy => amount if there is a reward refund to give.
+    mapping(address => mapping(address => uint256)) public refund;
 
     constructor(
         address _feeManager,
@@ -72,11 +63,11 @@ contract RefundAccountant is HealthCheckAccountant {
     {
         (totalFees, totalRefunds) = super.report(strategy, gain, loss);
 
-        Refund memory refundConfig = refund[msg.sender][strategy];
+        uint256 _refund = refund[msg.sender][strategy];
         // Check if the strategy is being given a refund.
-        if (refundConfig.refund) {
+        if (_refund != 0) {
             // Add it to the existing refunds.
-            totalRefunds += uint256(refundConfig.amount);
+            totalRefunds += _refund;
 
             // Make sure the vault is approved correctly.
             _checkAllowance(
@@ -96,13 +87,11 @@ contract RefundAccountant is HealthCheckAccountant {
      *
      * @param _vault Address of the vault to refund.
      * @param _strategy Address of the strategy to refund during the report.
-     * @param _refund Bool to turn it on or off.
-     * @param _amount Amount to refund per report.
+     * @param _amount Amount to refund on the next report.
      */
     function setRefund(
         address _vault,
         address _strategy,
-        bool _refund,
         uint256 _amount
     ) external virtual onlyFeeManager {
         require(vaults[_vault], "not added");
@@ -110,13 +99,9 @@ contract RefundAccountant is HealthCheckAccountant {
             IVault(_vault).strategies(_strategy).activation != 0,
             "!active"
         );
-        require(_refund || _amount == 0, "no refund and non zero amount");
 
-        refund[_vault][_strategy] = Refund({
-            refund: _refund,
-            amount: uint248(_amount)
-        });
+        refund[_vault][_strategy] = _amount;
 
-        emit UpdateRefund(_vault, _strategy, _refund, uint256(_amount));
+        emit UpdateRefund(_vault, _strategy, _amount);
     }
 }
diff --git a/tests/accountants/test_refund_accountant.py b/tests/accountants/test_refund_accountant.py
index 31571dd..49fbf17 100644
--- a/tests/accountants/test_refund_accountant.py
+++ b/tests/accountants/test_refund_accountant.py
@@ -22,70 +22,64 @@ def test_setup(daddy, vault, strategy, refund_accountant, fee_recipient):
     assert accountant.customConfig(vault.address, strategy.address).maxFee == 0
     assert accountant.customConfig(vault.address, strategy.address).maxGain == 0
     assert accountant.customConfig(vault.address, strategy.address).maxLoss == 0
-    assert accountant.refund(vault.address, strategy.address) == (False, 0)
+    assert accountant.refund(vault.address, strategy.address) == 0
 
 
 def test_add_reward_refund(daddy, vault, strategy, refund_accountant):
     accountant = refund_accountant
-    assert accountant.refund(vault.address, strategy.address) == (False, 0)
+    assert accountant.refund(vault.address, strategy.address) == 0
 
     amount = int(100)
 
     with ape.reverts("not added"):
-        accountant.setRefund(vault, strategy, True, amount, sender=daddy)
+        accountant.setRefund(vault, strategy, amount, sender=daddy)
 
     accountant.addVault(vault, sender=daddy)
 
     with ape.reverts("!active"):
-        accountant.setRefund(vault, strategy, True, amount, sender=daddy)
+        accountant.setRefund(vault, strategy, amount, sender=daddy)
 
     vault.add_strategy(strategy, sender=daddy)
 
-    tx = accountant.setRefund(vault, strategy, True, amount, sender=daddy)
+    tx = accountant.setRefund(vault, strategy, amount, sender=daddy)
 
     event = list(tx.decode_logs(accountant.UpdateRefund))
 
     assert len(event) == 1
     assert event[0].vault == vault.address
     assert event[0].strategy == strategy.address
-    assert event[0].refund == True
     assert event[0].amount == amount
-    assert accountant.refund(vault.address, strategy.address) == (True, 100)
+    assert accountant.refund(vault.address, strategy.address) == 100
 
-    with ape.reverts("no refund and non zero amount"):
-        accountant.setRefund(vault, strategy, False, amount, sender=daddy)
-
-    tx = accountant.setRefund(vault, strategy, False, 0, sender=daddy)
+    tx = accountant.setRefund(vault, strategy, 0, sender=daddy)
 
     event = list(tx.decode_logs(accountant.UpdateRefund))
 
     assert len(event) == 1
     assert event[0].vault == vault.address
     assert event[0].strategy == strategy.address
-    assert event[0].refund == False
     assert event[0].amount == 0
-    assert accountant.refund(vault.address, strategy.address) == (False, 0)
+    assert accountant.refund(vault.address, strategy.address) == 0
 
 
 def test_reward_refund(
     daddy, vault, strategy, refund_accountant, user, asset, deposit_into_vault
 ):
     accountant = refund_accountant
-    assert accountant.refund(vault.address, strategy.address) == (False, 0)
+    assert accountant.refund(vault.address, strategy.address) == 0
 
     amount = int(100)
     accountant.addVault(vault, sender=daddy)
     vault.add_strategy(strategy, sender=daddy)
 
-    tx = accountant.setRefund(vault, strategy, True, amount, sender=daddy)
+    tx = accountant.setRefund(vault, strategy, amount, sender=daddy)
 
     event = list(tx.decode_logs(accountant.UpdateRefund))
     assert len(event) == 1
     assert event[0].vault == vault.address
     assert event[0].strategy == strategy.address
-    assert event[0].refund == True
     assert event[0].amount == amount
-    assert accountant.refund(vault.address, strategy.address) == (True, 100)
+    assert accountant.refund(vault.address, strategy.address) == 100
 
     vault.set_accountant(accountant, sender=daddy)
 
@@ -120,7 +114,7 @@ def test_reward_refund(
     assert vault.fullProfitUnlockDate() > 0
 
     # Make sure the amounts got reset.
-    assert accountant.refund(vault.address, strategy.address) == (False, 0)
+    assert accountant.refund(vault.address, strategy.address) == 0
     tx = accountant.report(strategy, 0, 0, sender=vault)
     assert tx.return_value == (0, 0)
 
@@ -138,7 +132,7 @@ def test_reward_refund__with_gain(
     accountant = refund_accountant
     # Set performance fee to 10% and 0 management fee
     accountant.updateDefaultConfig(0, 1_000, 0, 10_000, 10_000, 0, sender=daddy)
-    assert accountant.refund(vault.address, strategy.address) == (False, 0)
+    assert accountant.refund(vault.address, strategy.address) == 0
 
     accountant.addVault(vault, sender=daddy)
     vault.add_strategy(strategy, sender=daddy)
@@ -150,15 +144,14 @@ def test_reward_refund__with_gain(
     gain = to_deposit // 10
     loss = 0
 
-    tx = accountant.setRefund(vault, strategy, True, refund, sender=daddy)
+    tx = accountant.setRefund(vault, strategy, refund, sender=daddy)
 
     event = list(tx.decode_logs(accountant.UpdateRefund))
     assert len(event) == 1
     assert event[0].vault == vault.address
     assert event[0].strategy == strategy.address
-    assert event[0].refund == True
     assert event[0].amount == refund
-    assert accountant.refund(vault.address, strategy.address) == (True, refund)
+    assert accountant.refund(vault.address, strategy.address) == refund
 
     vault.set_accountant(accountant, sender=daddy)
 
@@ -195,7 +188,7 @@ def test_reward_refund__with_gain(
     assert vault.fullProfitUnlockDate() > 0
 
     # Make sure the amounts got reset.
-    assert accountant.refund(vault.address, strategy.address) == (False, 0)
+    assert accountant.refund(vault.address, strategy.address) == 0
     tx = accountant.report(strategy, 0, 0, sender=vault)
     assert tx.return_value == (0, 0)
 
@@ -215,7 +208,7 @@ def test_reward_refund__with_loss__and_refund(
     accountant.updateDefaultConfig(
         0, 1_000, 10_000, 10_000, 10_000, 10_000, sender=daddy
     )
-    assert accountant.refund(vault.address, strategy.address) == (False, 0)
+    assert accountant.refund(vault.address, strategy.address) == 0
 
     accountant.addVault(vault, sender=daddy)
     vault.add_strategy(strategy, sender=daddy)
@@ -227,15 +220,14 @@ def test_reward_refund__with_loss__and_refund(
     gain = 0
     loss = to_deposit // 10
 
-    tx = accountant.setRefund(vault, strategy, True, refund, sender=daddy)
+    tx = accountant.setRefund(vault, strategy, refund, sender=daddy)
 
     event = list(tx.decode_logs(accountant.UpdateRefund))
     assert len(event) == 1
     assert event[0].vault == vault.address
     assert event[0].strategy == strategy.address
-    assert event[0].refund == True
     assert event[0].amount == refund
-    assert accountant.refund(vault.address, strategy.address) == (True, refund)
+    assert accountant.refund(vault.address, strategy.address) == refund
 
     vault.set_accountant(accountant, sender=daddy)
 
@@ -272,7 +264,7 @@ def test_reward_refund__with_loss__and_refund(
     assert vault.fullProfitUnlockDate() > 0
 
     # Make sure the amounts got reset.
-    assert accountant.refund(vault.address, strategy.address) == (False, 0)
+    assert accountant.refund(vault.address, strategy.address) == 0
     tx = accountant.report(strategy, 0, 0, sender=vault)
     assert tx.return_value == (0, 0)
 

From d8b01f8a85c745697cab114942fb8de08212aa55 Mon Sep 17 00:00:00 2001
From: Schlagonia <schlagonia@gmail.com>
Date: Mon, 5 Feb 2024 14:21:12 -0700
Subject: [PATCH 06/17] fix: debt allocator comments

---
 contracts/debtAllocators/DebtAllocator.sol | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/contracts/debtAllocators/DebtAllocator.sol b/contracts/debtAllocators/DebtAllocator.sol
index 7161e85..b3d633c 100644
--- a/contracts/debtAllocators/DebtAllocator.sol
+++ b/contracts/debtAllocators/DebtAllocator.sol
@@ -127,7 +127,7 @@ contract DebtAllocator {
     /// @notice Max loss to accept on debt updates in basis points.
     uint256 public maxDebtUpdateLoss;
 
-    /// @notice Mapping of addresses that are allowed to update debt.
+    /// @notice Mapping of addresses that are allowed to update debt ratios.
     mapping(address => bool) public managers;
 
     /// @notice Mapping of strategy => its config.
@@ -158,10 +158,9 @@ contract DebtAllocator {
     /**
      * @notice Debt update wrapper for the vault.
      * @dev This can be used if a minimum time between debt updates
-     *   is desired to be enforced and to enforce a max loss.
+     *   is desired to be used for the trigger and to enforce a max loss.
      *
-     *   This contract and the msg.sender must have the DEBT_MANAGER
-     *   role assigned to them.
+     *   This contract must have the DEBT_MANAGER role assigned to them.
      *
      *   The function signature matches the vault so no update to the
      *   call data is required.

From 68a7d149902ec88eafff21408da5196cb6550180 Mon Sep 17 00:00:00 2001
From: Schlagonia <schlagonia@gmail.com>
Date: Mon, 5 Feb 2024 14:44:23 -0700
Subject: [PATCH 07/17] feat: check if accountant set

---
 contracts/Managers/RoleManager.sol | 31 ++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/contracts/Managers/RoleManager.sol b/contracts/Managers/RoleManager.sol
index a2fd67e..2cfda95 100644
--- a/contracts/Managers/RoleManager.sol
+++ b/contracts/Managers/RoleManager.sol
@@ -393,20 +393,23 @@ contract RoleManager is Governance2Step {
      * @param _vault Address of the vault to set up the accountant for.
      */
     function _setAccountant(address _vault) internal virtual {
-        // Temporarily give this contract the ability to set the accountant.
-        IVault(_vault).add_role(address(this), Roles.ACCOUNTANT_MANAGER);
-
         // Get the current accountant.
         address accountant = getPositionHolder(ACCOUNTANT);
 
-        // Set the account on the vault.
-        IVault(_vault).set_accountant(accountant);
+        // If there is an accountant set.
+        if (accountant != address(0)) {
+            // Temporarily give this contract the ability to set the accountant.
+            IVault(_vault).add_role(address(this), Roles.ACCOUNTANT_MANAGER);
 
-        // Take away the role.
-        IVault(_vault).remove_role(address(this), Roles.ACCOUNTANT_MANAGER);
+            // Set the account on the vault.
+            IVault(_vault).set_accountant(accountant);
 
-        // Whitelist the vault in the accountant.
-        HealthCheckAccountant(accountant).addVault(_vault);
+            // Take away the role.
+            IVault(_vault).remove_role(address(this), Roles.ACCOUNTANT_MANAGER);
+
+            // Whitelist the vault in the accountant.
+            HealthCheckAccountant(accountant).addVault(_vault);
+        }
     }
 
     /**
@@ -799,7 +802,15 @@ contract RoleManager is Governance2Step {
     }
 
     /**
-     * @notice Get the address assigned to the allocator.
+     * @notice Get the address assigned to be the debt allocator if any.
+     * @return The address assigned to be the debt allocator if any.
+     */
+    function getDebtAllocator() external view virtual returns (address) {
+        return getPositionHolder(DEBT_ALLOCATOR);
+    }
+
+    /**
+     * @notice Get the address assigned to the allocator factory.
      * @return The address assigned to the allocator factory.
      */
     function getAllocatorFactory() external view virtual returns (address) {

From c3798af615d104432ccd3b0e4806cb6739ff3e8b Mon Sep 17 00:00:00 2001
From: Schlagonia <schlagonia@gmail.com>
Date: Mon, 5 Feb 2024 15:48:40 -0700
Subject: [PATCH 08/17] feat: remove role

---
 contracts/Managers/RoleManager.sol |  23 ++++++
 tests/manager/test_role_manager.py | 108 +++++++++++++++++++++++++++++
 2 files changed, 131 insertions(+)

diff --git a/contracts/Managers/RoleManager.sol b/contracts/Managers/RoleManager.sol
index 2cfda95..a3f0320 100644
--- a/contracts/Managers/RoleManager.sol
+++ b/contracts/Managers/RoleManager.sol
@@ -589,6 +589,29 @@ contract RoleManager is Governance2Step {
         emit RemovedVault(_vault);
     }
 
+    /**
+     * @notice Removes a specific role(s) for a `_holder` from the `_vaults`.
+     * @dev Can be used to remove one specific role or multiple.
+     * @param _vaults Array of vaults to adjust.
+     * @param _holder Address who's having a role removed.
+     * @param _role The role or roles to remove from the `_holder`.
+     */
+    function removeRoles(
+        address[] calldata _vaults,
+        address _holder,
+        uint256 _role
+    ) external virtual onlyPositionHolder(DADDY) {
+        address _vault;
+        for (uint256 i = 0; i < _vaults.length; ++i) {
+            _vault = _vaults[i];
+            // Make sure the vault is added to this Role Manager.
+            require(vaultConfig[_vault].asset != address(0), "vault not added");
+
+            // Remove the role.
+            IVault(_vault).remove_role(_holder, _role);
+        }
+    }
+
     /*//////////////////////////////////////////////////////////////
                             SETTERS
     //////////////////////////////////////////////////////////////*/
diff --git a/tests/manager/test_role_manager.py b/tests/manager/test_role_manager.py
index 1ac246c..ecc44ef 100644
--- a/tests/manager/test_role_manager.py
+++ b/tests/manager/test_role_manager.py
@@ -1441,3 +1441,111 @@ def test_remove_vault(
     vault.accept_role_manager(sender=daddy)
 
     assert vault.role_manager() == daddy
+
+
+def test_remove_role(
+    role_manager,
+    daddy,
+    brain,
+    security,
+    keeper,
+    strategy_manager,
+    asset,
+    user,
+    strategy,
+    healthcheck_accountant,
+    registry,
+    release_registry,
+    vault_factory,
+    debt_allocator_factory,
+):
+    setup_role_manager(
+        role_manager=role_manager,
+        release_registry=release_registry,
+        registry=registry,
+        vault_factory=vault_factory,
+        accountant=healthcheck_accountant,
+        daddy=daddy,
+    )
+
+    rating = int(2)
+
+    assert role_manager.getAllVaults() == []
+    assert (
+        role_manager.getVault(asset, vault_factory.apiVersion(), rating) == ZERO_ADDRESS
+    )
+    assert registry.numAssets() == 0
+    assert registry.numEndorsedVaults(asset) == 0
+
+    # Deploy a vault
+    tx = role_manager.newVault(asset, rating, sender=daddy)
+
+    event = list(tx.decode_logs(registry.NewEndorsedVault))[0]
+    vault = project.dependencies["yearn-vaults"]["v3.0.1"].VaultV3.at(event.vault)
+
+    event = list(tx.decode_logs(debt_allocator_factory.NewDebtAllocator))[0]
+    debt_allocator = project.DebtAllocator.at(event.allocator)
+
+    (vault_asset, vault_rating, vault_debt_allocator, index) = role_manager.vaultConfig(
+        vault
+    )
+
+    assert vault_asset == asset
+    assert vault_rating == rating
+    assert vault_debt_allocator == debt_allocator
+    assert index == 0
+    assert role_manager.getAllVaults() == [vault]
+    assert role_manager.getVault(asset, vault_factory.apiVersion(), rating) == vault
+    assert role_manager.vaults(index) == vault
+    assert role_manager.isVaultsRoleManager(vault) == True
+
+    # Check roles
+    assert vault.roles(role_manager) == 0
+    assert vault.roles(daddy) == daddy_roles
+
+    # Remove 1 role and see if the rest remain the same.
+    new_roles = daddy_roles & ~ROLES.ADD_STRATEGY_MANAGER
+
+    with ape.reverts("vault not added"):
+        role_manager.removeRoles(
+            [strategy], daddy, ROLES.ADD_STRATEGY_MANAGER, sender=daddy
+        )
+
+    with ape.reverts("!allowed"):
+        role_manager.removeRoles(
+            [vault], daddy, ROLES.ADD_STRATEGY_MANAGER, sender=user
+        )
+
+    tx = role_manager.removeRoles(
+        [vault], daddy, ROLES.ADD_STRATEGY_MANAGER, sender=daddy
+    )
+
+    event = list(tx.decode_logs(vault.RoleSet))
+
+    assert len(event) == 1
+    assert event[0].account == daddy
+    assert event[0].role == new_roles
+    assert vault.roles(daddy) == new_roles
+
+    with ape.reverts("not allowed"):
+        vault.add_strategy(strategy, sender=daddy)
+
+    # Remove two roles at once
+    to_remove = ROLES.REVOKE_STRATEGY_MANAGER | ROLES.FORCE_REVOKE_MANAGER
+
+    new_roles = new_roles & ~to_remove
+
+    tx = role_manager.removeRoles([vault], daddy, to_remove, sender=daddy)
+
+    event = list(tx.decode_logs(vault.RoleSet))
+
+    assert len(event) == 1
+    assert event[0].account == daddy
+    assert event[0].role == new_roles
+    assert vault.roles(daddy) == new_roles
+
+    with ape.reverts("not allowed"):
+        vault.revoke_strategy(strategy, sender=daddy)
+
+    with ape.reverts("not allowed"):
+        vault.force_revoke_strategy(strategy, sender=daddy)

From ee26e1a03053c389402b7351e39a3c90ac4a9320 Mon Sep 17 00:00:00 2001
From: Schlagonia <schlagonia@gmail.com>
Date: Mon, 5 Feb 2024 16:34:31 -0700
Subject: [PATCH 09/17] fix: factory is immutable

---
 contracts/Managers/RoleManager.sol         |  2 +-
 contracts/debtAllocators/DebtAllocator.sol | 13 ++++++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/contracts/Managers/RoleManager.sol b/contracts/Managers/RoleManager.sol
index a3f0320..88f28eb 100644
--- a/contracts/Managers/RoleManager.sol
+++ b/contracts/Managers/RoleManager.sol
@@ -600,7 +600,7 @@ contract RoleManager is Governance2Step {
         address[] calldata _vaults,
         address _holder,
         uint256 _role
-    ) external virtual onlyPositionHolder(DADDY) {
+    ) external virtual onlyGovernance {
         address _vault;
         for (uint256 i = 0; i < _vaults.length; ++i) {
             _vault = _vaults[i];
diff --git a/contracts/debtAllocators/DebtAllocator.sol b/contracts/debtAllocators/DebtAllocator.sol
index b3d633c..b22b924 100644
--- a/contracts/debtAllocators/DebtAllocator.sol
+++ b/contracts/debtAllocators/DebtAllocator.sol
@@ -107,12 +107,12 @@ contract DebtAllocator {
 
     uint256 internal constant MAX_BPS = 10_000;
 
+    /// @notice Address to get permissioned roles from.
+    address public immutable factory;
+
     /// @notice Address of the vault this serves as allocator for.
     address public vault;
 
-    /// @notice Address to get permissioned roles from.
-    address public factory;
-
     /// @notice Time to wait between debt updates in seconds.
     uint256 public minimumWait;
 
@@ -134,6 +134,10 @@ contract DebtAllocator {
     mapping(address => Config) internal _configs;
 
     constructor(address _vault, uint256 _minimumChange) {
+        // Set the factory to retrieve roles from. Will be the same for all clones so can use immutable.
+        factory = msg.sender;
+
+        // Initialize original version.
         initialize(_vault, _minimumChange);
     }
 
@@ -145,8 +149,7 @@ contract DebtAllocator {
      */
     function initialize(address _vault, uint256 _minimumChange) public virtual {
         require(address(vault) == address(0), "!initialized");
-        // Set the factory to retrieve roles from.
-        factory = msg.sender;
+
         // Set initial variables.
         vault = _vault;
         minimumChange = _minimumChange;

From 83419aabc8fcec70fb2fe7d5e5ea0327afc3139b Mon Sep 17 00:00:00 2001
From: Schlagonia <schlagonia@gmail.com>
Date: Mon, 5 Feb 2024 17:01:14 -0700
Subject: [PATCH 10/17] fix: role allowed

---
 contracts/debtAllocators/YieldManager/YieldManager.sol | 6 +++---
 tests/manager/test_role_manager.py                     | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/contracts/debtAllocators/YieldManager/YieldManager.sol b/contracts/debtAllocators/YieldManager/YieldManager.sol
index 332fdc7..2671918 100644
--- a/contracts/debtAllocators/YieldManager/YieldManager.sol
+++ b/contracts/debtAllocators/YieldManager/YieldManager.sol
@@ -219,7 +219,7 @@ contract YieldManager is Governance {
      */
     function validateAllocation(
         address _vault,
-        Allocation[] memory _newAllocations
+        Allocation[] calldata _newAllocations
     ) external view virtual returns (bool) {
         // Get the total assets the vault has.
         uint256 _totalAssets = IVault(_vault).totalAssets();
@@ -253,7 +253,7 @@ contract YieldManager is Governance {
      */
     function getCurrentAndExpectedRate(
         address _vault,
-        Allocation[] memory _newAllocations
+        Allocation[] calldata _newAllocations
     )
         external
         view
@@ -319,7 +319,7 @@ contract YieldManager is Governance {
      */
     function updateAllocationPermissioned(
         address _vault,
-        Allocation[] memory _newAllocations
+        Allocation[] calldata _newAllocations
     ) external virtual onlyGovernance {
         address allocator = vaultAllocator[_vault];
         require(allocator != address(0), "vault not added");
diff --git a/tests/manager/test_role_manager.py b/tests/manager/test_role_manager.py
index ecc44ef..785b3cd 100644
--- a/tests/manager/test_role_manager.py
+++ b/tests/manager/test_role_manager.py
@@ -1511,7 +1511,7 @@ def test_remove_role(
             [strategy], daddy, ROLES.ADD_STRATEGY_MANAGER, sender=daddy
         )
 
-    with ape.reverts("!allowed"):
+    with ape.reverts("!governance"):
         role_manager.removeRoles(
             [vault], daddy, ROLES.ADD_STRATEGY_MANAGER, sender=user
         )

From 571488375d9da4eb51de8853f62f59e7e1dc3b60 Mon Sep 17 00:00:00 2001
From: Schlagonia <schlagonia@gmail.com>
Date: Tue, 6 Feb 2024 09:12:24 -0700
Subject: [PATCH 11/17] fix: config override

---
 ape-config.yaml | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/ape-config.yaml b/ape-config.yaml
index 59a7dcc..a37dc94 100644
--- a/ape-config.yaml
+++ b/ape-config.yaml
@@ -21,20 +21,27 @@ dependencies:
   - name: yearn-vaults
     github: yearn/yearn-vaults-v3
     ref: v3.0.1
+    config_override:
+      dependencies:
+        - name: tokenized-strategy
+          github: yearn/tokenized-strategy
+          ref: v3.0.1
+          contracts_folder: src
     exclude: 
-     - test/
+     - test/**/*
+     
   - name: tokenized-strategy
     github: yearn/tokenized-strategy
     ref: v3.0.1
     contracts_folder: src
     exclude:
-     - test/
+     - test/**/*
   - name: periphery
     github: yearn/tokenized-strategy-periphery
     ref: master
     contracts_folder: src
     exclude: 
-     - test/
+     - test/**/*
 
 solidity:
   import_remapping:

From 95d784bcbd8249730d3cb82c33e6188f30f09491 Mon Sep 17 00:00:00 2001
From: Schlagonia <schlagonia@gmail.com>
Date: Tue, 6 Feb 2024 09:18:09 -0700
Subject: [PATCH 12/17] fix: include tests

---
 ape-config.yaml | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ape-config.yaml b/ape-config.yaml
index a37dc94..384b2ff 100644
--- a/ape-config.yaml
+++ b/ape-config.yaml
@@ -27,9 +27,7 @@ dependencies:
           github: yearn/tokenized-strategy
           ref: v3.0.1
           contracts_folder: src
-    exclude: 
-     - test/**/*
-     
+
   - name: tokenized-strategy
     github: yearn/tokenized-strategy
     ref: v3.0.1

From c9a2c75e99c69a0a5ce1d2f811761d74a722db0c Mon Sep 17 00:00:00 2001
From: Schlagonia <schlagonia@gmail.com>
Date: Tue, 6 Feb 2024 09:42:41 -0700
Subject: [PATCH 13/17] fix: dont import tests

---
 ape-config.yaml                           |  4 +-
 contracts/Mocks/MockTokenizedStrategy.sol | 80 ++++++++++++++++++++++-
 2 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/ape-config.yaml b/ape-config.yaml
index 384b2ff..a37dc94 100644
--- a/ape-config.yaml
+++ b/ape-config.yaml
@@ -27,7 +27,9 @@ dependencies:
           github: yearn/tokenized-strategy
           ref: v3.0.1
           contracts_folder: src
-
+    exclude: 
+     - test/**/*
+     
   - name: tokenized-strategy
     github: yearn/tokenized-strategy
     ref: v3.0.1
diff --git a/contracts/Mocks/MockTokenizedStrategy.sol b/contracts/Mocks/MockTokenizedStrategy.sol
index 8709463..7564a9b 100644
--- a/contracts/Mocks/MockTokenizedStrategy.sol
+++ b/contracts/Mocks/MockTokenizedStrategy.sol
@@ -1,7 +1,85 @@
 // SPDX-License-Identifier: AGPL-3.0
 pragma solidity 0.8.18;
 
-import {MockTokenizedStrategy} from "@yearn-vaults/test/mocks/ERC4626/MockTokenizedStrategy.sol";
+// SPDX-License-Identifier: GPL-3.0
+pragma solidity 0.8.18;
+
+import {TokenizedStrategy, ERC20} from "@tokenized-strategy/TokenizedStrategy.sol";
+
+contract MockTokenizedStrategy is TokenizedStrategy {
+    uint256 public minDebt;
+    uint256 public maxDebt = type(uint256).max;
+
+    // Private variables and functions used in this mock.
+    bytes32 public constant BASE_STRATEGY_STORAGE =
+        bytes32(uint256(keccak256("yearn.base.strategy.storage")) - 1);
+
+    function strategyStorage() internal pure returns (StrategyData storage S) {
+        // Since STORAGE_SLOT is a constant, we have to put a variable
+        // on the stack to access it from an inline assembly block.
+        bytes32 slot = BASE_STRATEGY_STORAGE;
+        assembly {
+            S.slot := slot
+        }
+    }
+
+    constructor(
+        address _asset,
+        string memory _name,
+        address _management,
+        address _keeper
+    ) {
+        // Cache storage pointer
+        StrategyData storage S = strategyStorage();
+
+        // Set the strategy's underlying asset
+        S.asset = ERC20(_asset);
+        // Set the Strategy Tokens name.
+        S.name = _name;
+        // Set decimals based off the `asset`.
+        S.decimals = ERC20(_asset).decimals();
+
+        // Set last report to this block.
+        S.lastReport = uint128(block.timestamp);
+
+        // Set the default management address. Can't be 0.
+        require(_management != address(0), "ZERO ADDRESS");
+        S.management = _management;
+        S.performanceFeeRecipient = _management;
+        // Set the keeper address
+        S.keeper = _keeper;
+    }
+
+    function setMinDebt(uint256 _minDebt) external {
+        minDebt = _minDebt;
+    }
+
+    function setMaxDebt(uint256 _maxDebt) external {
+        maxDebt = _maxDebt;
+    }
+
+    function availableDepositLimit(
+        address
+    ) public view virtual returns (uint256) {
+        uint256 _totalAssets = strategyStorage().totalIdle;
+        uint256 _maxDebt = maxDebt;
+        return _maxDebt > _totalAssets ? _maxDebt - _totalAssets : 0;
+    }
+
+    function availableWithdrawLimit(
+        address /*_owner*/
+    ) public view virtual returns (uint256) {
+        return type(uint256).max;
+    }
+
+    function deployFunds(uint256 _amount) external virtual {}
+
+    function freeFunds(uint256 _amount) external virtual {}
+
+    function harvestAndReport() external virtual returns (uint256) {
+        return strategyStorage().asset.balanceOf(address(this));
+    }
+}
 
 contract MockTokenized is MockTokenizedStrategy {
     uint256 public apr;

From a3c37e1355d0f6199d71d3c55dfb1810e6e0d23c Mon Sep 17 00:00:00 2001
From: Schlagonia <schlagonia@gmail.com>
Date: Tue, 6 Feb 2024 09:52:18 -0700
Subject: [PATCH 14/17] fix: license

---
 contracts/Mocks/MockTokenizedStrategy.sol | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/contracts/Mocks/MockTokenizedStrategy.sol b/contracts/Mocks/MockTokenizedStrategy.sol
index 7564a9b..8d4dbfb 100644
--- a/contracts/Mocks/MockTokenizedStrategy.sol
+++ b/contracts/Mocks/MockTokenizedStrategy.sol
@@ -1,9 +1,6 @@
 // SPDX-License-Identifier: AGPL-3.0
 pragma solidity 0.8.18;
 
-// SPDX-License-Identifier: GPL-3.0
-pragma solidity 0.8.18;
-
 import {TokenizedStrategy, ERC20} from "@tokenized-strategy/TokenizedStrategy.sol";
 
 contract MockTokenizedStrategy is TokenizedStrategy {

From 2bb42c06f29996e0f625f8efe983bc88d5a46779 Mon Sep 17 00:00:00 2001
From: Schlagonia <schlagonia@gmail.com>
Date: Tue, 6 Feb 2024 13:56:57 -0700
Subject: [PATCH 15/17] fix: mapping is internal

---
 contracts/Managers/RoleManager.sol | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contracts/Managers/RoleManager.sol b/contracts/Managers/RoleManager.sol
index 88f28eb..23a63ee 100644
--- a/contracts/Managers/RoleManager.sol
+++ b/contracts/Managers/RoleManager.sol
@@ -120,7 +120,7 @@ contract RoleManager is Governance2Step {
     mapping(address => VaultConfig) public vaultConfig;
     /// @notice Mapping of underlying asset, api version and rating to vault.
     mapping(address => mapping(string => mapping(uint256 => address)))
-        public _assetToVault;
+        internal _assetToVault;
 
     constructor(
         address _governance,

From 889f3d33aae32d457679ccd6116036e6ef098430 Mon Sep 17 00:00:00 2001
From: Schlagonia <schlagonia@gmail.com>
Date: Tue, 6 Feb 2024 16:35:44 -0700
Subject: [PATCH 16/17] fix: remove config override

---
 ape-config.yaml | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/ape-config.yaml b/ape-config.yaml
index a37dc94..1485aab 100644
--- a/ape-config.yaml
+++ b/ape-config.yaml
@@ -18,15 +18,10 @@ dependencies:
   - name: openzeppelin
     github: OpenZeppelin/openzeppelin-contracts
     ref: 4.8.2
+    
   - name: yearn-vaults
     github: yearn/yearn-vaults-v3
     ref: v3.0.1
-    config_override:
-      dependencies:
-        - name: tokenized-strategy
-          github: yearn/tokenized-strategy
-          ref: v3.0.1
-          contracts_folder: src
     exclude: 
      - test/**/*
      
@@ -36,6 +31,7 @@ dependencies:
     contracts_folder: src
     exclude:
      - test/**/*
+
   - name: periphery
     github: yearn/tokenized-strategy-periphery
     ref: master

From 4b64c9124bba30c8bdc14a5ae64b3f0a2a6c2209 Mon Sep 17 00:00:00 2001
From: Schlag <89420541+Schlagonia@users.noreply.github.com>
Date: Tue, 6 Feb 2024 16:52:56 -0700
Subject: [PATCH 17/17] fix: wording

Co-authored-by: FP <83050944+fp-crypto@users.noreply.github.com>
---
 contracts/debtAllocators/DebtAllocator.sol | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contracts/debtAllocators/DebtAllocator.sol b/contracts/debtAllocators/DebtAllocator.sol
index b22b924..616f58d 100644
--- a/contracts/debtAllocators/DebtAllocator.sol
+++ b/contracts/debtAllocators/DebtAllocator.sol
@@ -421,7 +421,7 @@ contract DebtAllocator {
 
     /**
      * @notice Set the minimum change variable for a strategy.
-     * @dev This is the minimum amount of debt that will have to be
+     * @dev This is the minimum amount of debt to be
      * added or pulled for it to trigger an update.
      *
      * @param _minimumChange The new minimum to set for the strategy.