Audit / other fixes (#2168)

* wip * audit and other fixes * more fixes * fix tests * fix storage * fix and add more tests * add test for margin only liquidation * storage dump fix * storage dump
Synthetixio · Jun 26, 2024 · 2a2882c · 2a2882c
1 parent 32d2eb1
commit 2a2882c
Show file tree

Hide file tree

Showing 27 changed files with 688 additions and 534 deletions.
diff --git a/markets/perps-market/cannonfile.test.toml b/markets/perps-market/cannonfile.test.toml
@@ -132,13 +132,6 @@ func = "setFeatureFlagAllowAll"
 from = "<%= settings.owner %>"
 args = ["<%= formatBytes32String('perpsSystem') %>", true]
 
-# add snxUSD as the only priority to deduct from on a given account, as default
-[invoke.setSynthDeductionPriority]
-target = ["PerpsMarketProxy"]
-func = "setSynthDeductionPriority"
-from = "<%= settings.owner %>"
-args = [[0]]
-
 [contract.MockPythERC7412Wrapper]
 artifact = "contracts/mocks/MockPythERC7412Wrapper.sol:MockPythERC7412Wrapper"
 

diff --git a/markets/perps-market/contracts/interfaces/IAccountEvents.sol b/markets/perps-market/contracts/interfaces/IAccountEvents.sol
@@ -6,10 +6,10 @@ pragma solidity >=0.8.11 <0.9.0;
  */
 interface IAccountEvents {
     /**
-     * @notice Gets fired when some collateral is deducted from the account for paying fees or liquidations.
-     * @param account Id of the account being deducted.
-     * @param collateralId Id of the collateral (synth) deducted.
+     * @notice Gets fired anytime an account is charged with fees, paying settlement rewards.
+     * @param accountId Id of the account being deducted.
      * @param amount Amount of synth market deducted from the account.
+     * @param accountDebt current debt of the account after charged amount.
      */
-    event CollateralDeducted(uint256 account, uint128 collateralId, uint256 amount);
+    event AccountCharged(uint128 accountId, int256 amount, uint256 accountDebt);
 }
diff --git a/markets/perps-market/contracts/interfaces/IAsyncOrderSettlementPythModule.sol b/markets/perps-market/contracts/interfaces/IAsyncOrderSettlementPythModule.sol
@@ -52,8 +52,6 @@ interface IAsyncOrderSettlementPythModule {
         int256 pnl;
         uint256 chargedInterest;
         int256 accruedFunding;
-        uint256 pnlUint;
-        uint256 amountToDeduct;
         uint256 settlementReward;
         uint256 fillPrice;
         uint256 totalFees;
@@ -64,6 +62,8 @@ interface IAsyncOrderSettlementPythModule {
         uint256 synthDeductionIterator;
         uint128[] deductedSynthIds;
         uint256[] deductedAmount;
+        int256 chargedAmount;
+        uint256 newAccountDebt;
     }
 
     /**

diff --git a/markets/perps-market/contracts/interfaces/IGlobalPerpsMarketModule.sol b/markets/perps-market/contracts/interfaces/IGlobalPerpsMarketModule.sol
@@ -12,12 +12,6 @@ interface IGlobalPerpsMarketModule {
      */
     event InterestRateUpdated(uint128 indexed superMarketId, uint128 interestRate);
 
-    /**
-     * @notice Gets fired when the synth deduction priority is updated by owner.
-     * @param newSynthDeductionPriority new synth id priority order for deductions.
-     */
-    event SynthDeductionPrioritySet(uint128[] newSynthDeductionPriority);
-
     /**
      * @notice Gets fired when keeper reward guard is set or updated.
      * @param minKeeperRewardUsd Minimum keeper reward expressed as USD value.
@@ -97,20 +91,6 @@ interface IGlobalPerpsMarketModule {
         view
         returns (uint256[] memory supportedCollaterals);
 
-    /**
-     * @notice Sets the synth deduction priority ordered list.
-     * @dev The synth deduction priority is used to determine the order in which synths are deducted from an account. Id 0 is snxUSD and should be first in the list.
-     * @param newSynthDeductionPriority Ordered array of synth market ids for deduction priority.
-     */
-    function setSynthDeductionPriority(uint128[] memory newSynthDeductionPriority) external;
-
-    /**
-     * @notice Gets the synth deduction priority ordered list.
-     * @dev The synth deduction priority is used to determine the order in which synths are deducted from an account. Id 0 is snxUSD and should be first in the list.
-     * @return synthDeductionPriority Ordered array of synth market ids for deduction priority.
-     */
-    function getSynthDeductionPriority() external view returns (uint128[] memory);
-
     /**
      * @notice Sets the keeper reward guard (min and max).
      * @param minKeeperRewardUsd Minimum keeper reward expressed as USD value.

diff --git a/markets/perps-market/contracts/interfaces/ILiquidationModule.sol b/markets/perps-market/contracts/interfaces/ILiquidationModule.sol
@@ -63,6 +63,18 @@ interface ILiquidationModule {
         bool fullLiquidation
     );
 
+    /**
+     * @notice Gets fired when an account margin is liquidated due to not paying down debt.
+     * @param accountId Id of the account liquidated.
+     * @param seizedMarginValue margin seized due to liquidation.
+     * @param liquidationReward reward for liquidating margin account
+     */
+    event AccountMarginLiquidation(
+        uint128 indexed accountId,
+        uint256 seizedMarginValue,
+        uint256 liquidationReward
+    );
+
     /**
      * @notice Liquidates an account.
      * @dev according to the current situation and account size it can be a partial or full liquidation.

diff --git a/markets/perps-market/contracts/modules/AsyncOrderCancelModule.sol b/markets/perps-market/contracts/modules/AsyncOrderCancelModule.sol
@@ -69,19 +69,13 @@ contract AsyncOrderCancelModule is IAsyncOrderCancelModule, IMarketEvents, IAcco
         runtime.fillPrice = asyncOrder.validateCancellation(settlementStrategy, price);
 
         if (runtime.settlementReward > 0) {
-            // deduct keeper reward
-            (uint128[] memory deductedSynthIds, uint256[] memory deductedAmount) = PerpsAccount
-                .load(runtime.accountId)
-                .deductFromAccount(runtime.settlementReward);
-            for (uint256 i = 0; i < deductedSynthIds.length; i++) {
-                if (deductedAmount[i] > 0) {
-                    emit CollateralDeducted(
-                        runtime.accountId,
-                        deductedSynthIds[i],
-                        deductedAmount[i]
-                    );
-                }
-            }
+            // charge account the settlement reward
+            uint256 accountDebt = PerpsAccount.load(runtime.accountId).charge(
+                -runtime.settlementReward.toInt()
+            );
+
+            emit AccountCharged(runtime.accountId, runtime.settlementReward.toInt(), accountDebt);
+
             // pay keeper
             PerpsMarketFactory.load().withdrawMarketUsd(
                 ERC2771Context._msgSender(),

diff --git a/markets/perps-market/contracts/modules/AsyncOrderSettlementPythModule.sol b/markets/perps-market/contracts/modules/AsyncOrderSettlementPythModule.sol
@@ -77,8 +77,6 @@ contract AsyncOrderSettlementPythModule is
         Position.Data storage oldPosition;
         (runtime.newPosition, runtime.totalFees, runtime.fillPrice, oldPosition) = asyncOrder
             .validateRequest(settlementStrategy, price);
-
-        runtime.amountToDeduct = runtime.totalFees;
         runtime.sizeDelta = asyncOrder.request.sizeDelta;
 
         PerpsMarketFactory.Data storage factory = PerpsMarketFactory.load();
@@ -88,7 +86,10 @@ contract AsyncOrderSettlementPythModule is
         (runtime.pnl, , runtime.chargedInterest, runtime.accruedFunding, , ) = oldPosition.getPnl(
             runtime.fillPrice
         );
-        perpsAccount.applyPnl(runtime.pnl);
+
+        runtime.chargedAmount = runtime.pnl - runtime.totalFees.toInt();
+        perpsAccount.charge(runtime.chargedAmount);
+        emit AccountCharged(runtime.accountId, runtime.chargedAmount, perpsAccount.debt);
 
         // after pnl is realized, update position
         runtime.updateData = PerpsMarket.loadValid(runtime.marketId).updatePositionData(
@@ -108,29 +109,7 @@ contract AsyncOrderSettlementPythModule is
             runtime.updateData.interestRate
         );
 
-        // since margin is deposited when trader deposits, as long as the owed collateral is deducted
-        // from internal accounting, fees are automatically realized by the stakers
-        if (runtime.amountToDeduct > 0) {
-            (runtime.deductedSynthIds, runtime.deductedAmount) = perpsAccount.deductFromAccount(
-                runtime.amountToDeduct
-            );
-            for (
-                runtime.synthDeductionIterator = 0;
-                runtime.synthDeductionIterator < runtime.deductedSynthIds.length;
-                runtime.synthDeductionIterator++
-            ) {
-                if (runtime.deductedAmount[runtime.synthDeductionIterator] > 0) {
-                    emit CollateralDeducted(
-                        runtime.accountId,
-                        runtime.deductedSynthIds[runtime.synthDeductionIterator],
-                        runtime.deductedAmount[runtime.synthDeductionIterator]
-                    );
-                }
-            }
-        }
-        runtime.settlementReward =
-            settlementStrategy.settlementReward +
-            KeeperCosts.load().getSettlementKeeperCosts();
+        runtime.settlementReward = AsyncOrder.settlementRewardCost(settlementStrategy);
 
         if (runtime.settlementReward > 0) {
             // pay keeper

diff --git a/markets/perps-market/contracts/modules/GlobalPerpsMarketModule.sol b/markets/perps-market/contracts/modules/GlobalPerpsMarketModule.sol
@@ -38,27 +38,6 @@ contract GlobalPerpsMarketModule is IGlobalPerpsMarketModule {
         supportedCollaterals = store.supportedCollateralTypes.values();
     }
 
-    /**
-     * @inheritdoc IGlobalPerpsMarketModule
-     */
-    function setSynthDeductionPriority(
-        uint128[] memory newSynthDeductionPriority
-    ) external override {
-        OwnableStorage.onlyOwner();
-        GlobalPerpsMarketConfiguration.load().updateSynthDeductionPriority(
-            newSynthDeductionPriority
-        );
-
-        emit SynthDeductionPrioritySet(newSynthDeductionPriority);
-    }
-
-    /**
-     * @inheritdoc IGlobalPerpsMarketModule
-     */
-    function getSynthDeductionPriority() external view override returns (uint128[] memory) {
-        return GlobalPerpsMarketConfiguration.load().synthDeductionPriority;
-    }
-
     /**
      * @inheritdoc IGlobalPerpsMarketModule
      */

diff --git a/markets/perps-market/contracts/modules/LiquidationModule.sol b/markets/perps-market/contracts/modules/LiquidationModule.sol
@@ -97,6 +97,10 @@ contract LiquidationModule is ILiquidationModule, IMarketEvents {
                 seizedMarginValue,
                 true
             );
+            // clear debt
+            account.updateAccountDebt(-(account.debt.toInt()));
+
+            emit AccountMarginLiquidation(accountId, seizedMarginValue, liquidationReward);
         } else {
             revert NotEligibleForMarginLiquidation(accountId);
         }
@@ -167,9 +171,12 @@ contract LiquidationModule is ILiquidationModule, IMarketEvents {
     function canLiquidateMarginOnly(
         uint128 accountId
     ) external view override returns (bool isEligible) {
-        (isEligible, ) = PerpsAccount.load(accountId).isEligibleForMarginLiquidation(
-            PerpsPrice.Tolerance.DEFAULT
-        );
+        PerpsAccount.Data storage account = PerpsAccount.load(accountId);
+        if (account.hasOpenPositions()) {
+            return false;
+        } else {
+            (isEligible, ) = account.isEligibleForMarginLiquidation(PerpsPrice.Tolerance.DEFAULT);
+        }
     }
 
     /**

diff --git a/markets/perps-market/contracts/modules/PerpsMarketFactoryModule.sol b/markets/perps-market/contracts/modules/PerpsMarketFactoryModule.sol
@@ -129,7 +129,9 @@ contract PerpsMarketFactoryModule is IPerpsMarketFactoryModule {
                 );
             }
 
-            int256 totalDebt = collateralValue.toInt() + totalMarketDebt;
+            int256 totalDebt = collateralValue.toInt() +
+                totalMarketDebt -
+                globalMarket.totalAccountsDebt.toInt();
             return MathUtil.max(0, totalDebt).toUint();
         }
 

diff --git a/markets/perps-market/contracts/storage/GlobalPerpsMarket.sol b/markets/perps-market/contracts/storage/GlobalPerpsMarket.sol
@@ -7,9 +7,9 @@ import {SetUtil} from "@synthetixio/core-contracts/contracts/utils/SetUtil.sol";
 import {MathUtil} from "../utils/MathUtil.sol";
 import {GlobalPerpsMarketConfiguration} from "./GlobalPerpsMarketConfiguration.sol";
 import {SafeCastU256, SafeCastI256, SafeCastU128} from "@synthetixio/core-contracts/contracts/utils/SafeCast.sol";
-import {Price} from "@synthetixio/spot-market/contracts/storage/Price.sol";
 import {PerpsAccount, SNX_USD_MARKET_ID} from "./PerpsAccount.sol";
 import {PerpsMarket} from "./PerpsMarket.sol";
+import {PerpsPrice} from "./PerpsPrice.sol";
 import {PerpsMarketFactory} from "./PerpsMarketFactory.sol";
 import {PerpsCollateralConfiguration} from "./PerpsCollateralConfiguration.sol";
 
@@ -22,6 +22,7 @@ library GlobalPerpsMarket {
     using SafeCastU128 for uint128;
     using DecimalMath for uint256;
     using SetUtil for SetUtil.UintSet;
+    using PerpsCollateralConfiguration for PerpsCollateralConfiguration.Data;
 
     bytes32 private constant _SLOT_GLOBAL_PERPS_MARKET =
         keccak256(abi.encode("io.synthetix.perps-market.GlobalPerpsMarket"));
@@ -61,6 +62,10 @@ library GlobalPerpsMarket {
         mapping(uint128 => uint256) collateralAmounts;
         SetUtil.UintSet activeCollateralTypes;
         SetUtil.UintSet activeMarkets;
+        /**
+         * @dev Total debt that hasn't been paid across all accounts.
+         */
+        uint256 totalAccountsDebt;
     }
 
     function load() internal pure returns (Data storage marketData) {
@@ -107,11 +112,14 @@ library GlobalPerpsMarket {
             if (collateralId == SNX_USD_MARKET_ID) {
                 total += self.collateralAmounts[collateralId];
             } else {
-                (uint256 collateralValue, ) = spotMarket.quoteSellExactIn(
-                    collateralId,
-                    self.collateralAmounts[collateralId],
-                    Price.Tolerance.DEFAULT
-                );
+                (uint256 collateralValue, ) = PerpsCollateralConfiguration
+                    .load(collateralId)
+                    .valueInUsd(
+                        self.collateralAmounts[collateralId],
+                        spotMarket,
+                        PerpsPrice.Tolerance.DEFAULT,
+                        false
+                    );
                 total += collateralValue;
             }
         }
@@ -133,6 +141,11 @@ library GlobalPerpsMarket {
         }
     }
 
+    function updateDebt(Data storage self, int256 debtDelta) internal {
+        int256 newTotalAccountsDebt = self.totalAccountsDebt.toInt() + debtDelta;
+        self.totalAccountsDebt = newTotalAccountsDebt < 0 ? 0 : newTotalAccountsDebt.toUint();
+    }
+
     /**
      * @notice Check if the account is set as liquidatable.
      */

diff --git a/markets/perps-market/contracts/storage/GlobalPerpsMarketConfiguration.sol b/markets/perps-market/contracts/storage/GlobalPerpsMarketConfiguration.sol
@@ -39,9 +39,10 @@ library GlobalPerpsMarketConfiguration {
         // solhint-disable-next-line var-name-mixedcase
         mapping(uint128 => uint256) __unused_1;
         /**
-         * @dev when deducting from user's margin which is made up of many synths, this priority governs which synth to sell for deduction
+         * @dev previously synth deduction priority
          */
-        uint128[] synthDeductionPriority;
+        // solhint-disable-next-line var-name-mixedcase
+        uint128[] __unused_2;
         /**
          * @dev minimum configured keeper reward for the sender who liquidates the account
          */
@@ -153,17 +154,6 @@ library GlobalPerpsMarketConfiguration {
         return MathUtil.min(MathUtil.max(minCap, keeperRewards + costOfExecutionInUsd), maxCap);
     }
 
-    function updateSynthDeductionPriority(
-        Data storage self,
-        uint128[] memory newSynthDeductionPriority
-    ) internal {
-        delete self.synthDeductionPriority;
-
-        for (uint256 i = 0; i < newSynthDeductionPriority.length; i++) {
-            self.synthDeductionPriority.push(newSynthDeductionPriority[i]);
-        }
-    }
-
     function collectFees(
         Data storage self,
         uint256 orderFees,