Add additional safety checks to rebalancing

.env.sepolia.example

@@ -35,7 +35,6 @@ CONTROLLER_REWARDS_COLLECTOR=0xFCFc06C6E1862Ee8e482359A15d8F137b8a072a1
 # Controller operators (acting addresses during deployment)
 # -----------------------------------------------------------------------------
 CONTROLLER_PROXY_ADMIN=0xFCFc06C6E1862Ee8e482359A15d8F137b8a072a1
-CONTROLLER_SWAPPER_OWNER=0xFCFc06C6E1862Ee8e482359A15d8F137b8a072a1
 CONTROLLER_VAULT_OWNER=0xFCFc06C6E1862Ee8e482359A15d8F137b8a072a1
 
 # -----------------------------------------------------------------------------

.github/workflows/ci.yml

@@ -20,6 +20,8 @@ jobs:
 
       - name: "Install Foundry"
         uses: "foundry-rs/foundry-toolchain@v1"
+        with:
+          version: "v1.4.3"
 
       - name: "Install dependencies"
         run: "forge install"
@@ -38,6 +40,8 @@ jobs:
 
       - name: "Install Foundry"
         uses: "foundry-rs/foundry-toolchain@v1"
+        with:
+          version: "v1.4.3"
 
       - name: "Install dependencies"
         run: "forge install"

script/Deploy.s.sol

@@ -40,7 +40,6 @@ contract Deploy is BaseScript {
 
     struct ControllerOperators {
         address proxyAdmin;
-        address swapperOwner;
         address vaultOwner;
     }
 
@@ -101,9 +100,7 @@ contract Deploy is BaseScript {
         );
         deployment.gunit = new GenericUnit(address(deployment.controller), "Generic Unit USD", "GU_USD");
 
-        deployment.swapper = new OneInchSwapper(
-            controllerOperators.swapperOwner, IOneInchAggregationRouterLike(externalAddresses.oneInchRouter)
-        );
+        deployment.swapper = new OneInchSwapper(IOneInchAggregationRouterLike(externalAddresses.oneInchRouter));
 
         deployment.controller
             .initialize(
@@ -209,7 +206,6 @@ contract Deploy is BaseScript {
 
         console2.log("-- Controller Operators --");
         console2.log("Proxy admin:", controllerOperators.proxyAdmin);
-        console2.log("Swapper owner:", controllerOperators.swapperOwner);
         console2.log("Vault owner:", controllerOperators.vaultOwner);
 
         console2.log("-- Controller Entities --");
@@ -251,9 +247,7 @@ contract Deploy is BaseScript {
 
     function _controllerOperators() internal view returns (ControllerOperators memory operators) {
         operators = ControllerOperators({
-            proxyAdmin: vm.envAddress("CONTROLLER_PROXY_ADMIN"),
-            swapperOwner: vm.envAddress("CONTROLLER_SWAPPER_OWNER"),
-            vaultOwner: vm.envAddress("CONTROLLER_VAULT_OWNER")
+            proxyAdmin: vm.envAddress("CONTROLLER_PROXY_ADMIN"), vaultOwner: vm.envAddress("CONTROLLER_VAULT_OWNER")
         });
     }
 

src/controller/RebalancingManager.sol

@@ -47,6 +47,14 @@ abstract contract RebalancingManager is BaseController, AccountingLogic {
      * @notice Thrown when the received amount is less than the minimum expected amount
      */
     error Rebalance_SlippageTooHigh();
+    /**
+     * @notice Thrown when the total share supply changes during a rebalance operation
+     */
+    error Rebalance_ShareSupplyChanged();
+    /**
+     * @notice Thrown when the actual received amount does not match the expected amount after a swap
+     */
+    error Rebalance_IncorrectToAmount();
 
     /**
      * @notice Internal initializer for the RebalancingManager contract
@@ -83,24 +91,25 @@ abstract contract RebalancingManager is BaseController, AccountingLogic {
         address toAsset = IControlledVault(toVault).asset();
 
         uint256 toAmount = fromAsset == toAsset
-            ? _rebalanceSameAssets(fromVault, toVault, fromAsset, fromAmount)
-            : _rebalanceDiffAssets(fromVault, toVault, fromAsset, toAsset, fromAmount, minToAmount, swapperData);
+            ? _rebalanceSameAssets(fromVault, fromAsset, fromAmount, toVault)
+            : _rebalanceDiffAssets(fromVault, fromAsset, fromAmount, toVault, toAsset, minToAmount, swapperData);
 
         emit Rebalanced(fromVault, toVault, fromAmount, toAmount);
     }
 
     /**
      * @dev Internal function to rebalance the same asset type between two vaults
      * @param fromVault The address of the vault to withdraw assets from
-     * @param toVault The address of the vault to deposit assets to
+     * @param asset The address of the asset being rebalanced
      * @param amount The amount of assets to rebalance between vaults
+     * @param toVault The address of the vault to deposit assets to
      * @return toAmount The actual amount of assets that were successfully rebalanced
      */
     function _rebalanceSameAssets(
         address fromVault,
-        address toVault,
         address asset,
-        uint256 amount
+        uint256 amount,
+        address toVault
     )
         internal
         returns (uint256 toAmount)
@@ -117,31 +126,38 @@ abstract contract RebalancingManager is BaseController, AccountingLogic {
      * - Protocol-wide backing value slippage protection
      * - Safety buffer validation to ensure losses don't exceed acceptable limits
      * @param fromVault The address of the vault to withdraw assets from
-     * @param toVault The address of the vault to deposit assets to
      * @param fromAsset The address of the asset being withdrawn from the source vault
-     * @param toAsset The address of the asset being deposited to the destination vault
      * @param fromAmount The amount of assets to withdraw from the source vault
+     * @param toVault The address of the vault to deposit assets to
+     * @param toAsset The address of the asset being deposited to the destination vault
      * @param minToAmount The minimum amount of destination assets expected (slippage protection)
      * @param swapperData Additional data passed to the swapper for asset conversion
      * @return toAmount The actual amount of destination assets received and deposited
      */
     function _rebalanceDiffAssets(
         address fromVault,
-        address toVault,
         address fromAsset,
-        address toAsset,
         uint256 fromAmount,
+        address toVault,
+        address toAsset,
         uint256 minToAmount,
         bytes calldata swapperData
     )
         internal
         returns (uint256 toAmount)
     {
-        // Store original backing value for slippage calculations
         uint256 originalBackingValue = backingAssetsValue();
+        uint256 originalShareSupply = _share.totalSupply();
+        uint256 originalToVaultBalance = IERC20(toAsset).balanceOf(toVault);
 
         IControlledVault(fromVault).controllerWithdraw(fromAsset, fromAmount, address(_swapper));
         toAmount = _swapper.swap(fromAsset, fromAmount, toAsset, minToAmount, toVault, swapperData);
+
+        // Note: Check swap amount before deposit because balanceOf returns the amount of unallocated assets
+
+        require(originalShareSupply == _share.totalSupply(), Rebalance_ShareSupplyChanged());
+        require(IERC20(toAsset).balanceOf(toVault) - originalToVaultBalance == toAmount, Rebalance_IncorrectToAmount());
+
         IControlledVault(toVault).controllerDeposit(toAmount);
 
         // Individual swap slippage protection

src/periphery/swapper/OneInchSwapper.sol

@@ -2,7 +2,6 @@
 pragma solidity 0.8.29;
 
 import { SafeERC20, IERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
-import { Ownable2Step, Ownable } from "@openzeppelin/contracts/access/Ownable2Step.sol";
 
 import { ISwapper } from "../../interfaces/ISwapper.sol";
 import { IOneInchAggregationRouterLike } from "../../interfaces/IOneInchAggregationRouterLike.sol";
@@ -12,24 +11,14 @@ import { IOneInchAggregationRouterLike } from "../../interfaces/IOneInchAggregat
  * @notice A swapper implementation that integrates with 1inch Aggregation Router for token swaps
  * @dev Implements the ISwapper interface to provide token swapping functionality through 1inch protocol
  */
-contract OneInchSwapper is Ownable2Step, ISwapper {
+contract OneInchSwapper is ISwapper {
     using SafeERC20 for IERC20;
 
     /**
      * @notice The 1inch Aggregation Router contract used for executing swaps
      */
     IOneInchAggregationRouterLike public immutable oneInchRouter;
 
-    /**
-     * @notice This mapping tracks which addresses are permitted to execute 1inch swap operations
-     */
-    mapping(address executor => bool) public allowedExecutors;
-
-    /**
-     * @notice Emitted when an executor's authorization status is updated
-     */
-    event ExecutorAuthorizationUpdated(address indexed executor, bool allowed);
-
     /**
      * @notice Thrown when a zero address is provided where a valid address is required
      */
@@ -50,10 +39,6 @@ contract OneInchSwapper is Ownable2Step, ISwapper {
      * @notice Thrown when the swap does not use the entire input amount
      */
     error PartialFill();
-    /**
-     * @notice Thrown when an unauthorized executor attempts to perform a swap
-     */
-    error UnauthorizedExecutor();
     /**
      * @notice Thrown when the swap description parameters do not match the expected values
      */
@@ -63,7 +48,7 @@ contract OneInchSwapper is Ownable2Step, ISwapper {
      * @notice Initializes the OneInchSwapper with the 1inch router address
      * @param _oneInchRouter The address of the 1inch Aggregation Router contract
      */
-    constructor(address owner, IOneInchAggregationRouterLike _oneInchRouter) Ownable(owner) {
+    constructor(IOneInchAggregationRouterLike _oneInchRouter) {
         oneInchRouter = _oneInchRouter;
     }
 
@@ -100,7 +85,6 @@ contract OneInchSwapper is Ownable2Step, ISwapper {
         (address executor, IOneInchAggregationRouterLike.SwapDescription memory desc, bytes memory swapData) =
             abi.decode(swapperParams[4:], (address, IOneInchAggregationRouterLike.SwapDescription, bytes));
 
-        require(allowedExecutors[executor], UnauthorizedExecutor());
         require(desc.srcToken == assetIn, InvalidSwapDescription());
         require(desc.dstToken == assetOut, InvalidSwapDescription());
         require(desc.amount == amountIn, InvalidSwapDescription());
@@ -117,15 +101,4 @@ contract OneInchSwapper is Ownable2Step, ISwapper {
 
         return amountOut;
     }
-
-    /**
-     * @dev Updates the allowance status for an executor address
-     * @param executor The address to update the allowance for
-     * @param allowed True to allow the address to be used as executor of the swap, false to disallow
-     */
-    function setAllowedExecutor(address executor, bool allowed) external onlyOwner {
-        require(executor != address(0), ZeroAddress());
-        allowedExecutors[executor] = allowed;
-        emit ExecutorAuthorizationUpdated(executor, allowed);
-    }
 }

tests/fork/OneInchSwapper.fork.t.sol

@@ -8,24 +8,19 @@ import { OneInchSwapper, IOneInchAggregationRouterLike, IERC20 } from "../../src
 abstract contract OneInchSwapperForkTest is Test {
     IOneInchAggregationRouterLike constant SWAP_ROUTER =
         IOneInchAggregationRouterLike(0x111111125421cA6dc452d289314280a0f8842A65);
-    address constant EXECUTOR = 0x5141B82f5fFDa4c6fE1E372978F1C5427640a190;
 
     IERC20 constant USDC = IERC20(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48);
     IERC20 constant USDT = IERC20(0xdAC17F958D2ee523a2206206994597C13D831ec7);
     IERC20 constant USDS = IERC20(0xdC035D45d973E3EC169d2276DDab16f1e407384F);
 
     OneInchSwapper swapper;
 
-    address owner = makeAddr("owner");
     address user = makeAddr("user");
 
     function setUp() public virtual {
         vm.createSelectFork("mainnet");
 
-        swapper = new OneInchSwapper(owner, SWAP_ROUTER);
-
-        vm.prank(owner);
-        swapper.setAllowedExecutor(EXECUTOR, true);
+        swapper = new OneInchSwapper(SWAP_ROUTER);
     }
 }
 

tests/unit/controller/Controller.rebalancingManager.t.sol

@@ -35,6 +35,7 @@ contract Controller_RebalancingManager_Rebalance_Test is Controller_RebalancingM
     uint256 toAmount = 100e18;
     uint256 minToAmount = 10e18;
     bytes swapperData;
+    bytes[] balances;
 
     function _mockVaultAsset(address vault, address asset) internal {
         vm.mockCall(vault, abi.encodeWithSelector(IControlledVault.asset.selector), abi.encode(asset));
@@ -50,6 +51,11 @@ contract Controller_RebalancingManager_Rebalance_Test is Controller_RebalancingM
         vm.mockCall(fromAsset, abi.encodeWithSelector(IERC20.transfer.selector), abi.encode(true));
         vm.mockCall(toAsset, abi.encodeWithSelector(IERC20.transfer.selector), abi.encode(true));
 
+        balances = new bytes[](2);
+        balances[0] = abi.encode(0);
+        balances[1] = abi.encode(toAmount);
+        vm.mockCalls(toAsset, abi.encodeWithSelector(IERC20.balanceOf.selector, toVault), balances);
+
         vm.mockCall(address(share), abi.encodeWithSelector(IERC20.totalSupply.selector), abi.encode(1800e18));
     }
 
@@ -137,7 +143,34 @@ contract Controller_RebalancingManager_Rebalance_Test is Controller_RebalancingM
         controller.rebalance(fromVault, fromAmount, toVault, minToAmount, swapperData);
     }
 
+    function testFuzz_shouldRevert_whenShareSupplyChanges(uint256 _newSupply) public {
+        vm.assume(_newSupply != 1800e18);
+
+        bytes[] memory supplies = new bytes[](2);
+        supplies[0] = abi.encode(1800e18);
+        supplies[1] = abi.encode(_newSupply);
+
+        vm.mockCalls(address(share), abi.encodeWithSelector(IERC20.totalSupply.selector), supplies);
+
+        vm.prank(manager);
+        vm.expectRevert(RebalancingManager.Rebalance_ShareSupplyChanged.selector);
+        controller.rebalance(fromVault, fromAmount, toVault, minToAmount, swapperData);
+    }
+
+    function testFuzz_shouldRevert_whenSwapperReturnValueNotMatchingBalanceChange(uint256 _toAmount) public {
+        vm.assume(_toAmount != toAmount);
+
+        balances[1] = abi.encode(_toAmount);
+        vm.mockCalls(toAsset, abi.encodeWithSelector(IERC20.balanceOf.selector, toVault), balances);
+
+        vm.prank(manager);
+        vm.expectRevert(RebalancingManager.Rebalance_IncorrectToAmount.selector);
+        controller.rebalance(fromVault, fromAmount, toVault, minToAmount, swapperData);
+    }
+
     function test_shouldRevert_whenToAmountLessThanMinToAmount() public {
+        balances[1] = abi.encode(minToAmount - 1);
+        vm.mockCalls(toAsset, abi.encodeWithSelector(IERC20.balanceOf.selector, toVault), balances);
         vm.mockCall(address(swapper), abi.encodeWithSelector(ISwapper.swap.selector), abi.encode(minToAmount - 1));
 
         vm.prank(manager);

tests/unit/periphery/swapper/OneInchSwapper.t.sol

@@ -13,7 +13,6 @@ import {
 abstract contract OneInchSwapperTest is Test {
     OneInchSwapper swapper;
 
-    address owner = makeAddr("owner");
     address router = makeAddr("router");
     address assetIn = makeAddr("assetIn");
     address assetOut = makeAddr("assetOut");
@@ -32,7 +31,7 @@ abstract contract OneInchSwapperTest is Test {
     }
 
     function setUp() public virtual {
-        swapper = new OneInchSwapper(owner, IOneInchAggregationRouterLike(router));
+        swapper = new OneInchSwapper(IOneInchAggregationRouterLike(router));
 
         vm.mockCall(assetIn, abi.encodeWithSelector(IERC20.approve.selector), abi.encode(true));
         vm.mockCall(
@@ -53,12 +52,6 @@ abstract contract OneInchSwapperTest is Test {
 }
 
 contract OneInchSwapper_Swap_Test is OneInchSwapperTest {
-    function setUp() public override {
-        super.setUp();
-        vm.prank(owner);
-        swapper.setAllowedExecutor(executor, true);
-    }
-
     function test_shouldRevert_whenAssetInIsZeroAddress() public {
         vm.expectRevert(OneInchSwapper.ZeroAddress.selector);
         swapper.swap(address(0), amountIn, assetOut, minAmountOut, recipient, swapperParams);
@@ -84,14 +77,6 @@ contract OneInchSwapper_Swap_Test is OneInchSwapperTest {
         swapper.swap(assetIn, amountIn, assetOut, 0, recipient, swapperParams);
     }
 
-    function test_shouldRevert_whenExecutorIsNotAllowed() public {
-        executor = makeAddr("notAllowedExecutor");
-        swapperParams = _encodeSwapperParams();
-
-        vm.expectRevert(OneInchSwapper.UnauthorizedExecutor.selector);
-        swapper.swap(assetIn, amountIn, assetOut, minAmountOut, recipient, swapperParams);
-    }
-
     function test_shouldRevert_whenSrcTokenDoesNotMatchAssetIn() public {
         desc.srcToken = makeAddr("differentAssetIn");
         swapperParams = _encodeSwapperParams();
@@ -178,41 +163,3 @@ contract OneInchSwapper_Swap_Test is OneInchSwapperTest {
         assertEq(result, amountOut);
     }
 }
-
-contract OneInchSwapper_SetAllowedExecutor_Test is OneInchSwapperTest {
-    function test_shouldRevert_whenNotOwner() public {
-        vm.expectRevert();
-        vm.prank(makeAddr("notOwner"));
-        swapper.setAllowedExecutor(executor, true);
-    }
-
-    function test_shouldRevert_whenExecutorIsZeroAddress() public {
-        vm.expectRevert(OneInchSwapper.ZeroAddress.selector);
-        vm.prank(owner);
-        swapper.setAllowedExecutor(address(0), true);
-    }
-
-    function test_shouldSetAllowedExecutor() public {
-        vm.prank(owner);
-        swapper.setAllowedExecutor(executor, true);
-        assertTrue(swapper.allowedExecutors(executor));
-
-        vm.prank(owner);
-        swapper.setAllowedExecutor(executor, false);
-        assertFalse(swapper.allowedExecutors(executor));
-    }
-
-    function test_shouldEmit_ExecutorAuthorizationUpdated() public {
-        vm.expectEmit();
-        emit OneInchSwapper.ExecutorAuthorizationUpdated(executor, true);
-
-        vm.prank(owner);
-        swapper.setAllowedExecutor(executor, true);
-
-        vm.expectEmit();
-        emit OneInchSwapper.ExecutorAuthorizationUpdated(executor, false);
-
-        vm.prank(owner);
-        swapper.setAllowedExecutor(executor, false);
-    }
-}