feat: various improvements

.gitignore

@@ -15,4 +15,7 @@ docs/
 
 # coverage
 lcov.info
-coverage
+coverage
+
+# gas
+.gas-snapshot

src/FourSixTwoSixAgg.sol

@@ -18,8 +18,6 @@ contract FourSixTwoSixAgg is BalanceForwarder, EVCUtil, ERC4626, AccessControlEn
 
     error Reentrancy();
     error ArrayLengthMismatch();
-    error AddressesOutOfOrder();
-    error DuplicateInitialStrategy();
     error InitialAllocationPointsZero();
     error NotEnoughAssets();
     error NegativeYield();
@@ -120,14 +118,16 @@ contract FourSixTwoSixAgg is BalanceForwarder, EVCUtil, ERC4626, AccessControlEn
 
         strategies[address(0)] =
             Strategy({allocated: 0, allocationPoints: uint120(_initialCashAllocationPoints), active: true});
-        totalAllocationPoints += _initialCashAllocationPoints;
+
+        uint256 _totalAllocationPoints = _initialCashAllocationPoints;
 
         for (uint256 i; i < _initialStrategies.length; ++i) {
             strategies[_initialStrategies[i]] =
                 Strategy({allocated: 0, allocationPoints: uint120(_initialStrategiesAllocationPoints[i]), active: true});
 
-            totalAllocationPoints += _initialStrategiesAllocationPoints[i];
+            _totalAllocationPoints += _initialStrategiesAllocationPoints[i];
         }
+        totalAllocationPoints = _totalAllocationPoints;
 
         // Setup DEFAULT_ADMIN
         _grantRole(DEFAULT_ADMIN_ROLE, _msgSender());
@@ -179,9 +179,8 @@ contract FourSixTwoSixAgg is BalanceForwarder, EVCUtil, ERC4626, AccessControlEn
     function harvestMultipleStrategies(address[] calldata _strategies) external nonReentrant {
         for (uint256 i; i < _strategies.length; ++i) {
             _harvest(_strategies[i]);
-
-            _gulp();
         }
+        _gulp();
     }
 
     /// @notice Adjust a certain strategy's allocation points.
@@ -194,17 +193,13 @@ contract FourSixTwoSixAgg is BalanceForwarder, EVCUtil, ERC4626, AccessControlEn
         onlyRole(ALLOCATION_ADJUSTER_ROLE)
     {
         Strategy memory strategyDataCache = strategies[strategy];
-        uint256 totalAllocationPointsCache = totalAllocationPoints;
 
         if (!strategyDataCache.active) {
             revert InactiveStrategy();
         }
 
         strategies[strategy].allocationPoints = uint120(newPoints);
-
-        totalAllocationPoints = (newPoints > strategyDataCache.allocationPoints)
-            ? totalAllocationPointsCache + (newPoints - strategyDataCache.allocationPoints)
-            : totalAllocationPointsCache - (strategyDataCache.allocationPoints - newPoints);
+        totalAllocationPoints = totalAllocationPoints + newPoints - strategyDataCache.allocationPoints;
     }
 
     /// @notice Swap two strategies indexes in the withdrawal queue.
@@ -216,7 +211,8 @@ contract FourSixTwoSixAgg is BalanceForwarder, EVCUtil, ERC4626, AccessControlEn
         nonReentrant
         onlyRole(WITHDRAW_QUEUE_REORDERER_ROLE)
     {
-        if (index1 >= withdrawalQueue.length || index2 >= withdrawalQueue.length) {
+        uint256 length = withdrawalQueue.length;
+        if (index1 >= length || index2 >= length) {
             revert OutOfBounds();
         }
 
@@ -255,21 +251,23 @@ contract FourSixTwoSixAgg is BalanceForwarder, EVCUtil, ERC4626, AccessControlEn
     /// @dev Can only be called by an address that have the STRATEGY_REMOVER_ROLE
     /// @param strategy Address of the strategy
     function removeStrategy(address strategy) external nonReentrant onlyRole(STRATEGY_REMOVER_ROLE) {
-        if (!strategies[strategy].active) {
+        Strategy storage strategyStorage = strategies[strategy];
+
+        if (!strategyStorage.active) {
             revert AlreadyRemoved();
         }
 
-        strategies[strategy].active = false;
-        totalAllocationPoints -= strategies[strategy].allocationPoints;
-        strategies[strategy].allocationPoints = 0;
+        totalAllocationPoints -= strategyStorage.allocationPoints;
+        strategyStorage.active = false;
+        strategyStorage.allocationPoints = 0;
 
         // remove from withdrawalQueue
         uint256 lastStrategyIndex = withdrawalQueue.length - 1;
 
-        for (uint256 i = 0; i <= lastStrategyIndex; i++) {
-            if ((withdrawalQueue[i] == strategy) && (i != lastStrategyIndex)) {
-                (withdrawalQueue[i], withdrawalQueue[lastStrategyIndex]) =
-                    (withdrawalQueue[lastStrategyIndex], withdrawalQueue[i]);
+        for (uint256 i = 0; i < lastStrategyIndex; ++i) {
+            if (withdrawalQueue[i] == strategy) {
+                withdrawalQueue[i] = withdrawalQueue[lastStrategyIndex];
+                withdrawalQueue[lastStrategyIndex] = strategy;
             }
         }
 
@@ -419,7 +417,8 @@ contract FourSixTwoSixAgg is BalanceForwarder, EVCUtil, ERC4626, AccessControlEn
         totalAssetsDeposited -= assets;
 
         uint256 assetsRetrieved = IERC20(asset()).balanceOf(address(this));
-        for (uint256 i; i < withdrawalQueue.length; i++) {
+        uint256 numStrategies = withdrawalQueue.length;
+        for (uint256 i; i < numStrategies; ++i) {
             if (assetsRetrieved >= assets) {
                 break;
             }
@@ -429,16 +428,16 @@ contract FourSixTwoSixAgg is BalanceForwarder, EVCUtil, ERC4626, AccessControlEn
             _harvest(address(strategy));
             _gulp();
 
-            Strategy memory strategyData = strategies[withdrawalQueue[i]];
+            Strategy storage strategyStorage = strategies[address(strategy)];
 
             uint256 sharesBalance = strategy.balanceOf(address(this));
             uint256 underlyingBalance = strategy.convertToAssets(sharesBalance);
 
             uint256 desiredAssets = assets - assetsRetrieved;
-            uint256 withdrawAmount = (underlyingBalance >= desiredAssets) ? desiredAssets : underlyingBalance;
+            uint256 withdrawAmount = (underlyingBalance > desiredAssets) ? desiredAssets : underlyingBalance;
 
             // Update allocated assets
-            strategies[withdrawalQueue[i]].allocated = strategyData.allocated - uint120(withdrawAmount);
+            strategyStorage.allocated -= uint120(withdrawAmount);
             totalAllocated -= withdrawAmount;
 
             // update assetsRetrieved

test/common/FourSixTwoSixAggBase.t.sol

@@ -87,4 +87,14 @@ contract FourSixTwoSixAggBase is EVaultTestBase {
         vm.prank(from);
         fourSixTwoSixAgg.addStrategy(strategy, allocationPoints);
     }
+
+    function _getWithdrawalQueue() internal view returns (address[] memory) {
+        uint256 length = fourSixTwoSixAgg.withdrawalQueueLength();
+
+        address[] memory queue = new address[](length);
+        for (uint256 i = 0; i < length; ++i) {
+            queue[i] = fourSixTwoSixAgg.withdrawalQueue(i);
+        }
+        return queue;
+    }
 }

test/unit/RemoveStrategy.t.sol

@@ -50,6 +50,56 @@ contract RemoveStrategyTest is FourSixTwoSixAggBase {
         assertEq(fourSixTwoSixAgg.withdrawalQueueLength(), withdrawalQueueLengthBefore - 1);
     }
 
+    function testRemoveStrategy_WithdrawalQueueOrdering() public {
+        address strategy1 =
+            factory.createProxy(address(0), true, abi.encodePacked(address(assetTST), address(oracle), unitOfAccount));
+        address strategy2 =
+            factory.createProxy(address(0), true, abi.encodePacked(address(assetTST), address(oracle), unitOfAccount));
+        address strategy3 =
+            factory.createProxy(address(0), true, abi.encodePacked(address(assetTST), address(oracle), unitOfAccount));
+
+        _addStrategy(manager, strategy1, strategyAllocationPoints);
+        _addStrategy(manager, strategy2, strategyAllocationPoints);
+        _addStrategy(manager, strategy3, strategyAllocationPoints);
+
+        address[] memory withdrawalQueue = _getWithdrawalQueue();
+        assertEq(withdrawalQueue.length, 4);
+        assertEq(withdrawalQueue[0], address(eTST));
+        assertEq(withdrawalQueue[1], strategy1);
+        assertEq(withdrawalQueue[2], strategy2);
+        assertEq(withdrawalQueue[3], strategy3);
+
+        vm.prank(manager);
+        fourSixTwoSixAgg.removeStrategy(strategy2);
+
+        withdrawalQueue = _getWithdrawalQueue();
+        assertEq(withdrawalQueue.length, 3);
+        assertEq(withdrawalQueue[0], address(eTST));
+        assertEq(withdrawalQueue[1], strategy1);
+        assertEq(withdrawalQueue[2], strategy3);
+
+        vm.prank(manager);
+        fourSixTwoSixAgg.removeStrategy(strategy3);
+
+        withdrawalQueue = _getWithdrawalQueue();
+        assertEq(withdrawalQueue.length, 2);
+        assertEq(withdrawalQueue[0], address(eTST));
+        assertEq(withdrawalQueue[1], strategy1);
+
+        vm.prank(manager);
+        fourSixTwoSixAgg.removeStrategy(address(eTST));
+
+        withdrawalQueue = _getWithdrawalQueue();
+        assertEq(withdrawalQueue.length, 1);
+        assertEq(withdrawalQueue[0], strategy1);
+
+        vm.prank(manager);
+        fourSixTwoSixAgg.removeStrategy(strategy1);
+
+        withdrawalQueue = _getWithdrawalQueue();
+        assertEq(withdrawalQueue.length, 0);
+    }
+
     function testRemoveStrategy_fromUnauthorized() public {
         vm.prank(deployer);
         vm.expectRevert();