v2.3 Audit Fixes

.github/workflows/CI.yml

@@ -6,10 +6,8 @@ on:
   pull_request:
     branches:
       - main
-      - v2.1
-      - v2.1.1
-      - v2.2
       - v2.3
+      - v2.3-fix-review
   workflow_dispatch:
 
 env:
@@ -513,12 +511,13 @@ jobs:
         with:
           name: verifier_unit_test_coverage
           path: ./packages/perennial-verifier/coverage/lcov.info
-  
+
   # [PERIPHERY UNIT COVERAGE]
   periphery-unit-test-coverage:
     name: 'Periphery Unit Tests Coverage'
     runs-on: ubuntu-latest
-    needs: [account-unit-test, oracle-unit-test, order-unit-test, vault-unit-test, extensions-unit-test, verifier-unit-test]
+    needs:
+      [account-unit-test, oracle-unit-test, order-unit-test, vault-unit-test, extensions-unit-test, verifier-unit-test]
     steps:
       - uses: actions/setup-node@v3
         with:
@@ -558,10 +557,10 @@ jobs:
         run: sudo apt-get update && sudo apt-get install lcov
       - name: Combine unit test coverage
         run: lcov -a account_unit_test_coverage/lcov.info
-          -a oracle_unit_test_coverage/lcov.info 
-          -a order_unit_test_coverage/lcov.info 
-          -a vault_unit_test_coverage/lcov.info 
-          -a extensions_unit_test_coverage/lcov.info 
+          -a oracle_unit_test_coverage/lcov.info
+          -a order_unit_test_coverage/lcov.info
+          -a vault_unit_test_coverage/lcov.info
+          -a extensions_unit_test_coverage/lcov.info
           -a verifier_unit_test_coverage/lcov.info
           --output-file periphery_unit_coverage.info
       - name: Upload combined unit test lcov file
@@ -629,12 +628,12 @@ jobs:
       - name: Install lcov
         run: sudo apt-get update && sudo apt-get install lcov
       - name: Combine integration test coverage
-        run: lcov -a account_integration_test_coverage/lcov.info 
-          -a oracle_integration_test_coverage/lcov.info 
-          -a oracle_sepolia_integration_test_coverage/lcov.info 
-          -a order_integration_test_coverage/lcov.info 
-          -a vault_integration_test_coverage/lcov.info 
-          -a extensions_integration_test_coverage/lcov.info 
+        run: lcov -a account_integration_test_coverage/lcov.info
+          -a oracle_integration_test_coverage/lcov.info
+          -a oracle_sepolia_integration_test_coverage/lcov.info
+          -a order_integration_test_coverage/lcov.info
+          -a vault_integration_test_coverage/lcov.info
+          -a extensions_integration_test_coverage/lcov.info
           --output-file periphery_integration_coverage.info
       - name: Upload combined integration test lcov file
         uses: actions/upload-artifact@v3

package.json

@@ -14,7 +14,7 @@
   "devDependencies": {
     "@codechecks/client": "^0.1.10",
     "@defi-wonderland/smock": "^2.0.7",
-    "@equilibria/root": "2.3.0-rc7",
+    "@equilibria/root": "2.3.0-rc8",
     "@nomicfoundation/hardhat-chai-matchers": "^1.0.6",
     "@nomicfoundation/hardhat-network-helpers": "^1.0.8",
     "@nomicfoundation/hardhat-toolbox": "2.0.2",

packages/common/testutil/types.ts

@@ -105,6 +105,7 @@ export interface MarketParameter {
   riskFee: BigNumberish
   maxPendingGlobal: number
   maxPendingLocal: number
+  maxPriceDeviation: BigNumberish
   closed: boolean
   settle: boolean
 }
@@ -356,6 +357,7 @@ export const DEFAULT_MARKET_PARAMETER: MarketParameter = {
   riskFee: 0,
   maxPendingGlobal: 0,
   maxPendingLocal: 0,
+  maxPriceDeviation: 0,
   closed: false,
   settle: false,
 }

packages/perennial-account/contracts/AccountVerifier.sol

@@ -4,6 +4,7 @@ pragma solidity 0.8.24;
 import { EIP712 } from "@openzeppelin/contracts/utils/cryptography/EIP712.sol";
 import { SignatureChecker } from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol";
 import { VerifierBase } from "@equilibria/root/verifier/VerifierBase.sol";
+import { IMarketFactory } from "@equilibria/perennial-v2/contracts/interfaces/IMarketFactory.sol";
 
 import { IAccountVerifier } from "./interfaces/IAccountVerifier.sol";
 import { IRelayVerifier } from "./interfaces/IRelayVerifier.sol"; // only needed for docstrings
@@ -21,8 +22,13 @@ import { RelayedAccessUpdateBatch, RelayedAccessUpdateBatchLib } from "./types/R
 /// @title Verifier
 /// @notice ERC712 signed message verifier for the Perennial V2 Collateral Accounts package.
 contract AccountVerifier is VerifierBase, IAccountVerifier {
+    /// @dev market factory to check authorization
+    IMarketFactory internal immutable marketFactory;
+
     /// @dev Initializes the domain separator and parameter caches
-    constructor() EIP712("Perennial V2 Collateral Accounts", "1.0.0") { }
+    constructor(IMarketFactory _marketFactory) EIP712("Perennial V2 Collateral Accounts", "1.0.0") {
+        marketFactory = _marketFactory;
+    }
 
     /// @inheritdoc IAccountVerifier
     function verifyAction(Action calldata action, bytes calldata signature)
@@ -148,4 +154,12 @@ contract AccountVerifier is VerifierBase, IAccountVerifier {
             outerSignature
         )) revert VerifierInvalidSignerError();
     }
+
+    /// @notice Checks whether signer is allowed to sign a message for account
+    /// @param account user to check authorization for (not the collateral account)
+    /// @param signer address which signed a message for the account
+    /// @return true if signer is authorized, otherwise false
+    function _authorized(address account, address signer) internal view override returns (bool) {
+        return super._authorized(account, signer) || marketFactory.signers(account, signer);
+    }
 }

packages/perennial-account/contracts/Controller.sol

@@ -31,13 +31,13 @@ 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;
+    IMarketFactory public immutable marketFactory;
 
     /// @inheritdoc IController
     IAccountVerifier public verifier;
@@ -59,18 +59,17 @@ contract Controller is Factory, IController {
 
     /// @dev Creates instance of Controller
     /// @param implementation_ Collateral account contract initialized with stablecoin addresses
-    constructor(address implementation_) Factory(implementation_) {
+    constructor(address implementation_, IMarketFactory marketFactory_) Factory(implementation_) {
         USDC = Account(implementation_).USDC();
         DSU = Account(implementation_).DSU();
+        marketFactory = marketFactory_;
     }
 
     /// @inheritdoc IController
     function initialize(
-        IMarketFactory marketFactory_,
         IAccountVerifier verifier_
     ) external initializer(1) {
         __Factory__initialize();
-        marketFactory = marketFactory_;
         verifier = verifier_;
     }
 
@@ -104,10 +103,17 @@ 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;
         }
+
+        // if group does not exist or was deleted, arrays will be empty and function will return (0, false, 0)
     }
 
     /// @inheritdoc IController
@@ -163,8 +169,6 @@ contract Controller is Factory, IController {
     function _changeRebalanceConfigWithSignature(RebalanceConfigChange calldata configChange, bytes calldata signature) internal {
         // ensure the message was signed by the owner or a delegated signer
         verifier.verifyRebalanceConfigChange(configChange, signature);
-        _ensureValidSigner(configChange.action.common.account, configChange.action.common.signer);
-
         // sum of the target allocations of all markets in the group
         _updateRebalanceGroup(configChange, configChange.action.common.account);
     }
@@ -174,18 +178,12 @@ contract Controller is Factory, IController {
         emit AccountDeployed(owner, account);
     }
 
-    /// @dev reverts if user is not authorized to sign transactions for the owner
-    function _ensureValidSigner(address owner, address signer) internal view {
-        if (signer != owner && !marketFactory.signers(owner, signer)) revert ControllerInvalidSignerError();
-    }
-
     function _deployAccountWithSignature(
         DeployAccount calldata deployAccount_,
         bytes calldata signature
     ) internal returns (IAccount account) {
         address owner = deployAccount_.action.common.account;
         verifier.verifyDeployAccount(deployAccount_, signature);
-        _ensureValidSigner(owner, deployAccount_.action.common.signer);
 
         // create the account
         account = _createAccount(owner);
@@ -198,7 +196,6 @@ contract Controller is Factory, IController {
     ) internal {
         // ensure the message was signed by the owner or a delegated signer
         verifier.verifyMarketTransfer(marketTransfer, signature);
-        _ensureValidSigner(marketTransfer.action.common.account, marketTransfer.action.common.signer);
 
         // only Markets with DSU collateral are supported
         IMarket market = IMarket(marketTransfer.market);
@@ -214,7 +211,6 @@ contract Controller is Factory, IController {
     ) internal {
         // ensure the message was signed by the owner or a delegated signer
         verifier.verifyWithdrawal(withdrawal, signature);
-        _ensureValidSigner(withdrawal.action.common.account, withdrawal.action.common.signer);
 
         // call the account's implementation to push to owner
         account.withdraw(withdrawal.amount, withdrawal.unwrap);

packages/perennial-account/contracts/Controller_Arbitrum.sol

@@ -4,16 +4,21 @@ pragma solidity 0.8.24;
 import { Kept_Arbitrum, Kept } from "@equilibria/root/attribute/Kept/Kept_Arbitrum.sol";
 import { UFixed18 } from "@equilibria/root/number/types/UFixed18.sol";
 import { IVerifierBase } from "@equilibria/root/verifier/interfaces/IVerifierBase.sol";
+import { IMarketFactory } from "@equilibria/perennial-v2/contracts/interfaces/IMarketFactory.sol";
 import { Controller_Incentivized } from "./Controller_Incentivized.sol";
 
 /// @title Controller_Arbitrum
 /// @notice Controller which compensates keepers for handling or relaying messages on Arbitrum L2.
 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) {}
+    /// @param marketFactory Market Factory contract
+    /// @param nonceManager Verifier contract to which nonce and group cancellations are relayed
+    constructor(
+        address implementation,
+        IMarketFactory marketFactory,
+        IVerifierBase nonceManager
+    ) Controller_Incentivized(implementation, marketFactory, nonceManager) {}
 
     /// @dev Use the Kept_Arbitrum implementation for calculating the dynamic fee
     function _calldataFee(