legacy market audit fixes (#2309)

* fix migration paused should not allow `migrateOnBehalf`

* fix ensure collateral accounting is correct after liquidation

* theoretical changes for clean expired locks refactor to handle liquidation edge case

* prevent vault being brought into liquidatable state after migration

* add check for vault cratio after delegation change

* fix assigned debt being distributed to other vaults and other pools

* ensure updating rewards for user before liquidation

* prevent divide by 0 when all debt is migrated from v2x

* verify vault c-ratio after minting new debt

* add zero debt burned check just in case

* lint fix

* fix all but one test

* fix the last test

its just an issue finding event

* fix c-ratio handling for vault, small refactor

* change ordering per suggestion

* fix small edge case in cleanAccountLocks

also add tests that i put off

* fix more small bugs found by the auditor

* update warning comment

* add call to distribute debt to pools

in the unlikely case that a pool gets bumped in as a result of the
distribution of debt to a single user, we call distribute debt to pools

* Revert "add call to distribute debt to pools"

This reverts commit 9b95bbc.

markets/legacy-market/contracts/LegacyMarket.sol

@@ -45,6 +45,9 @@ contract LegacyMarket is ILegacyMarket, Ownable, UUPSImplementation, IMarket, IE
     // NOTE: below field is now unused but we leave it here to reduce maintenance burden
     ISNXDistributor public rewardsDistributor;
 
+    // in case an account nft was not able to be transferred to a owner's address due to some error, we allow transferring it later using this structure.
+    mapping(uint256 => address) deferredAccounts;
+
     error MigrationInProgress();
 
     // redefine event so it can be catched by ethers
@@ -162,7 +165,7 @@ contract LegacyMarket is ILegacyMarket, Ownable, UUPSImplementation, IMarket, IE
         uint256 afterDebt = iss.debtBalanceOf(address(this), "sUSD");
 
         // approximately equal check because some rounding error can happen on the v2x side
-        if (beforeDebt - afterDebt < amount - 1) {
+        if (beforeDebt - afterDebt == 0 || beforeDebt - afterDebt < amount - 100) {
             revert V2xPaused();
         }
 
@@ -187,6 +190,10 @@ contract LegacyMarket is ILegacyMarket, Ownable, UUPSImplementation, IMarket, IE
      * @inheritdoc ILegacyMarket
      */
     function migrateOnBehalf(address staker, uint128 accountId) external {
+        if (staker == ERC2771Context._msgSender() && pauseMigration) {
+            revert Paused();
+        }
+
         _migrate(staker, accountId);
     }
 
@@ -277,16 +284,38 @@ contract LegacyMarket is ILegacyMarket, Ownable, UUPSImplementation, IMarket, IE
             debtValueMigrated
         );
 
+        if (v3System.isVaultLiquidatable(preferredPoolId, address(oldSynthetix))) {
+            revert Paused();
+        }
+
         // send the built v3 account to the staker
-        IERC721(v3System.getAccountTokenAddress()).safeTransferFrom(
-            address(this),
-            staker,
-            accountId
-        );
+        try
+            IERC721(v3System.getAccountTokenAddress()).safeTransferFrom(
+                address(this),
+                staker,
+                accountId
+            )
+        {} catch {
+            deferredAccounts[accountId] = staker;
+        }
 
         emit AccountMigrated(staker, accountId, collateralMigrated, debtValueMigrated);
     }
 
+    /**
+     * @dev In case a previously migrated account was not able to be sent to a user during the migration, this function can be
+     * called in order to claim the token afterwards to any address.
+     */
+    function transferDeferredAccount(uint256 accountId, address to) external {
+        if (deferredAccounts[accountId] != ERC2771Context._msgSender()) {
+            revert AccessError.Unauthorized(ERC2771Context._msgSender());
+        }
+
+        deferredAccounts[accountId] = address(0);
+
+        IERC721(v3System.getAccountTokenAddress()).safeTransferFrom(address(this), to, accountId);
+    }
+
     /**
      * @dev Moves the collateral and debt associated {staker} in the V2 system to this market.
      */
@@ -351,6 +380,10 @@ contract LegacyMarket is ILegacyMarket, Ownable, UUPSImplementation, IMarket, IE
             revert V2xPaused();
         }
 
+        if (totalDebtShares == 0) {
+            return 0;
+        }
+
         return (debtSharesMigrated * totalSystemDebt) / totalDebtShares;
     }
 

protocol/synthetix/contracts/interfaces/ICollateralModule.sol

@@ -43,17 +43,10 @@ interface ICollateralModule {
 
     /**
      * @notice Emitted when a lock is cleared from an account due to expiration
-     * @param accountId The id of the account that has the expired lock
-     * @param collateralType The address of the collateral type that was unlocked
      * @param tokenAmount The amount of collateral that was unlocked, demoninated in system units (1e18)
      * @param expireTimestamp unix timestamp at which the unlock is due to expire
      */
-    event CollateralLockExpired(
-        uint128 indexed accountId,
-        address indexed collateralType,
-        uint256 tokenAmount,
-        uint64 expireTimestamp
-    );
+    event CollateralLockExpired(uint256 tokenAmount, uint64 expireTimestamp);
 
     /**
      * @notice Emitted when `tokenAmount` of collateral of type `collateralType` is withdrawn from account `accountId` by `sender`.
@@ -72,6 +65,9 @@ interface ICollateralModule {
     /**
      * @notice Deposits `tokenAmount` of collateral of type `collateralType` into account `accountId`.
      * @dev Anyone can deposit into anyone's active account without restriction.
+     * @dev Depositing to account will automatically clear expired locks on a user's account. If there are an
+     * extremely large number of locks to process, it may not be possible to call `deposit` due to the block gas
+     * limit. In cases such as these, `cleanExpiredLocks` must be called first to clear any outstanding locks.
      * @param accountId The id of the account that is making the deposit.
      * @param collateralType The address of the token to be deposited.
      * @param tokenAmount The amount being deposited, denominated in the token's native decimal representation.
@@ -132,7 +128,7 @@ interface ICollateralModule {
         address collateralType,
         uint256 offset,
         uint256 count
-    ) external returns (uint256 cleared);
+    ) external returns (uint256 cleared, uint256 remainingLockAmountD18);
 
     /**
      * @notice Get a list of locks existing in account. Lists all locks in storage, even if they are expired

protocol/synthetix/contracts/interfaces/IVaultModule.sol

@@ -22,6 +22,11 @@ interface IVaultModule {
      */
     error InvalidCollateralAmount();
 
+    /**
+     * @notice Thrown when there is insufficient c-ratio in the vault after delegating
+     */
+    error InsufficientVaultCollateralRatio(uint128 poolId, address collateralType);
+
     /**
      * @notice Emitted when {sender} updates the delegation of collateral in the specified liquidity position.
      * @param accountId The id of the account whose position was updated.

protocol/synthetix/contracts/modules/core/AssociateDebtModule.sol

@@ -56,21 +56,22 @@ contract AssociateDebtModule is IAssociateDebtModule {
             revert AccessError.Unauthorized(ERC2771Context._msgSender());
         }
 
+        // Remove the pro-rata debt we are about to assign from the market level (ensures it does not leak down to any other pools or vaults that may be connected)
+        marketData.poolsDebtDistribution.distributeValue(-amount.toInt());
+
         // Refresh latest account debt (do this before hasMarket check to verify max debt per share)
         poolData.updateAccountDebt(collateralType, accountId);
 
+        // Rebalance here because the pool may be bumped in/out of range of the market depending on
+        // the debt change that just happened
+        poolData.rebalanceMarketsInPool();
+
         // The market must appear in pool configuration of the specified position (and not be out of range)
         if (!poolData.hasMarket(marketId)) {
             revert NotFundedByPool(marketId, poolId);
         }
 
-        // rebalance here because this is a good opporitunity to do so, and because its required for correct debt accounting after account debt update
-        poolData.rebalanceMarketsInPool();
-
-        // Remove the debt we're about to assign to a specific position, pro-rata
-        epochData.distributeDebtToAccounts(-amount.toInt());
-
-        // Assign this debt to the specified position
+        // We can now reassign this debt to the specified position
         epochData.assignDebtToAccount(accountId, amount.toInt());
 
         // since the reassignment of debt removed some debt form the user's account before it was added, a consoldation is necessary

protocol/synthetix/contracts/modules/core/CollateralModule.sol

@@ -56,6 +56,8 @@ contract CollateralModule is ICollateralModule {
 
         collateralType.safeTransferFrom(depositFrom, self, tokenAmount);
 
+        account.cleanAccountLocks(collateralType, 0, 999999999999999);
+
         account.collaterals[collateralType].increaseAvailableCollateral(
             CollateralConfiguration.load(collateralType).convertTokenToSystemAmount(tokenAmount)
         );
@@ -134,42 +136,8 @@ contract CollateralModule is ICollateralModule {
         address collateralType,
         uint256 offset,
         uint256 count
-    ) external override returns (uint256 cleared) {
-        CollateralLock.Data[] storage locks = Account
-            .load(accountId)
-            .collaterals[collateralType]
-            .locks;
-
-        uint64 currentTime = block.timestamp.to64();
-
-        uint256 len = locks.length;
-
-        if (offset >= len) {
-            return 0;
-        }
-
-        if (count == 0 || offset + count >= len) {
-            count = len - offset;
-        }
-
-        uint256 index = offset;
-        for (uint256 i = 0; i < count; i++) {
-            if (locks[index].lockExpirationTime <= currentTime) {
-                emit CollateralLockExpired(
-                    accountId,
-                    collateralType,
-                    locks[index].amountD18,
-                    locks[index].lockExpirationTime
-                );
-
-                locks[index] = locks[locks.length - 1];
-                locks.pop();
-            } else {
-                index++;
-            }
-        }
-
-        return count + offset - index;
+    ) external override returns (uint256 cleared, uint256 remainingLockAmountD18) {
+        return Account.load(accountId).collaterals[collateralType].cleanExpiredLocks(offset, count);
     }
 
     /**

protocol/synthetix/contracts/modules/core/IssueUSDModule.sol

@@ -78,22 +78,27 @@ contract IssueUSDModule is IIssueUSDModule {
             );
         }
 
+        CollateralConfiguration.Data storage config = CollateralConfiguration.load(collateralType);
+
         // If the resulting debt of the account is greater than zero, ensure that the resulting c-ratio is sufficient
         (, uint256 collateralValue) = pool.currentAccountCollateral(collateralType, accountId);
-        if (newDebt > 0) {
-            CollateralConfiguration.load(collateralType).verifyIssuanceRatio(
-                newDebt.toUint(),
-                collateralValue,
-                pool.collateralConfigurations[collateralType].issuanceRatioD18
-            );
-        }
+        config.verifyIssuanceRatio(
+            newDebt,
+            collateralValue,
+            pool.collateralConfigurations[collateralType].issuanceRatioD18
+        );
 
         // Increase the debt of the position
         pool.assignDebtToAccount(collateralType, accountId, amount.toInt());
 
         // Decrease the credit available in the vault
         pool.recalculateVaultCollateral(collateralType);
 
+        // Confirm that the vault debt is not in liquidation
+        int256 rawVaultDebt = pool.currentVaultDebt(collateralType);
+        (, uint256 vaultCollateralValue) = pool.currentVaultCollateral(collateralType);
+        config.verifyLiquidationRatio(rawVaultDebt, vaultCollateralValue);
+
         AssociatedSystem.Data storage usdToken = AssociatedSystem.load(_USD_TOKEN);
 
         // Mint stablecoins to the core system

protocol/synthetix/contracts/modules/core/LiquidationModule.sol

@@ -28,6 +28,7 @@ contract LiquidationModule is ILiquidationModule {
     using AssociatedSystem for AssociatedSystem.Data;
     using CollateralConfiguration for CollateralConfiguration.Data;
     using Collateral for Collateral.Data;
+    using Account for Account.Data;
     using Pool for Pool.Data;
     using Vault for Vault.Data;
     using VaultEpoch for VaultEpoch.Data;
@@ -52,17 +53,15 @@ contract LiquidationModule is ILiquidationModule {
         // Ensure the account receiving rewards exists
         Account.exists(liquidateAsAccountId);
 
-        Pool.Data storage pool = Pool.load(poolId);
         CollateralConfiguration.Data storage collateralConfig = CollateralConfiguration.load(
             collateralType
         );
-        VaultEpoch.Data storage epoch = pool.vaults[collateralType].currentEpoch();
+        VaultEpoch.Data storage epoch = Pool.load(poolId).vaults[collateralType].currentEpoch();
 
-        int256 rawDebt = pool.updateAccountDebt(collateralType, accountId);
-        (uint256 collateralAmount, uint256 collateralValue) = pool.currentAccountCollateral(
-            collateralType,
-            accountId
-        );
+        int256 rawDebt = Pool.load(poolId).updateAccountDebt(collateralType, accountId);
+        (uint256 collateralAmount, uint256 collateralValue) = Pool
+            .load(poolId)
+            .currentAccountCollateral(collateralType, accountId);
         liquidationData.collateralLiquidated = collateralAmount;
 
         // Verify whether the position is eligible for liquidation
@@ -92,9 +91,20 @@ contract LiquidationModule is ILiquidationModule {
             revert MustBeVaultLiquidated();
         }
 
+        // distribute any outstanding rewards distributor value to the user who is about to be liquidated, since technically they are eligible.
+        Pool.load(poolId).updateRewardsToVaults(
+            Vault.PositionSelector(accountId, poolId, collateralType)
+        );
+
         // This will clear the user's account the same way as if they had withdrawn normally
         epoch.updateAccountPosition(accountId, 0, 0);
 
+        // in case the liquidation caused the user to have less collateral than is actually locked in their account,
+        // this will ensure their locks are good.
+        // NOTE: limit is set to 50 here to prevent the user from DoSsing their account liquidation by creating locks on their own account
+        // if the limit is surpassed, their locks wont be scaled upon liquidation and that is their problem
+        Account.load(accountId).cleanAccountLocks(collateralType, 0, 50);
+
         // Distribute the liquidated collateral among other positions in the vault, minus the reward amount
         epoch.collateralAmounts.scale(
             liquidationData.collateralLiquidated.toInt() - liquidationData.amountRewarded.toInt()
@@ -107,7 +117,7 @@ contract LiquidationModule is ILiquidationModule {
         epoch.distributeDebtToAccounts(liquidationData.debtLiquidated.toInt());
 
         // The collateral is reduced by `amountRewarded`, so we need to reduce the stablecoins capacity available to the markets
-        pool.recalculateVaultCollateral(collateralType);
+        Pool.load(poolId).recalculateVaultCollateral(collateralType);
 
         // Send amountRewarded to the specified account
         Account.load(liquidateAsAccountId).collaterals[collateralType].increaseAvailableCollateral(
@@ -201,6 +211,9 @@ contract LiquidationModule is ILiquidationModule {
 
             // Reduce the collateral of the remaining positions in the vault
             epoch.collateralAmounts.scale(-liquidationData.collateralLiquidated.toInt());
+
+            // ensure markets get accurate accounting of available collateral
+            pool.recalculateVaultCollateral(collateralType);
         }
 
         // Send liquidationData.collateralLiquidated to the specified account

protocol/synthetix/contracts/modules/core/VaultModule.sol

@@ -111,6 +111,8 @@ contract VaultModule is IVaultModule {
             leverage
         );
 
+        _verifyPoolCratio(poolId, collateralType);
+
         _updateAccountCollateralPools(
             accountId,
             poolId,
@@ -132,7 +134,7 @@ contract VaultModule is IVaultModule {
             // Minimum collateralization ratios are configured in the system per collateral type.abi
             // Ensure that the account's updated position satisfies this requirement.
             CollateralConfiguration.load(collateralType).verifyIssuanceRatio(
-                debt < 0 ? 0 : debt.toUint(),
+                debt,
                 newCollateralAmountD18.mulDecimal(collateralPrice),
                 minIssuanceRatioD18
             );
@@ -302,6 +304,19 @@ contract VaultModule is IVaultModule {
         }
     }
 
+    function _verifyPoolCratio(uint128 poolId, address collateralType) internal {
+        Pool.Data storage pool = Pool.load(poolId);
+        int256 rawVaultDebt = pool.currentVaultDebt(collateralType);
+        (, uint256 collateralValue) = pool.currentVaultCollateral(collateralType);
+        if (
+            rawVaultDebt > 0 &&
+            collateralValue.divDecimal(rawVaultDebt.toUint()) <
+            CollateralConfiguration.load(collateralType).liquidationRatioD18
+        ) {
+            revert InsufficientVaultCollateralRatio(poolId, collateralType);
+        }
+    }
+
     /**
      * @dev Registers the pool in the given account's collaterals array.
      */