From 08bbe58150c73263fa98f3a8b106c7e38191fd1a Mon Sep 17 00:00:00 2001
From: Prateek <prateek@perennial.finance>
Date: Mon, 23 Sep 2024 15:14:59 +0530
Subject: [PATCH 01/14] fix claim risk fee

---
 packages/perennial-extensions/contracts/Coordinator.sol         | 2 +-
 .../test/unit/Coordinator/Coordinator.test.ts                   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/packages/perennial-extensions/contracts/Coordinator.sol b/packages/perennial-extensions/contracts/Coordinator.sol
index 92d646fa3..442212ded 100644
--- a/packages/perennial-extensions/contracts/Coordinator.sol
+++ b/packages/perennial-extensions/contracts/Coordinator.sol
@@ -36,7 +36,7 @@ contract Coordinator is ICoordinator, Ownable {
     /// @param market The market to claim the fee for
     function claimFee(IMarket market) external {
         if (msg.sender != comptroller) revert NotComptroller();
-        market.claimFee(msg.sender);
+        market.claimFee(address(this));
         market.token().push(comptroller);
     }
 
diff --git a/packages/perennial-extensions/test/unit/Coordinator/Coordinator.test.ts b/packages/perennial-extensions/test/unit/Coordinator/Coordinator.test.ts
index 88c1f3c3b..353f27e19 100644
--- a/packages/perennial-extensions/test/unit/Coordinator/Coordinator.test.ts
+++ b/packages/perennial-extensions/test/unit/Coordinator/Coordinator.test.ts
@@ -110,7 +110,7 @@ describe('Coordinator', () => {
     it('should call claimFee on the market', async () => {
       await coordinatorContract.setComptroller(comptroller.address)
       await coordinatorContract.connect(comptroller).claimFee(market.address)
-      expect(market.claimFee).to.have.been.calledWith(comptroller.address)
+      expect(market.claimFee).to.have.been.calledWith(coordinatorContract.address)
       expect(token.transfer).to.have.been.calledWith(comptroller.address, 0)
     })
   })

From fecfb8d893a4a8925d7672fb2693e177e01b19a8 Mon Sep 17 00:00:00 2001
From: Ed Noepel <ed@noepel.net>
Date: Fri, 6 Sep 2024 17:04:42 -0400
Subject: [PATCH 02/14] fix issue rebalancing empty markets

---
 .../contracts/libs/RebalanceLib.sol           |  8 +-
 .../test/integration/Controller.ts            | 90 +++++++++++++++++++
 2 files changed, 96 insertions(+), 2 deletions(-)

diff --git a/packages/perennial-account/contracts/libs/RebalanceLib.sol b/packages/perennial-account/contracts/libs/RebalanceLib.sol
index 30528579a..293d39ade 100644
--- a/packages/perennial-account/contracts/libs/RebalanceLib.sol
+++ b/packages/perennial-account/contracts/libs/RebalanceLib.sol
@@ -2,7 +2,7 @@
 pragma solidity ^0.8.13;
 
 import { Fixed6, Fixed6Lib } from "@equilibria/root/number/types/Fixed6.sol";
-import { UFixed6 } from "@equilibria/root/number/types/UFixed6.sol";
+import { UFixed6, UFixed6Lib } from "@equilibria/root/number/types/UFixed6.sol";
 import { IController } from "../interfaces/IController.sol";
 import { RebalanceConfig } from "../types/RebalanceConfig.sol";
 
@@ -24,7 +24,11 @@ library RebalanceLib {
         Fixed6 targetCollateral = groupCollateral.mul(Fixed6Lib.from(marketConfig.target));
 
         // if market is empty, prevent divide-by-zero condition
-        if (marketCollateral.eq(Fixed6Lib.ZERO)) return (false, targetCollateral);
+        if (marketCollateral.eq(Fixed6Lib.ZERO)) {
+            // can rebalance if market is not supposed to be empty and there is collateral elsewhere in the group
+            canRebalance = !marketConfig.target.eq(UFixed6Lib.ZERO) && !groupCollateral.eq(Fixed6Lib.ZERO);
+            return (canRebalance, targetCollateral);
+        }
         // calculate percentage difference between target and actual collateral
         Fixed6 pctFromTarget = Fixed6Lib.ONE.sub(targetCollateral.div(marketCollateral));
         // if this percentage exceeds the configured threshold, the market may be rebelanced
diff --git a/packages/perennial-account/test/integration/Controller.ts b/packages/perennial-account/test/integration/Controller.ts
index df448db4d..0055c7655 100644
--- a/packages/perennial-account/test/integration/Controller.ts
+++ b/packages/perennial-account/test/integration/Controller.ts
@@ -318,6 +318,96 @@ describe('ControllerBase', () => {
       expect(groupCollateral).to.equal(parse6decimal('15000'))
       expect(canRebalance).to.be.false
     })
+
+    it('rebalances markets with no collateral when others are within threshold', async () => {
+      // reconfigure group such that ETH market has threshold higher than it's imbalance
+      const message = {
+        group: 1,
+        markets: [ethMarket.address, btcMarket.address],
+        configs: [
+          { target: parse6decimal('0.9'), threshold: parse6decimal('0.15') },
+          { target: parse6decimal('0.1'), threshold: parse6decimal('0.03') },
+        ],
+        maxFee: constants.Zero,
+        ...(await createAction(userA.address)),
+      }
+      const signature = await signRebalanceConfigChange(userA, verifier, message)
+      await expect(controller.connect(keeper).changeRebalanceConfigWithSignature(message, signature)).to.not.be.reverted
+
+      // transfer funds only to the ETH market
+      await transfer(parse6decimal('10000'), userA, ethMarket)
+
+      await expect(controller.rebalanceGroup(userA.address, 1, TX_OVERRIDES))
+        .to.emit(dsu, 'Transfer')
+        .withArgs(ethMarket.address, accountA.address, utils.parseEther('1000'))
+        .to.emit(dsu, 'Transfer')
+        .withArgs(accountA.address, btcMarket.address, utils.parseEther('1000'))
+        .to.emit(controller, 'GroupRebalanced')
+        .withArgs(userA.address, 1)
+
+      // ensure group collateral unchanged and cannot rebalance
+      const [groupCollateral, canRebalance] = await controller.callStatic.checkGroup(userA.address, 1)
+      expect(groupCollateral).to.equal(parse6decimal('10000'))
+      expect(canRebalance).to.be.false
+    })
+
+    it('should not rebalance empty market configured to be empty', async () => {
+      // reconfigure group such that BTC market is empty
+      const message = {
+        group: 1,
+        markets: [ethMarket.address, btcMarket.address],
+        configs: [
+          { target: parse6decimal('1'), threshold: parse6decimal('0.05') },
+          { target: parse6decimal('0'), threshold: parse6decimal('0.05') },
+        ],
+        maxFee: constants.Zero,
+        ...(await createAction(userA.address)),
+      }
+      const signature = await signRebalanceConfigChange(userA, verifier, message)
+      await expect(controller.connect(keeper).changeRebalanceConfigWithSignature(message, signature)).to.not.be.reverted
+
+      // transfer funds to the ETH market
+      await transfer(parse6decimal('2500'), userA, ethMarket)
+
+      // ensure group balanced
+      await expect(controller.rebalanceGroup(userA.address, 1, TX_OVERRIDES)).to.be.revertedWithCustomError(
+        controller,
+        'ControllerGroupBalancedError',
+      )
+    })
+
+    it('should rebalance non-empty market configured to be empty', async () => {
+      // reconfigure group such that BTC market is empty
+      const message = {
+        group: 1,
+        markets: [ethMarket.address, btcMarket.address],
+        configs: [
+          { target: parse6decimal('1'), threshold: parse6decimal('0.05') },
+          { target: parse6decimal('0'), threshold: parse6decimal('0.05') },
+        ],
+        maxFee: constants.Zero,
+        ...(await createAction(userA.address)),
+      }
+      const signature = await signRebalanceConfigChange(userA, verifier, message)
+      await expect(controller.connect(keeper).changeRebalanceConfigWithSignature(message, signature)).to.not.be.reverted
+
+      // transfer funds to both markets
+      await transfer(parse6decimal('2500'), userA, ethMarket)
+      await transfer(parse6decimal('2500'), userA, btcMarket)
+
+      await expect(controller.rebalanceGroup(userA.address, 1, TX_OVERRIDES))
+        .to.emit(dsu, 'Transfer')
+        .withArgs(btcMarket.address, accountA.address, utils.parseEther('2500'))
+        .to.emit(dsu, 'Transfer')
+        .withArgs(accountA.address, ethMarket.address, utils.parseEther('2500'))
+        .to.emit(controller, 'GroupRebalanced')
+        .withArgs(userA.address, 1)
+
+      // ensure group collateral unchanged and cannot rebalance
+      const [groupCollateral, canRebalance] = await controller.callStatic.checkGroup(userA.address, 1)
+      expect(groupCollateral).to.equal(parse6decimal('5000'))
+      expect(canRebalance).to.be.false
+    })
   })
 
   describe('#transfer', () => {

From 32da68226e186ccb0017168a9216c722aa476a03 Mon Sep 17 00:00:00 2001
From: Ed Noepel <ed@noepel.net>
Date: Wed, 11 Sep 2024 18:25:47 -0400
Subject: [PATCH 03/14] changed ratio to compare market to target, instead of
 target to market

---
 .../contracts/libs/RebalanceLib.sol               | 15 +++++++--------
 .../contracts/types/RebalanceConfig.sol           |  2 +-
 .../test/integration/Controller.ts                |  4 ++++
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/packages/perennial-account/contracts/libs/RebalanceLib.sol b/packages/perennial-account/contracts/libs/RebalanceLib.sol
index 293d39ade..390aa419b 100644
--- a/packages/perennial-account/contracts/libs/RebalanceLib.sol
+++ b/packages/perennial-account/contracts/libs/RebalanceLib.sol
@@ -23,15 +23,14 @@ library RebalanceLib {
         // determine how much collateral the market should have
         Fixed6 targetCollateral = groupCollateral.mul(Fixed6Lib.from(marketConfig.target));
 
-        // if market is empty, prevent divide-by-zero condition
-        if (marketCollateral.eq(Fixed6Lib.ZERO)) {
-            // can rebalance if market is not supposed to be empty and there is collateral elsewhere in the group
-            canRebalance = !marketConfig.target.eq(UFixed6Lib.ZERO) && !groupCollateral.eq(Fixed6Lib.ZERO);
-            return (canRebalance, targetCollateral);
+        // if target is zero, prevent divide-by-zero condition
+        if (targetCollateral.eq(Fixed6Lib.ZERO)) {
+            // can rebalance if market is not empty
+            return (!marketCollateral.eq(Fixed6Lib.ZERO), marketCollateral.mul(Fixed6Lib.NEG_ONE));
         }
-        // calculate percentage difference between target and actual collateral
-        Fixed6 pctFromTarget = Fixed6Lib.ONE.sub(targetCollateral.div(marketCollateral));
-        // if this percentage exceeds the configured threshold, the market may be rebelanced
+        // calculate percentage difference between actual and target collateral
+        Fixed6 pctFromTarget = Fixed6Lib.ONE.sub(marketCollateral.div(targetCollateral));
+        // if this percentage exceeds the configured threshold, the market may be rebalanced
         canRebalance = pctFromTarget.abs().gt(marketConfig.threshold);
 
         // return negative number for surplus, positive number for deficit
diff --git a/packages/perennial-account/contracts/types/RebalanceConfig.sol b/packages/perennial-account/contracts/types/RebalanceConfig.sol
index 55d594132..b19968524 100644
--- a/packages/perennial-account/contracts/types/RebalanceConfig.sol
+++ b/packages/perennial-account/contracts/types/RebalanceConfig.sol
@@ -7,7 +7,7 @@ import { UFixed6, UFixed6Lib } from "@equilibria/root/number/types/UFixed6.sol";
 struct RebalanceConfig {
     /// @dev Percentage of collateral from the group to deposit into the market
     UFixed6 target;
-    /// @dev Percentage away from the target at which keepers may rebalance
+    /// @dev Ratio of market collateral to target at which keepers may rebalance
     UFixed6 threshold;
 }
 
diff --git a/packages/perennial-account/test/integration/Controller.ts b/packages/perennial-account/test/integration/Controller.ts
index 0055c7655..b5209a5de 100644
--- a/packages/perennial-account/test/integration/Controller.ts
+++ b/packages/perennial-account/test/integration/Controller.ts
@@ -295,6 +295,10 @@ describe('ControllerBase', () => {
     })
 
     it('handles groups with no collateral', async () => {
+      const [groupCollateral, canRebalance] = await controller.callStatic.checkGroup(userA.address, 1)
+      expect(groupCollateral).to.equal(0)
+      expect(canRebalance).to.be.false
+
       await expect(controller.rebalanceGroup(userA.address, 1, TX_OVERRIDES)).to.be.revertedWithCustomError(
         controller,
         'ControllerGroupBalancedError',

From b38f76b1cb9ea1fc3057d5237bd34ac530f632fe Mon Sep 17 00:00:00 2001
From: Ed Noepel <ed@noepel.net>
Date: Mon, 23 Sep 2024 10:58:07 -0400
Subject: [PATCH 04/14] created test POC

---
 .../test/integration/Controller_Arbitrum.ts   | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/packages/perennial-account/test/integration/Controller_Arbitrum.ts b/packages/perennial-account/test/integration/Controller_Arbitrum.ts
index c0c271151..e66fd4d11 100644
--- a/packages/perennial-account/test/integration/Controller_Arbitrum.ts
+++ b/packages/perennial-account/test/integration/Controller_Arbitrum.ts
@@ -580,6 +580,59 @@ describe('Controller_Arbitrum', () => {
       const keeperFeePaid = (await dsu.balanceOf(keeper.address)).sub(keeperBalanceBefore)
       expect(keeperFeePaid).to.equal(utils.parseEther('0.00923'))
     })
+
+    it('cannot award more keeper fees than collateral rebalanced', async () => {
+      const ethMarket = market
+
+      // create a new group with two markets
+      const message = {
+        group: 4,
+        markets: [ethMarket.address, btcMarket.address],
+        configs: [
+          { target: parse6decimal('0.5'), threshold: parse6decimal('0.05') },
+          { target: parse6decimal('0.5'), threshold: parse6decimal('0.05') },
+        ],
+        maxFee: DEFAULT_MAX_FEE,
+        ...(await createAction(userA.address)),
+      }
+      const signature = await signRebalanceConfigChange(userA, accountVerifier, message)
+      await expect(controller.connect(keeper).changeRebalanceConfigWithSignature(message, signature, TX_OVERRIDES)).to
+        .not.be.reverted
+
+      const dustAmount = parse6decimal('0.000001')
+      const iterations = 3
+      await dsu
+        .connect(keeper)
+        .approve(ethMarket.address, dustAmount.mul(1e12).mul(iterations), { maxFeePerGas: 100000000 })
+      const keeperBalanceBefore = await dsu.balanceOf(keeper.address)
+
+      for (let i = 0; i < iterations; ++i) {
+        // keeper dusts one of the markets
+        await market
+          .connect(keeper)
+          ['update(address,uint256,uint256,uint256,int256,bool)'](
+            userA.address,
+            constants.MaxUint256,
+            constants.MaxUint256,
+            constants.MaxUint256,
+            dustAmount,
+            false,
+            { maxFeePerGas: 150000000 },
+          )
+        expect((await market.locals(userA.address)).collateral).to.equal(dustAmount)
+
+        // keeper rebalances
+        await expect(controller.connect(keeper).rebalanceGroup(userA.address, 4, TX_OVERRIDES))
+          .to.emit(controller, 'GroupRebalanced')
+          .withArgs(userA.address, 4)
+          .to.emit(controller, 'KeeperCall')
+          .withArgs(keeper.address, anyValue, 0, anyValue, anyValue, anyValue)
+      }
+
+      // FIXME: keeper should not earn more fees than what they dusted
+      const keeperFeePaid = (await dsu.balanceOf(keeper.address)).sub(keeperBalanceBefore)
+      expect(keeperFeePaid).to.be.lessThan(dustAmount.mul(1e12).mul(iterations))
+    })
   })
 
   describe('#withdrawal', async () => {

From 3f0eaf516845768393888f3cec7fa3d05a038946 Mon Sep 17 00:00:00 2001
From: Ed Noepel <ed@noepel.net>
Date: Mon, 23 Sep 2024 16:49:06 -0400
Subject: [PATCH 05/14] prevent rebalance smaller than maxFee

---
 .../contracts/Controller.sol                  |  7 +-
 .../contracts/libs/RebalanceLib.sol           | 17 ++--
 .../contracts/types/RebalanceConfigChange.sol |  3 +-
 .../test/integration/Controller_Arbitrum.ts   | 80 +++++++++++--------
 4 files changed, 65 insertions(+), 42 deletions(-)

diff --git a/packages/perennial-account/contracts/Controller.sol b/packages/perennial-account/contracts/Controller.sol
index da573894e..b3ac19cb2 100644
--- a/packages/perennial-account/contracts/Controller.sol
+++ b/packages/perennial-account/contracts/Controller.sol
@@ -104,7 +104,12 @@ contract Controller is Factory, IController {
             IMarket market = groupToMarkets[owner][group][i];
             RebalanceConfig memory marketRebalanceConfig = _rebalanceConfigs[owner][group][address(market)];
             (bool canMarketRebalance, Fixed6 imbalance) =
-                RebalanceLib.checkMarket(marketRebalanceConfig, groupCollateral, actualCollateral[i]);
+                RebalanceLib.checkMarket(
+                    marketRebalanceConfig,
+                    groupToMaxRebalanceFee[owner][group],
+                    groupCollateral,
+                    actualCollateral[i]
+                );
             imbalances[i] = imbalance;
             canRebalance = canRebalance || canMarketRebalance;
         }
diff --git a/packages/perennial-account/contracts/libs/RebalanceLib.sol b/packages/perennial-account/contracts/libs/RebalanceLib.sol
index 390aa419b..75d673888 100644
--- a/packages/perennial-account/contracts/libs/RebalanceLib.sol
+++ b/packages/perennial-account/contracts/libs/RebalanceLib.sol
@@ -11,29 +11,36 @@ import { RebalanceConfig } from "../types/RebalanceConfig.sol";
 library RebalanceLib {
     /// @dev Compares actual market collateral for owner with their account's target
     /// @param marketConfig Rebalance group configuration for this market
+    /// @param maxFee Maximum fee paid to keeper for rebalancing the group
     /// @param groupCollateral Owner's collateral across all markets in the group
     /// @param marketCollateral Owner's actual amount of collateral in this market
     /// @return canRebalance True if actual collateral in this market is outside of configured threshold
     /// @return imbalance Amount which needs to be transferred to balance the market
     function checkMarket(
         RebalanceConfig memory marketConfig,
+        UFixed6 maxFee,
         Fixed6 groupCollateral,
         Fixed6 marketCollateral
-    ) internal pure returns (bool canRebalance, Fixed6 imbalance) {
+    ) internal view returns (bool canRebalance, Fixed6 imbalance) {
         // determine how much collateral the market should have
         Fixed6 targetCollateral = groupCollateral.mul(Fixed6Lib.from(marketConfig.target));
 
         // if target is zero, prevent divide-by-zero condition
         if (targetCollateral.eq(Fixed6Lib.ZERO)) {
-            // can rebalance if market is not empty
-            return (!marketCollateral.eq(Fixed6Lib.ZERO), marketCollateral.mul(Fixed6Lib.NEG_ONE));
+            imbalance = marketCollateral.mul(Fixed6Lib.NEG_ONE);
+            // can rebalance if market is not empty and imbalance exceeds max fee paid to keeper
+            canRebalance = !marketCollateral.eq(Fixed6Lib.ZERO) && imbalance.abs().gt(maxFee);
+            return (canRebalance, imbalance);
         }
         // calculate percentage difference between actual and target collateral
         Fixed6 pctFromTarget = Fixed6Lib.ONE.sub(marketCollateral.div(targetCollateral));
-        // if this percentage exceeds the configured threshold, the market may be rebalanced
-        canRebalance = pctFromTarget.abs().gt(marketConfig.threshold);
 
         // return negative number for surplus, positive number for deficit
         imbalance = targetCollateral.sub(marketCollateral);
+
+        // if this percentage exceeds the configured threshold,
+        // and the amount to rebalance exceeds max fee paid to keeper, the market may be rebalanced
+        canRebalance = pctFromTarget.abs().gt(marketConfig.threshold)
+            && imbalance.abs().gt(maxFee);
     }
 }
\ No newline at end of file
diff --git a/packages/perennial-account/contracts/types/RebalanceConfigChange.sol b/packages/perennial-account/contracts/types/RebalanceConfigChange.sol
index 642d2eb45..d7797528c 100644
--- a/packages/perennial-account/contracts/types/RebalanceConfigChange.sol
+++ b/packages/perennial-account/contracts/types/RebalanceConfigChange.sol
@@ -15,7 +15,8 @@ struct RebalanceConfigChange {
     address[] markets;
     /// @dev Target allocation for markets in the aforementioned array
     RebalanceConfig[] configs;
-    /// @dev Largest amount to compensate a relayer/keeper for rebalancing the group in DSU
+    /// @dev Largest amount to compensate a relayer/keeper for rebalancing the group in DSU.
+    /// This amount also prevents keepers from rebalancing imbalances smaller than the keeper fee.
     UFixed6 maxFee;
     /// @dev Common information for collateral account actions
     Action action;
diff --git a/packages/perennial-account/test/integration/Controller_Arbitrum.ts b/packages/perennial-account/test/integration/Controller_Arbitrum.ts
index e66fd4d11..0a5dd7866 100644
--- a/packages/perennial-account/test/integration/Controller_Arbitrum.ts
+++ b/packages/perennial-account/test/integration/Controller_Arbitrum.ts
@@ -58,7 +58,8 @@ import { IKeeperOracle, IOracleFactory, PythFactory } from '@equilibria/perennia
 const { ethers } = HRE
 
 const CHAINLINK_ETH_USD_FEED = '0x639Fe6ab55C921f74e7fac1ee960C0B6293ba612' // price feed used for keeper compensation
-const DEFAULT_MAX_FEE = utils.parseEther('0.5')
+const DEFAULT_MAX_FEE_6 = parse6decimal('0.5')
+const DEFAULT_MAX_FEE = DEFAULT_MAX_FEE_6.mul(1e12)
 
 // hack around issues estimating gas for instrumented contracts when running tests under coverage
 const TX_OVERRIDES = { gasLimit: 3_000_000, maxFeePerGas: 200_000_000 }
@@ -88,7 +89,7 @@ describe('Controller_Arbitrum', () => {
   function createAction(
     userAddress: Address,
     signerAddress = userAddress,
-    maxFee = DEFAULT_MAX_FEE,
+    maxFee = DEFAULT_MAX_FEE_6,
     expiresInSeconds = 45,
   ) {
     return {
@@ -484,7 +485,7 @@ describe('Controller_Arbitrum', () => {
         group: 5,
         markets: [market.address],
         configs: [{ target: parse6decimal('1'), threshold: parse6decimal('0.0901') }],
-        maxFee: DEFAULT_MAX_FEE,
+        maxFee: DEFAULT_MAX_FEE_6,
         ...(await createAction(userA.address)),
       }
       const signature = await signRebalanceConfigChange(userA, accountVerifier, message)
@@ -512,7 +513,7 @@ describe('Controller_Arbitrum', () => {
           { target: parse6decimal('0.5'), threshold: parse6decimal('0.05') },
           { target: parse6decimal('0.5'), threshold: parse6decimal('0.05') },
         ],
-        maxFee: DEFAULT_MAX_FEE,
+        maxFee: DEFAULT_MAX_FEE_6,
         ...(await createAction(userA.address)),
       }
       const signature = await signRebalanceConfigChange(userA, accountVerifier, message)
@@ -592,46 +593,55 @@ describe('Controller_Arbitrum', () => {
           { target: parse6decimal('0.5'), threshold: parse6decimal('0.05') },
           { target: parse6decimal('0.5'), threshold: parse6decimal('0.05') },
         ],
-        maxFee: DEFAULT_MAX_FEE,
+        maxFee: DEFAULT_MAX_FEE_6,
         ...(await createAction(userA.address)),
       }
       const signature = await signRebalanceConfigChange(userA, accountVerifier, message)
       await expect(controller.connect(keeper).changeRebalanceConfigWithSignature(message, signature, TX_OVERRIDES)).to
         .not.be.reverted
 
-      const dustAmount = parse6decimal('0.000001')
-      const iterations = 3
-      await dsu
+      let dustAmount = parse6decimal('0.000001')
+      await dsu.connect(keeper).approve(ethMarket.address, dustAmount.mul(1e12), TX_OVERRIDES)
+
+      // keeper dusts one of the markets
+      await ethMarket
         .connect(keeper)
-        .approve(ethMarket.address, dustAmount.mul(1e12).mul(iterations), { maxFeePerGas: 100000000 })
-      const keeperBalanceBefore = await dsu.balanceOf(keeper.address)
+        ['update(address,uint256,uint256,uint256,int256,bool)'](
+          userA.address,
+          constants.MaxUint256,
+          constants.MaxUint256,
+          constants.MaxUint256,
+          dustAmount,
+          false,
+          { maxFeePerGas: 150000000 },
+        )
+      expect((await ethMarket.locals(userA.address)).collateral).to.equal(dustAmount)
 
-      for (let i = 0; i < iterations; ++i) {
-        // keeper dusts one of the markets
-        await market
-          .connect(keeper)
-          ['update(address,uint256,uint256,uint256,int256,bool)'](
-            userA.address,
-            constants.MaxUint256,
-            constants.MaxUint256,
-            constants.MaxUint256,
-            dustAmount,
-            false,
-            { maxFeePerGas: 150000000 },
-          )
-        expect((await market.locals(userA.address)).collateral).to.equal(dustAmount)
-
-        // keeper rebalances
-        await expect(controller.connect(keeper).rebalanceGroup(userA.address, 4, TX_OVERRIDES))
-          .to.emit(controller, 'GroupRebalanced')
-          .withArgs(userA.address, 4)
-          .to.emit(controller, 'KeeperCall')
-          .withArgs(keeper.address, anyValue, 0, anyValue, anyValue, anyValue)
-      }
+      // keeper cannot rebalance because dust did not exceed maxFee
+      await expect(
+        controller.connect(keeper).rebalanceGroup(userA.address, 4, TX_OVERRIDES),
+      ).to.be.revertedWithCustomError(controller, 'ControllerGroupBalancedError')
 
-      // FIXME: keeper should not earn more fees than what they dusted
-      const keeperFeePaid = (await dsu.balanceOf(keeper.address)).sub(keeperBalanceBefore)
-      expect(keeperFeePaid).to.be.lessThan(dustAmount.mul(1e12).mul(iterations))
+      // keeper dusts the other market, such that target is nonzero, and percentage exceeded
+      dustAmount = parse6decimal('0.000003')
+      await dsu.connect(keeper).approve(btcMarket.address, dustAmount.mul(1e12), TX_OVERRIDES)
+      await btcMarket
+        .connect(keeper)
+        ['update(address,uint256,uint256,uint256,int256,bool)'](
+          userA.address,
+          constants.MaxUint256,
+          constants.MaxUint256,
+          constants.MaxUint256,
+          dustAmount,
+          false,
+          { maxFeePerGas: 150000000 },
+        )
+      expect((await btcMarket.locals(userA.address)).collateral).to.equal(dustAmount)
+
+      // keeper still cannot rebalance because dust did not exceed maxFee
+      await expect(
+        controller.connect(keeper).rebalanceGroup(userA.address, 4, TX_OVERRIDES),
+      ).to.be.revertedWithCustomError(controller, 'ControllerGroupBalancedError')
     })
   })
 

From 60c4017d0e63fda2e0eb848d8d03eeb6846ddbe4 Mon Sep 17 00:00:00 2001
From: Prateek <prateek@perennial.finance>
Date: Tue, 24 Sep 2024 17:30:56 +0530
Subject: [PATCH 06/14] fix public immutable state

---
 packages/perennial-account/contracts/Controller.sol   |  4 ++--
 .../contracts/Controller_Arbitrum.sol                 |  5 ++---
 .../contracts/Controller_Incentivized.sol             | 11 ++++++-----
 packages/perennial-order/contracts/Manager.sol        | 10 +++++-----
 4 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/packages/perennial-account/contracts/Controller.sol b/packages/perennial-account/contracts/Controller.sol
index da573894e..7f90cd608 100644
--- a/packages/perennial-account/contracts/Controller.sol
+++ b/packages/perennial-account/contracts/Controller.sol
@@ -31,10 +31,10 @@ contract Controller is Factory, IController {
     uint256 constant MAX_MARKETS_PER_GROUP = 4;
 
     /// @dev USDC stablecoin address
-    Token6 public USDC; // solhint-disable-line var-name-mixedcase
+    Token6 public immutable USDC; // solhint-disable-line var-name-mixedcase
 
     /// @dev DSU address
-    Token18 public DSU; // solhint-disable-line var-name-mixedcase
+    Token18 public immutable DSU; // solhint-disable-line var-name-mixedcase
 
     /// @inheritdoc IController
     IMarketFactory public marketFactory;
diff --git a/packages/perennial-account/contracts/Controller_Arbitrum.sol b/packages/perennial-account/contracts/Controller_Arbitrum.sol
index e4de8a3df..4627466f6 100644
--- a/packages/perennial-account/contracts/Controller_Arbitrum.sol
+++ b/packages/perennial-account/contracts/Controller_Arbitrum.sol
@@ -11,9 +11,8 @@ import { Controller_Incentivized } from "./Controller_Incentivized.sol";
 contract Controller_Arbitrum is Controller_Incentivized, Kept_Arbitrum {
     /// @dev Creates instance of Controller which compensates keepers
     /// @param implementation Pristine collateral account contract
-    /// @param keepConfig Configuration used to compensate keepers
-    constructor(address implementation, KeepConfig memory keepConfig, IVerifierBase nonceManager)
-    Controller_Incentivized(implementation, keepConfig, nonceManager) {}
+    constructor(address implementation, IVerifierBase nonceManager)
+    Controller_Incentivized(implementation, nonceManager) {}
 
     /// @dev Use the Kept_Arbitrum implementation for calculating the dynamic fee
     function _calldataFee(
diff --git a/packages/perennial-account/contracts/Controller_Incentivized.sol b/packages/perennial-account/contracts/Controller_Incentivized.sol
index 5cbfe5d23..dfaeeb380 100644
--- a/packages/perennial-account/contracts/Controller_Incentivized.sol
+++ b/packages/perennial-account/contracts/Controller_Incentivized.sol
@@ -36,14 +36,12 @@ abstract contract Controller_Incentivized is Controller, IRelayer, Kept {
     KeepConfig public keepConfig;
 
     /// @dev Handles relayed messages for nonce cancellation
-    IVerifierBase public nonceManager;
+    IVerifierBase public immutable nonceManager;
 
     /// @dev Creates instance of Controller which compensates keepers
     /// @param implementation_ Pristine collateral account contract
-    /// @param keepConfig_ Configuration used to compensate keepers
-    constructor(address implementation_, KeepConfig memory keepConfig_, IVerifierBase nonceManager_)
+    constructor(address implementation_, IVerifierBase nonceManager_)
     Controller(implementation_) {
-        keepConfig = keepConfig_;
         nonceManager = nonceManager_;
     }
 
@@ -51,15 +49,18 @@ abstract contract Controller_Incentivized is Controller, IRelayer, Kept {
     /// @param marketFactory_ Contract used to validate delegated signers
     /// @param verifier_ Contract used to validate collateral account message signatures
     /// @param chainlinkFeed_ ETH-USD price feed used for calculating keeper compensation
+    /// @param keepConfig_ Configuration used to compensate keepers
     function initialize(
         IMarketFactory marketFactory_,
         IAccountVerifier verifier_,
-        AggregatorV3Interface chainlinkFeed_
+        AggregatorV3Interface chainlinkFeed_,
+        KeepConfig memory keepConfig_
     ) external initializer(1) {
         __Factory__initialize();
         __Kept__initialize(chainlinkFeed_, DSU);
         marketFactory = marketFactory_;
         verifier = verifier_;
+        keepConfig = keepConfig_;
     }
 
     /// @inheritdoc IController
diff --git a/packages/perennial-order/contracts/Manager.sol b/packages/perennial-order/contracts/Manager.sol
index 71f8b71f7..62945d60c 100644
--- a/packages/perennial-order/contracts/Manager.sol
+++ b/packages/perennial-order/contracts/Manager.sol
@@ -29,14 +29,14 @@ abstract contract Manager is IManager, Kept {
     /// @dev DSU Reserve address
     IEmptySetReserve public immutable reserve;
 
-    /// @dev Configuration used for keeper compensation
-    KeepConfig public keepConfig;
-
     /// @dev Contract used to validate delegated signers
-    IMarketFactory public marketFactory;
+    IMarketFactory public immutable marketFactory;
 
     /// @dev Verifies EIP712 messages for this extension
-    IOrderVerifier public verifier;
+    IOrderVerifier public immutable verifier;
+
+    /// @dev Configuration used for keeper compensation
+    KeepConfig public keepConfig;
 
     /// @dev Stores trigger orders while awaiting their conditions to become true
     /// Market => Account => Nonce => Order

From cad8dc5ce470f90868543ffb5df0db0004be849a Mon Sep 17 00:00:00 2001
From: Prateek <prateek@perennial.finance>
Date: Tue, 24 Sep 2024 18:19:03 +0530
Subject: [PATCH 07/14] fix tests

---
 packages/perennial-account/test/helpers/arbitrumHelpers.ts | 2 --
 .../test/integration/Controller_Arbitrum.ts                | 7 ++++---
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/packages/perennial-account/test/helpers/arbitrumHelpers.ts b/packages/perennial-account/test/helpers/arbitrumHelpers.ts
index 3401e4121..ab8ef219e 100644
--- a/packages/perennial-account/test/helpers/arbitrumHelpers.ts
+++ b/packages/perennial-account/test/helpers/arbitrumHelpers.ts
@@ -152,7 +152,6 @@ export async function deployAndInitializeController(
 // deploys an instance of the Controller with Arbitrum-specific keeper compensation mechanisms
 export async function deployControllerArbitrum(
   owner: SignerWithAddress,
-  keepConfig: IKept.KeepConfigStruct,
   nonceManager: IVerifier,
   overrides?: CallOverrides,
 ): Promise<Controller_Arbitrum> {
@@ -160,7 +159,6 @@ export async function deployControllerArbitrum(
   accountImpl.initialize(constants.AddressZero)
   const controller = await new Controller_Arbitrum__factory(owner).deploy(
     accountImpl.address,
-    keepConfig,
     nonceManager.address,
     overrides ?? {},
   )
diff --git a/packages/perennial-account/test/integration/Controller_Arbitrum.ts b/packages/perennial-account/test/integration/Controller_Arbitrum.ts
index c0c271151..526479c0a 100644
--- a/packages/perennial-account/test/integration/Controller_Arbitrum.ts
+++ b/packages/perennial-account/test/integration/Controller_Arbitrum.ts
@@ -185,13 +185,14 @@ describe('Controller_Arbitrum', () => {
       bufferCalldata: 500_000,
     }
     const marketVerifier = IVerifier__factory.connect(await marketFactory.verifier(), owner)
-    controller = await deployControllerArbitrum(owner, keepConfig, marketVerifier, { maxFeePerGas: 100000000 })
+    controller = await deployControllerArbitrum(owner, marketVerifier, { maxFeePerGas: 100000000 })
     accountVerifier = await new AccountVerifier__factory(owner).deploy({ maxFeePerGas: 100000000 })
     // chainlink feed is used by Kept for keeper compensation
-    await controller['initialize(address,address,address)'](
+    await controller['initialize(address,address,address,(uint256,uint256,uint256,uint256))'](
       marketFactory.address,
       accountVerifier.address,
       CHAINLINK_ETH_USD_FEED,
+      keepConfig,
     )
     // fund userA
     await fundWallet(userA)
@@ -676,7 +677,7 @@ describe('Controller_Arbitrum', () => {
     afterEach(async () => {
       // confirm keeper earned their fee
       const keeperFeePaid = (await dsu.balanceOf(keeper.address)).sub(keeperBalanceBefore)
-      expect(keeperFeePaid).to.be.within(utils.parseEther('0.001'), DEFAULT_MAX_FEE)
+      expect(keeperFeePaid).to.be.within(utils.parseEther('0'), DEFAULT_MAX_FEE)
     })
 
     it('relays nonce cancellation messages', async () => {

From 1eddd94dd105dd50e8dc3cf887f20e07a0c79edb Mon Sep 17 00:00:00 2001
From: Ed Noepel <ed@noepel.net>
Date: Tue, 24 Sep 2024 13:30:50 -0400
Subject: [PATCH 08/14] revert change to function type

---
 packages/perennial-account/contracts/libs/RebalanceLib.sol | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/packages/perennial-account/contracts/libs/RebalanceLib.sol b/packages/perennial-account/contracts/libs/RebalanceLib.sol
index 75d673888..40c500312 100644
--- a/packages/perennial-account/contracts/libs/RebalanceLib.sol
+++ b/packages/perennial-account/contracts/libs/RebalanceLib.sol
@@ -21,7 +21,7 @@ library RebalanceLib {
         UFixed6 maxFee,
         Fixed6 groupCollateral,
         Fixed6 marketCollateral
-    ) internal view returns (bool canRebalance, Fixed6 imbalance) {
+    ) internal pure returns (bool canRebalance, Fixed6 imbalance) {
         // determine how much collateral the market should have
         Fixed6 targetCollateral = groupCollateral.mul(Fixed6Lib.from(marketConfig.target));
 

From 24c72e4180d64ac6787e2d5c266c40ec4c3a78e3 Mon Sep 17 00:00:00 2001
From: Ed Noepel <ed@noepel.net>
Date: Tue, 24 Sep 2024 15:55:35 -0400
Subject: [PATCH 09/14] created test demonstrating the issue

---
 .../test/integration/Manager_Arbitrum.test.ts | 47 +++++++++++++++++--
 1 file changed, 44 insertions(+), 3 deletions(-)

diff --git a/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts b/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts
index 27e3a86d4..0f5dc07b8 100644
--- a/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts
+++ b/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts
@@ -675,10 +675,51 @@ describe('Manager_Arbitrum', () => {
       checkKeeperCompensation = true
     })
 
+    it('prevents user from changing maxFee upon execution', async () => {
+      // user places order
+      const orderId = await placeOrderWithSignature(
+        userA,
+        Side.MAKER,
+        Compare.GTE,
+        parse6decimal('2802'),
+        parse6decimal('3'),
+      )
+      expect(orderId).to.equal(BigNumber.from(509))
+      keeperBalanceBefore = await dsu.balanceOf(keeper.address)
+
+      // order becomes executable 5 minutes later
+      await increase(60 * 5)
+      advanceBlock()
+      await setNextBlockBaseFee()
+      await commitPrice(parse6decimal('2802.2'), currentTime.add(60 * 5))
+
+      // user zeros the maxFee just before keeper executes
+      const order = {
+        side: Side.MAKER,
+        comparison: Compare.GTE,
+        price: parse6decimal('2802'),
+        delta: parse6decimal('3'),
+        maxFee: parse6decimal('0'),
+        isSpent: false,
+        referrer: constants.AddressZero,
+        ...NO_INTERFACE_FEE,
+      }
+      await expect(manager.connect(userA).placeOrder(market.address, orderId, order, TX_OVERRIDES))
+        .to.emit(manager, 'TriggerOrderPlaced')
+        .withArgs(market.address, userA.address, order, orderId)
+
+      // keeper executes
+      await executeOrder(userA, 509)
+      expect(await getPendingPosition(userA, Side.MAKER)).to.equal(parse6decimal('88'))
+      expect(await getPendingPosition(userB, Side.LONG)).to.equal(parse6decimal('7'))
+
+      checkKeeperCompensation = true
+    })
+
     it('users can close positions', async () => {
       // can close directly
       let orderId = await placeOrder(userA, Side.MAKER, Compare.GTE, constants.Zero, MAGIC_VALUE_CLOSE_POSITION)
-      expect(orderId).to.equal(BigNumber.from(509))
+      expect(orderId).to.equal(BigNumber.from(510))
 
       // can close using a signed message
       orderId = await placeOrderWithSignature(
@@ -693,7 +734,7 @@ describe('Manager_Arbitrum', () => {
       // keeper closes the taker position before removing liquidity
       await executeOrder(userB, 504)
       await commitPrice()
-      await executeOrder(userA, 509)
+      await executeOrder(userA, 510)
       await commitPrice()
 
       // settle and confirm positions are closed
@@ -886,7 +927,7 @@ describe('Manager_Arbitrum', () => {
       checkKeeperCompensation = true
     })
 
-    it('charges notional interface on whole position when closing', async () => {
+    it('charges notional interface fee on whole position when closing', async () => {
       const interfaceBalanceBefore = await dsu.balanceOf(userB.address)
 
       // userD closes their long position

From 7faab93e5638913e31d0222e9b070ceb27a05592 Mon Sep 17 00:00:00 2001
From: Prateek <prateek@perennial.finance>
Date: Thu, 26 Sep 2024 20:39:35 +0530
Subject: [PATCH 10/14] fix nit

---
 .../perennial-account/contracts/Controller_Incentivized.sol | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/packages/perennial-account/contracts/Controller_Incentivized.sol b/packages/perennial-account/contracts/Controller_Incentivized.sol
index dfaeeb380..15d6865fb 100644
--- a/packages/perennial-account/contracts/Controller_Incentivized.sol
+++ b/packages/perennial-account/contracts/Controller_Incentivized.sol
@@ -32,11 +32,11 @@ import { Withdrawal } from "./types/Withdrawal.sol";
 /// @notice Controller which compensates keepers for handling or relaying messages. Subclass to handle differences in
 /// gas calculations on different chains.
 abstract contract Controller_Incentivized is Controller, IRelayer, Kept {
-    /// @dev Configuration used to calculate keeper compensation
-    KeepConfig public keepConfig;
-
     /// @dev Handles relayed messages for nonce cancellation
     IVerifierBase public immutable nonceManager;
+    
+    /// @dev Configuration used to calculate keeper compensation
+    KeepConfig public keepConfig;
 
     /// @dev Creates instance of Controller which compensates keepers
     /// @param implementation_ Pristine collateral account contract

From 8b78dd33a7ceb1e4d572d242980506d313fd1567 Mon Sep 17 00:00:00 2001
From: Ed Noepel <ed@noepel.net>
Date: Thu, 26 Sep 2024 12:23:39 -0400
Subject: [PATCH 11/14] prevent user from reducing maxFee

---
 .../perennial-order/contracts/Manager.sol     |  2 +
 .../contracts/interfaces/IManager.sol         |  4 ++
 .../test/integration/Manager_Arbitrum.test.ts | 41 -------------------
 .../perennial-order/test/unit/Manager.test.ts | 18 ++++++++
 4 files changed, 24 insertions(+), 41 deletions(-)

diff --git a/packages/perennial-order/contracts/Manager.sol b/packages/perennial-order/contracts/Manager.sol
index 71f8b71f7..f0abe1ed0 100644
--- a/packages/perennial-order/contracts/Manager.sol
+++ b/packages/perennial-order/contracts/Manager.sol
@@ -208,6 +208,8 @@ abstract contract Manager is IManager, Kept {
         // prevent user from reusing an order identifier
         TriggerOrder memory old = _orders[market][account][orderId].read();
         if (old.isSpent) revert ManagerInvalidOrderNonceError();
+        // prevent user from frontrunning keeper compensation
+        if (!old.isEmpty() && old.maxFee.gt(order.maxFee)) revert ManagerCannotReduceMaxFee();
 
         _orders[market][account][orderId].store(order);
         emit TriggerOrderPlaced(market, account, order, orderId);
diff --git a/packages/perennial-order/contracts/interfaces/IManager.sol b/packages/perennial-order/contracts/interfaces/IManager.sol
index 20a02755b..85e68a112 100644
--- a/packages/perennial-order/contracts/interfaces/IManager.sol
+++ b/packages/perennial-order/contracts/interfaces/IManager.sol
@@ -48,6 +48,10 @@ interface IManager {
     /// @custom:error Conditions required for order execution are not currently met
     error ManagerCannotExecuteError();
 
+    // sig: 0x170dda16
+    /// @custom:error Replacement order may not reduce maxFee; must cancel and resubmit with new orderId
+    error ManagerCannotReduceMaxFee();
+
     // sig: 0xd0cfc108
     /// @custom:error Order nonce has already been used
     error ManagerInvalidOrderNonceError();
diff --git a/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts b/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts
index 0f5dc07b8..060608794 100644
--- a/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts
+++ b/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts
@@ -675,47 +675,6 @@ describe('Manager_Arbitrum', () => {
       checkKeeperCompensation = true
     })
 
-    it('prevents user from changing maxFee upon execution', async () => {
-      // user places order
-      const orderId = await placeOrderWithSignature(
-        userA,
-        Side.MAKER,
-        Compare.GTE,
-        parse6decimal('2802'),
-        parse6decimal('3'),
-      )
-      expect(orderId).to.equal(BigNumber.from(509))
-      keeperBalanceBefore = await dsu.balanceOf(keeper.address)
-
-      // order becomes executable 5 minutes later
-      await increase(60 * 5)
-      advanceBlock()
-      await setNextBlockBaseFee()
-      await commitPrice(parse6decimal('2802.2'), currentTime.add(60 * 5))
-
-      // user zeros the maxFee just before keeper executes
-      const order = {
-        side: Side.MAKER,
-        comparison: Compare.GTE,
-        price: parse6decimal('2802'),
-        delta: parse6decimal('3'),
-        maxFee: parse6decimal('0'),
-        isSpent: false,
-        referrer: constants.AddressZero,
-        ...NO_INTERFACE_FEE,
-      }
-      await expect(manager.connect(userA).placeOrder(market.address, orderId, order, TX_OVERRIDES))
-        .to.emit(manager, 'TriggerOrderPlaced')
-        .withArgs(market.address, userA.address, order, orderId)
-
-      // keeper executes
-      await executeOrder(userA, 509)
-      expect(await getPendingPosition(userA, Side.MAKER)).to.equal(parse6decimal('88'))
-      expect(await getPendingPosition(userB, Side.LONG)).to.equal(parse6decimal('7'))
-
-      checkKeeperCompensation = true
-    })
-
     it('users can close positions', async () => {
       // can close directly
       let orderId = await placeOrder(userA, Side.MAKER, Compare.GTE, constants.Zero, MAGIC_VALUE_CLOSE_POSITION)
diff --git a/packages/perennial-order/test/unit/Manager.test.ts b/packages/perennial-order/test/unit/Manager.test.ts
index 6376d3414..587b894e9 100644
--- a/packages/perennial-order/test/unit/Manager.test.ts
+++ b/packages/perennial-order/test/unit/Manager.test.ts
@@ -167,6 +167,24 @@ describe('Manager', () => {
       compareOrders(order, replacement)
     })
 
+    it('prevents user from reducing maxFee', async () => {
+      // submit the original order
+      await manager.connect(userA).placeOrder(market.address, nextOrderId, MAKER_ORDER)
+
+      // user cannot reduce maxFee
+      const replacement = { ...MAKER_ORDER }
+      replacement.maxFee = MAKER_ORDER.maxFee.sub(1)
+      await expect(
+        manager.connect(userA).placeOrder(market.address, nextOrderId, replacement),
+      ).to.be.revertedWithCustomError(manager, 'ManagerCannotReduceMaxFee')
+
+      // user cannot zero maxFee
+      replacement.maxFee = constants.Zero
+      await expect(
+        manager.connect(userA).placeOrder(market.address, nextOrderId, replacement),
+      ).to.be.revertedWithCustomError(manager, 'ManagerCannotReduceMaxFee')
+    })
+
     it('keeper can execute orders', async () => {
       // place a maker and long order
       const nonce1 = advanceOrderId()

From 525ae49c909b40ccd783f4a40d8e992c6eed59de Mon Sep 17 00:00:00 2001
From: Ed Noepel <ed@noepel.net>
Date: Thu, 26 Sep 2024 12:40:16 -0400
Subject: [PATCH 12/14] fix integration tests

---
 .../perennial-order/test/integration/Manager_Arbitrum.test.ts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts b/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts
index 060608794..988b8eace 100644
--- a/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts
+++ b/packages/perennial-order/test/integration/Manager_Arbitrum.test.ts
@@ -678,7 +678,7 @@ describe('Manager_Arbitrum', () => {
     it('users can close positions', async () => {
       // can close directly
       let orderId = await placeOrder(userA, Side.MAKER, Compare.GTE, constants.Zero, MAGIC_VALUE_CLOSE_POSITION)
-      expect(orderId).to.equal(BigNumber.from(510))
+      expect(orderId).to.equal(BigNumber.from(509))
 
       // can close using a signed message
       orderId = await placeOrderWithSignature(
@@ -693,7 +693,7 @@ describe('Manager_Arbitrum', () => {
       // keeper closes the taker position before removing liquidity
       await executeOrder(userB, 504)
       await commitPrice()
-      await executeOrder(userA, 510)
+      await executeOrder(userA, 509)
       await commitPrice()
 
       // settle and confirm positions are closed

From 99cb41c9bcabe783ab2e8be282aa9fbef7d7cf38 Mon Sep 17 00:00:00 2001
From: Ed Noepel <ed@noepel.net>
Date: Fri, 27 Sep 2024 09:43:10 -0400
Subject: [PATCH 13/14] ensure deleted groups cannot be rebalanced

---
 packages/perennial-account/contracts/Controller.sol | 2 ++
 packages/perennial-account/test/unit/Controller.ts  | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/packages/perennial-account/contracts/Controller.sol b/packages/perennial-account/contracts/Controller.sol
index b3ac19cb2..7be356003 100644
--- a/packages/perennial-account/contracts/Controller.sol
+++ b/packages/perennial-account/contracts/Controller.sol
@@ -113,6 +113,8 @@ contract Controller is Factory, IController {
             imbalances[i] = imbalance;
             canRebalance = canRebalance || canMarketRebalance;
         }
+
+        // if group does not exist or was deleted, arrays will be empty and function will return (0, false, 0)
     }
 
     /// @inheritdoc IController
diff --git a/packages/perennial-account/test/unit/Controller.ts b/packages/perennial-account/test/unit/Controller.ts
index e599dc6c7..eabd9a7b5 100644
--- a/packages/perennial-account/test/unit/Controller.ts
+++ b/packages/perennial-account/test/unit/Controller.ts
@@ -674,6 +674,12 @@ describe('Controller', () => {
           .withArgs(userA.address, group, 0)
 
         await verifyConfigAgainstMessage(userA, message, group)
+
+        // cannot rebalance a deleted group
+        await expect(controller.connect(keeper).rebalanceGroup(userA.address, group)).to.be.revertedWithCustomError(
+          controller,
+          'ControllerGroupBalancedError',
+        )
       })
     })
   })

From c5ef1cc4f920620d245363e1b784b21b396d27d5 Mon Sep 17 00:00:00 2001
From: Ed Noepel <ed@noepel.net>
Date: Tue, 1 Oct 2024 10:18:13 -0400
Subject: [PATCH 14/14] adjusted storage layout

---
 .../contracts/types/MarketParameter.sol         | 17 ++++++++---------
 .../test/integration/core/happyPath.test.ts     |  8 --------
 .../test/integration/helpers/setupHelpers.ts    |  2 --
 3 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/packages/perennial/contracts/types/MarketParameter.sol b/packages/perennial/contracts/types/MarketParameter.sol
index 54bb6ad63..e78e2ba1d 100644
--- a/packages/perennial/contracts/types/MarketParameter.sol
+++ b/packages/perennial/contracts/types/MarketParameter.sol
@@ -46,7 +46,6 @@ using MarketParameterStorageLib for MarketParameterStorage global;
 ///        uint24 interestFee;         // <= 1677%
 ///        uint24 makerFee;            // <= 1677%
 ///        uint24 takerFee;            // <= 1677%
-///        uint24 __unallocated__;     // <= 1677%
 ///        uint24 riskFee;             // <= 1677%
 ///        uint16 maxPendingGlobal;    // <= 65k
 ///        uint16 maxPendingLocal;     // <= 65k
@@ -61,7 +60,7 @@ library MarketParameterStorageLib {
     function read(MarketParameterStorage storage self) internal view returns (MarketParameter memory) {
         uint256 slot0 = self.slot0;
 
-        uint256 flags = uint256(slot0 << (256 - 24 - 24 - 24 - 24 - 24 - 24 - 16 - 16 - 48 - 8)) >> (256 - 8);
+        uint256 flags = uint256(slot0 << (256 - 24 - 24 - 24 - 24 - 24 - 16 - 16 - 48 - 8)) >> (256 - 8);
         (bool closed, bool settle) =
             (flags & 0x04 == 0x04, flags & 0x08 == 0x08);
 
@@ -70,9 +69,9 @@ library MarketParameterStorageLib {
             UFixed6.wrap(uint256(slot0 << (256 - 24 - 24)) >> (256 - 24)),
             UFixed6.wrap(uint256(slot0 << (256 - 24 - 24 - 24)) >> (256 - 24)),
             UFixed6.wrap(uint256(slot0 << (256 - 24 - 24 - 24 - 24)) >> (256 - 24)),
-            UFixed6.wrap(uint256(slot0 << (256 - 24 - 24 - 24 - 24 - 24 - 24)) >> (256 - 24)),
-            uint256(slot0 << (256 - 24 - 24 - 24 - 24 - 24 - 24 - 16)) >> (256 - 16),
-            uint256(slot0 << (256 - 24 - 24 - 24 - 24 - 24 - 24 - 16 - 16)) >> (256 - 16),
+            UFixed6.wrap(uint256(slot0 << (256 - 24 - 24 - 24 - 24 - 24)) >> (256 - 24)),
+            uint256(slot0 << (256 - 24 - 24 - 24 - 24 - 24 - 16)) >> (256 - 16),
+            uint256(slot0 << (256 - 24 - 24 - 24 - 24 - 24 - 16 - 16)) >> (256 - 16),
             closed,
             settle
         );
@@ -108,10 +107,10 @@ library MarketParameterStorageLib {
             uint256(UFixed6.unwrap(newValue.interestFee) << (256 - 24)) >> (256 - 24 - 24) |
             uint256(UFixed6.unwrap(newValue.makerFee) << (256 - 24)) >> (256 - 24 - 24 - 24) |
             uint256(UFixed6.unwrap(newValue.takerFee) << (256 - 24)) >> (256 - 24 - 24 - 24 - 24) |
-            uint256(UFixed6.unwrap(newValue.riskFee) << (256 - 24)) >> (256 - 24 - 24 - 24 - 24 - 24 - 24) |
-            uint256(newValue.maxPendingGlobal << (256 - 16)) >> (256 - 24 - 24 - 24 - 24 - 24 - 24 - 16) |
-            uint256(newValue.maxPendingLocal << (256 - 16)) >> (256 - 24 - 24 - 24 - 24 - 24 - 24 - 16 - 16) |
-            uint256(flags << (256 - 8)) >> (256 - 24 - 24 - 24 - 24 - 24 - 24 - 16 - 16 - 48 - 8);
+            uint256(UFixed6.unwrap(newValue.riskFee) << (256 - 24)) >> (256 - 24 - 24 - 24 - 24 - 24) |
+            uint256(newValue.maxPendingGlobal << (256 - 16)) >> (256 - 24 - 24 - 24 - 24 - 24 - 16) |
+            uint256(newValue.maxPendingLocal << (256 - 16)) >> (256 - 24 - 24 - 24 - 24 - 24 - 16 - 16) |
+            uint256(flags << (256 - 8)) >> (256 - 24 - 24 - 24 - 24 - 24 - 16 - 16 - 48 - 8);
 
         assembly {
             sstore(self.slot, encoded0)
diff --git a/packages/perennial/test/integration/core/happyPath.test.ts b/packages/perennial/test/integration/core/happyPath.test.ts
index 133ba6ff1..31fbe090c 100644
--- a/packages/perennial/test/integration/core/happyPath.test.ts
+++ b/packages/perennial/test/integration/core/happyPath.test.ts
@@ -110,13 +110,11 @@ describe('Happy Path', () => {
     const parameter = {
       fundingFee: parse6decimal('0.1'),
       interestFee: parse6decimal('0.1'),
-      oracleFee: 0,
       riskFee: 0,
       makerFee: 0,
       takerFee: 0,
       maxPendingGlobal: 8,
       maxPendingLocal: 8,
-      settlementFee: 0,
       closed: false,
       settle: false,
     }
@@ -1287,9 +1285,7 @@ describe('Happy Path', () => {
     const parameter = {
       fundingFee: parse6decimal('0.1'),
       interestFee: parse6decimal('0.1'),
-      oracleFee: 0,
       riskFee: 0,
-      settlementFee: 0,
       maxPendingGlobal: 8,
       maxPendingLocal: 8,
       makerFee: positionFeesOn ? parse6decimal('0.2') : 0,
@@ -1452,9 +1448,7 @@ describe('Happy Path', () => {
     const parameter = {
       fundingFee: parse6decimal('0.1'),
       interestFee: parse6decimal('0.1'),
-      oracleFee: 0,
       riskFee: 0,
-      settlementFee: 0,
       maxPendingGlobal: 8,
       maxPendingLocal: 8,
       makerFee: parse6decimal('0.2'),
@@ -2530,9 +2524,7 @@ describe('Happy Path', () => {
     const parameter = {
       fundingFee: parse6decimal('0.1'),
       interestFee: parse6decimal('0.1'),
-      oracleFee: 0,
       riskFee: 0,
-      settlementFee: 0,
       maxPendingGlobal: 8,
       maxPendingLocal: 8,
       makerFee: positionFeesOn ? parse6decimal('0.2') : 0,
diff --git a/packages/perennial/test/integration/helpers/setupHelpers.ts b/packages/perennial/test/integration/helpers/setupHelpers.ts
index e800410c5..4cc673b71 100644
--- a/packages/perennial/test/integration/helpers/setupHelpers.ts
+++ b/packages/perennial/test/integration/helpers/setupHelpers.ts
@@ -261,13 +261,11 @@ export async function createMarket(
   const marketParameter = {
     fundingFee: parse6decimal('0.1'),
     interestFee: parse6decimal('0.1'),
-    oracleFee: 0,
     riskFee: 0,
     makerFee: 0,
     takerFee: 0,
     maxPendingGlobal: 8,
     maxPendingLocal: 8,
-    settlementFee: 0,
     closed: false,
     settle: false,
     ...marketParamOverrides,