MasterVault rebalance slippage

contracts/tokenbridge/libraries/vault/MasterVault.sol

@@ -35,7 +35,9 @@
 ///         i.e. if the underlying asset has 6 decimals, the MasterVault will have 24 decimals.
 ///
 ///         For a subVault to be compatible with the MasterVault, it must adhere to the following:
+///         - must be fully ERC4626 compliant
 ///         - previewMint and previewDeposit must not be manipulable
+///         - deposit and withdraw must not be manipulable / sandwichable
 ///         - previewMint and previewDeposit must be roughly linear with respect to amounts.
 ///           Superlinear previewMint or sublinear previewDeposit may cause the MasterVault to overcharge on deposits and underpay on withdrawals.
 ///         - must not have deposit / withdrawal fees (because rebalancing can happen frequently)
@@ -82,6 +84,10 @@
     );
     error PerformanceFeeUnchanged(bool enabled);
     error RebalanceCooldownTooLow(uint32 requested, uint32 minimum);
+    error RebalanceExchRateTooLow(
+        int256 minExchRateWad, int256 deltaAssets, uint256 subVaultShares
+    );
+    error RebalanceExchRateWrongSign(int256 minExchRateWad);
 
     /*
     Storage layout notes:
@@ -255,56 +261,94 @@
     /// @dev    Will revert if the cooldown period has not passed
     ///         Will revert if the target allocation is already met
     ///         Will revert if the amount to deposit/withdraw is less than the minimumRebalanceAmount.
-    function rebalance() external whenNotPaused nonReentrant onlyRole(KEEPER_ROLE) {
+    /// @param  minExchRateWad Minimum exchange rate (1e18 * deltaAssets / abs(subVaultShares)) for the deposit/withdraw operation
+    ///                        Negative indicates a subVault deposit (negative deltaAssets),
+    ///                        positive indicates a subVault withdraw (positive deltaAssets).
+    function rebalance(int256 minExchRateWad)
+        external
+        whenNotPaused
+        nonReentrant
+        onlyRole(KEEPER_ROLE)
+    {
         // Check cooldown
         uint256 timeSinceLastRebalance = block.timestamp - lastRebalanceTime;
         if (timeSinceLastRebalance < rebalanceCooldown) {
             revert RebalanceCooldownNotMet(timeSinceLastRebalance, rebalanceCooldown);
         }
 
         uint256 totalAssetsUp = _totalAssetsLessProfit(MathUpgradeable.Rounding.Up);
         uint256 totalAssetsDown = _totalAssetsLessProfit(MathUpgradeable.Rounding.Down);
         uint256 idleTargetUp =
             totalAssetsUp.mulDiv(1e18 - targetAllocationWad, 1e18, MathUpgradeable.Rounding.Up);
         uint256 idleTargetDown =
             totalAssetsDown.mulDiv(1e18 - targetAllocationWad, 1e18, MathUpgradeable.Rounding.Down);
         uint256 idleBalance = asset.balanceOf(address(this));
 
         if (idleTargetDown <= idleBalance && idleBalance <= idleTargetUp) {
             revert TargetAllocationMet();
         }
 
         if (idleBalance < idleTargetDown) {
-            // we need to withdraw from subvault
+            // we are withdrawing, so slippage tolerance must be non negative
+            if (minExchRateWad < 0) {
+                revert RebalanceExchRateWrongSign(minExchRateWad);
+            }
+
             uint256 desiredWithdraw = idleTargetDown - idleBalance;
             uint256 maxWithdrawable = subVault.maxWithdraw(address(this));
             uint256 withdrawAmount =
                 desiredWithdraw < maxWithdrawable ? desiredWithdraw : maxWithdrawable;
+
             if (withdrawAmount < minimumRebalanceAmount) {
                 revert RebalanceAmountTooSmall(
                     false, withdrawAmount, desiredWithdraw, minimumRebalanceAmount
                 );
             }
-            subVault.withdraw(withdrawAmount, address(this), address(this));
+
+            uint256 subVaultShares = subVault.withdraw(withdrawAmount, address(this), address(this));
+            uint256 actualExchRate =
+                withdrawAmount.mulDiv(1e18, subVaultShares, MathUpgradeable.Rounding.Down);
+
+            if (actualExchRate < uint256(minExchRateWad)) {
+                revert RebalanceExchRateTooLow(
+                    minExchRateWad, int256(withdrawAmount), subVaultShares
+                );
+            }
+
             emit Rebalanced(false, desiredWithdraw, withdrawAmount);
         } else {
-            // we need to deposit into subvault
+            // we are depositing, so slippage tolerance must be non positive
+            if (minExchRateWad > 0) {
+                revert RebalanceExchRateWrongSign(minExchRateWad);
+            }
+
             uint256 desiredDeposit = idleBalance - idleTargetUp;
             uint256 maxDepositable = subVault.maxDeposit(address(this));
             uint256 depositAmount =
                 desiredDeposit < maxDepositable ? desiredDeposit : maxDepositable;
+
             if (depositAmount < minimumRebalanceAmount) {
                 revert RebalanceAmountTooSmall(
                     true, depositAmount, desiredDeposit, minimumRebalanceAmount
                 );
             }
+
             asset.safeApprove(address(subVault), depositAmount);
-            subVault.deposit(depositAmount, address(this));
+            uint256 subVaultShares = subVault.deposit(depositAmount, address(this));
+            uint256 actualExchRate =
+                depositAmount.mulDiv(1e18, subVaultShares, MathUpgradeable.Rounding.Up);
+
+            if (actualExchRate > uint256(-minExchRateWad)) {
+                revert RebalanceExchRateTooLow(
+                    minExchRateWad, -int256(depositAmount), subVaultShares
+                );
+            }
+
             emit Rebalanced(true, desiredDeposit, depositAmount);
         }
 
         lastRebalanceTime = uint40(block.timestamp);
     }
 
     /// @notice Distribute performance fees to the beneficiary
     function distributePerformanceFee() external whenNotPaused nonReentrant onlyRole(KEEPER_ROLE) {
@@ -374,7 +418,7 @@
     }
 
     /// @notice Toggle performance fee collection on/off
-    ///         When enabling, principalPriceWad snaps to the current price. 
+    ///         When enabling, principalPriceWad snaps to the current price.
     ///         When price increases afterwards, profit is earned for the beneficiary.
     ///         If disabling, any pending performance fees are distributed immediately.
     /// @param enabled True to enable performance fees, false to disable
@@ -409,7 +453,7 @@
         emit BeneficiaryUpdated(oldBeneficiary, newBeneficiary);
     }
 
-    /// @notice Add or remove a subvault from the whitelist. 
+    /// @notice Add or remove a subvault from the whitelist.
     ///         Malicious, misconfigured, or buggy subVaults may cause total loss of funds.
     /// @param _subVault The subvault address to update
     /// @param _whitelisted True to whitelist the subvault, false to remove it

contracts/tokenbridge/libraries/vault/MasterVaultRoles.sol

@@ -4,6 +4,7 @@ pragma solidity ^0.8.0;
 import {
     AccessControlEnumerableUpgradeable
 } from "@openzeppelin/contracts-upgradeable/access/AccessControlEnumerableUpgradeable.sol";
+
 //todo: document trust levels and rug abilities
 /// @notice Roles system for MasterVaults.
 ///         Each MasterVault will have a reference to a singleton MasterVaultRoles contract, in addition to inheriting MasterVaultRoles directly.

test-foundry/libraries/vault/scenarios/MasterVaultScenario01.t.sol

@@ -82,7 +82,7 @@ contract MasterVaultScenario01Test is MasterVaultScenarioCoreTest {
         // Step 2: User B deposits 300 USDC
         uint256 sharesB = _deposit(userB, 300);
 
-        vault.rebalance();
+        vault.rebalance(type(int256).min + 1);
 
         // Verify intermediate state
         user = userA;

test-foundry/libraries/vault/scenarios/MasterVaultScenario02.t.sol

@@ -86,7 +86,7 @@ contract MasterVaultScenario02Test is MasterVaultScenarioCoreTest {
         // Step 2: User B deposits 300 USDC
         uint256 sharesB = _deposit(userB, 300);
 
-        vault.rebalance();
+        vault.rebalance(type(int256).min + 1);
 
         // Verify intermediate state
         user = userA;

test-foundry/libraries/vault/scenarios/MasterVaultScenario03.t.sol

@@ -101,7 +101,7 @@ contract MasterVaultScenario03Test is MasterVaultScenarioCoreTest {
         // Step 2: User B deposits 300 USDC
         uint256 sharesB = _deposit(userB, 300);
 
-        vault.rebalance();
+        vault.rebalance(type(int256).min + 1);
 
         // Verify intermediate state
         user = userA;

test-foundry/libraries/vault/scenarios/MasterVaultScenario04.t.sol

@@ -105,7 +105,7 @@ contract MasterVaultScenario04Test is MasterVaultScenarioCoreTest {
         // Step 2: User B deposits 300 USDC
         uint256 sharesB1 = _deposit(userB, 300);
 
-        vault.rebalance();
+        vault.rebalance(type(int256).min + 1);
 
         // Step 3: Subvault wins 100 USDC (25% profit)
         _simulateProfit(100);

test-foundry/libraries/vault/scenarios/MasterVaultScenario05.t.sol

@@ -153,7 +153,7 @@ contract MasterVaultScenario05Test is MasterVaultScenarioCoreTest {
         // Step 2: User B deposits 300 USDC
         _deposit(userB, 300);
 
-        vault.rebalance();
+        vault.rebalance(type(int256).min + 1);
 
         // Step 3: Subvault wins 100 USDC
         _simulateProfit(100);
@@ -172,7 +172,7 @@ contract MasterVaultScenario05Test is MasterVaultScenarioCoreTest {
         _deposit(userB, 300);
 
         vm.warp(block.timestamp + 2);
-        vault.rebalance();
+        vault.rebalance(type(int256).min + 1);
 
         // Step 7: User A redeems 200 shares
         _redeem(userA, 200 * DEAD_SHARES);
@@ -187,7 +187,7 @@ contract MasterVaultScenario05Test is MasterVaultScenarioCoreTest {
         _deposit(userB, 300);
 
         vm.warp(block.timestamp + 2);
-        vault.rebalance();
+        vault.rebalance(type(int256).min + 1);
 
         // Step 11: Subvault loses 100 USDC (25% loss)
         _simulateLoss(100);

test-foundry/libraries/vault/scenarios/MasterVaultScenario06.t.sol

@@ -110,7 +110,7 @@ contract MasterVaultScenario06Test is MasterVaultScenarioCoreTest {
         // Step 2: User B deposits 300 USDC
         uint256 sharesB = _deposit(userB, 300);
 
-        vault.rebalance();
+        vault.rebalance(type(int256).min + 1);
 
         // Verify intermediate state 1
         user = userA;

test-foundry/libraries/vault/scenarios/MasterVaultScenario07.t.sol

@@ -150,7 +150,7 @@ contract MasterVaultScenario07Test is MasterVaultScenarioCoreTest {
         // Step 2: User B deposits 300 USDC
         uint256 sharesB = _deposit(userB, 300);
 
-        vault.rebalance();
+        vault.rebalance(type(int256).min + 1);
 
         // Verify intermediate state 1
         user = userA;
@@ -216,7 +216,7 @@ contract MasterVaultScenario07Test is MasterVaultScenarioCoreTest {
         uint256 sharesA2 = _deposit(userA, 100);
 
         vm.warp(block.timestamp + 2);
-        vault.rebalance();
+        vault.rebalance(type(int256).min + 1);
 
         // Verify intermediate state 3
         user = userA;

test-foundry/libraries/vault/scenarios/MasterVaultScenario08.t.sol

@@ -156,7 +156,7 @@ contract MasterVaultScenario08Test is MasterVaultScenarioCoreTest {
         // Step 2: User B deposits 300 USDC
         uint256 sharesB1 = _deposit(userB, 300);
 
-        vault.rebalance();
+        vault.rebalance(type(int256).min + 1);
 
         // Verify intermediate state 1
         user = userA;
@@ -198,7 +198,7 @@ contract MasterVaultScenario08Test is MasterVaultScenarioCoreTest {
         uint256 sharesB2 = _deposit(userB, 300);
 
         vm.warp(block.timestamp + 2);
-        vault.rebalance();
+        vault.rebalance(type(int256).min + 1);
 
         // Calculate expected shares for second deposit
         // After loss, totalAssets = 301, totalSupply = 401 * DEAD_SHARES

test-foundry/libraries/vault/scenarios/MasterVaultScenario09.t.sol

@@ -115,7 +115,7 @@ contract MasterVaultScenario09Test is MasterVaultScenarioCoreTest {
         // Step 2: User B deposits 300 USDC
         uint256 sharesB = _deposit(userB, 300);
 
-        vault.rebalance();
+        vault.rebalance(type(int256).min + 1);
 
         // Verify intermediate state 1
         user = userA;

test-foundry/libraries/vault/scenarios/MasterVaultScenario10.t.sol

@@ -136,7 +136,7 @@ contract MasterVaultScenario10Test is MasterVaultScenarioCoreTest {
         // Step 2: User B deposits 300 USDC
         uint256 sharesB = _deposit(userB, 300);
 
-        vault.rebalance();
+        vault.rebalance(type(int256).min + 1);
 
         // Verify intermediate state 1
         user = userA;
@@ -164,7 +164,7 @@ contract MasterVaultScenario10Test is MasterVaultScenarioCoreTest {
         uint256 sharesC = _deposit(userC, 100);
 
         vm.warp(block.timestamp + 2);
-        vault.rebalance();
+        vault.rebalance(type(int256).min + 1);
 
         // C should get 100 * DEAD_SHARES because they deposit at principal value
         assertEq(sharesC, 100 * DEAD_SHARES, "User C should get 100 shares at principal price");