BFP Market

Proof of concept: PoC

When canceling an order with the cancelOrder function the keeper fee is directly accounted with updateAccountDebtAndCollateral, however if the user has no sUSD collateral updateAccountDebtAndCollateral reassigns the account’s debt to the keeper fee, therefore overwriting any existing debt for the account.

Consider Including the accounts existing debt when charging the fee with the updateAccountDebtAndCollateral function, otherwise create a dedicated function to charge the keeper fee from the user’s margin.

Synthetix Team: The issue was resolved in PR#2079.

Proof of concept: PoC

In the splitAccount function the toAccount which is created from a portion of the fromAccount is validated to meet the minimum initial margin requirement, however the fromAccount is not validated to still uphold the initial margin requirement after the split.

As a result it is possible for the fromAccount to circumvent the minimum initialMargin and create positions that are prone to insolvent liquidations and create bad debt. Additionally it is possible for a malicious actor to liquidate their small position that is left behind this way and wind up with a net profit from the flag and liquidation fee.

Validate that the fromAccount is still above the initial margin requirement after the split occurs.

Synthetix Team: The issue was resolved in PR#2096.

The reportedDebt function aims to report the net funding fees which have yet to be paid to or from traders, among other things.

Following from the documentation in PerpMarket.sol:

/// debtCorrection = positions.sum(p.collateralUsd - p.size * (p.entryPrice + p.entryFunding)) /// marketDebt = market.skew * (price + nextFundingEntry) + debtCorrection

reportedDebt aims to compute the outstanding funding amount via market.skew * nextFundingEntry

• - positions.sum(p.size * p.entryFunding)

However in the implementation of the reportedDebt function, the unrecordedFunding is used as the nextFundingEntry in the equation above. Instead the currentFundingAccruedComputed + unrecordedFunding ought to be used as the nextFundingEntry. This clearly invalidates the computation of the outstanding funding amount as often the individual p.entryFunding values will be larger than the unrecordedFunding portion.

Use currentFundingAccruedComputed + unrecordedFunding instead of just unrecordedFunding when computing the outstanding funding fees of the market.

Synthetix Team: The issue was resolved in PR#2091.

Proof of concept: PoC

Function mergeAccounts can merge any 2 accounts, adding their collateral and combining their positions. If the positions are opposite to each other the accounts will be merged with a reduced position size, however the market size will remain the same. This enables attacker to increase the size of the market as much as they want, while paying only order fees.

The impact from this is complex as size is taken into quite a few calculations and checks:

1. Increasing size increases utilization lowering PnL for other users.
2. Increasing size will reach max OI, which will prevent users from opening trades.
3. Utilizing 100% of the market will cause the LPs to be locked, as delegateCollateral will revert,

which leads to minimumCredit checking if we are over the limit.

delegateCollateral -> _verifyNotCapacityLocked -> findMarketWithCapacityLocked -> isCapacityLocked -> getLockedCreditCapacity -> minimumCredit

Note that this doesn't need to be "exploited", as it will occur naturally with use of the protocol.

Check if the two positions are different and if true update the market size.

Synthetix Team: The issue was resolved in PR#2095.

Proof of concept: PoC

Users can manipulate the skew to gain profit by making large orders and then merging them to use the fill price penalty as a way to gain a profit. A position is opened with the fill price, where depending on the skew the fill price can deviate from the actual price. That deviation is not realized when opening the position, but left as unrealized PnL.

When merging 2 positions mergeAccounts calculates the margin only for the to address, and assumes the from has no outstanding PnL. Then it adds its collateral and debt and sets the price to the current oracle price.

The above enables us to: 1. Have a small position - 10 USD. 2. Make a new bigger position - 100k USD, using 1% of the skew. 3. Use the hooks and merge this new position (deleting the unrealized loss from the fill price discount). 4. Our new position is at the current oracle price, but the skew is still there. 5. Close the position (lowering the skew) to claim the fill price incentive. With the above example any user with enough capital can gain constant profits from the market, while only paying order and keeper fees. Furthermore, the disincentive to imbalance longs and shorts is bypassed.

Calculate any outstanding losses for the fromPosition based on the newly assigned fromPosition.entryPrice and account for these in the newly merged to position.

Synthetix Team: The issue was resolved in PR#2112.

Proof of concept: PoC

Users without sUSD deposited as collateral will accumulate losses in debtUsd. This happens in updateAccountDebtAndCollateral. In the scenario where a trader closes a position, and realizes a loss, there will be some collateral left and a pending debtUsd to pay.

The issue arises when a user tries to withdraw the deposited collateral without any open position. The validatePositionPostWithdraw will fail to do its job, as the position.size is 0, leading to isLiquidateable to return false and im to be 0. Therefore, the protocol will allow users to withdraw all their collateral, even with a pending debt to pay.

Validate the case where there is no open position, checking if the discountedCollateralUsd < debtUsd.

Synthetix Team: The issue was resolved in PR#2100.

Proof of concept: PoC

LP’s which delegated their collateral can exit the pools between BFP flag and liquidate. On flag all of the collateral is distributed instantly, while keeping totalDebt the same (non-sUSD collateral) since the position still exists. However when liquidating totalDebt will change effectively assigning debt to the LP providers.

This allows LPs who have already staked for some time (above requiredMinDelegationTime) to flag a large position, claim the rewards in the payoutToken and then to decrease their exposure with delegateCollateral to 0, skipping the debt exposure when the position gets liquidated.

Consider the following scenario: 1) Bob and Alice both delegated equal amounts of collateral. 2) 10 ETH is distributed upon liquidatable position being flagged. 3) Alice claims her 5 ETH and removes her delegation. 4) The position is liquidated. 5) The debt is solely distributed to Bob’s LP position, so the account’s debt is increased. 6) Alice can re-delegate her collateral and all of the distributed debt is still on Bob’s position.

Ultimately, Alice was able to avoid the socialization of debt while still gaining the same amount of rewards from the RewardDistributor.

This issue ultimately arises from awarding LPs with liquidated collateral before the liquidated position has been cleared from the reportedDebt. Consider avoiding the distribution of all collateral upon flagging. Instead, distribute collateral upon liquidatePosition, proportional with the amount of size being liquidated or only once the position has been entirely liquidated.

Synthetix Team: The introduction of asynchronous delegation will resolve this.

When depositing and withdrawing sUSD collateral, a fee is accrued in the depositMarketUsd and withdrawMarketUsd functions in the MarketManagerModule. For example, in a deposit, the fee is subtracted from the amount and added to creditCapacityD18:

market.creditCapacityD18 += (amount - feeAmount)

However, these fees are handled only inside the Synthetix V3 system, whereas the BFP market adds the entire deposited amount to the trader balance, neglecting the fees:

accountMargin.collaterals[synthMarketId] += absAmountDelta

This way the trader does not experience the fees levied from their collateral deposit and withdrawal actions, rather the liquidity providers and potentially traders with unrealized profits are affected. This can be exploited to manipulate creditCapacityD18 and netIssuanceD18 by continuously calling modifyCollateral in order to increase netIssuanceD18 and decrease creditCapacityD18.

Anyone can batch deposit-withdraw calls in one TX using Morpho flashloans (0% fees) in order to maximize the impact with minimal capital requirements.

Increasing netIssuanceD18 will report more debt for the LPs than actually exists and lower their profits. Decreasing creditCapacityD18 could lead to traders getting stuck inside the BFP as withdrawMarketUsd reverts inside getWithdrawableMarketUsd and withdrawMarketCollateral can revert with newWithdrawableMarketUsd < 0.

Account for the fees being charged in the BFP market, reducing the amount of collateral traders are credited with upon deposits by the fee amount and reducing the amount of collateral traders ultimately redeem from the Synthetix V3 system by the fees.

Synthetix Team: Fees will be removed with SIP-365.

Proof of concept: PoC

In the mergeAccounts function the outstanding PnL for the to account is realized, however the debtCorrection is not updated to account for this realized PnL.

As a result every merge where the to account has outstanding PnL will result in a double counting of that PnL in the debtCorrection, as the skew still includes the old position and the account has now materialized it’s gain or loss into it’s collateral.

Update the debtCorrection for the settlement of the to account’s PnL.

Synthetix Team: The issue was resolved in PR#2133.

The minimumCredit function defines an amount which the available credit capacity cannot go below depending on the size of the market and the current index price. However this calculation perturbs the amount that ought to be reserved for shorts.

For example:

• A market has 10 index tokens as short open interest.
• The market size is 10.
• The price of the index token doubles.
• The minimum credit is now a ratio based on the 10 tokens of short open interest, now valued at

twice the price.

This inaccurately represents the amount of backing liquidity that ought to be reserved for the market as shorts can only ever gain up to their cost-basis and when the price of the index token rises, shorts are in a loss.

Account for the long and short open interest separately when computing how much backing liquidity ought to be reserved.

For example, the GMX V2 system reserves liquidity based upon openInterestInTokens * price for longs and openInterest (position cost) for shorts.

Synthetix Team: Acknowledged.

During liquidatePosition, updateMarketPreLiquidation is called to perform pre-steps and validation, including the recomputation of utilization. market.recomputeUtilization is incorrectly called before market.size is reduced by the liquidation size. The new utilization rate should be calculated with the new market size, similar to settleOrder in OrderModule.sol.

By calling recomputeUtilization before updating size, the liquidation leaves utilization rate unchanged when it should have reduced it, affecting all remaining traders.

Assume the utilization rate was very high, and a large position was just liquidated. This should in effect bring down utilization rate and improve the margins of all other traders. However, due to this error, the margins of all other traders remain unchanged, which could then lead to unfair liquidations.

Call recomputeUtilization after market size and skew are updated in the updateMarketPreLiquidation function.

Synthetix Team: The issue was resolved in PR#2101.

Proof of concept: PoC

In the isMarginLiquidatable function, accounts are determined to be liquidatable when they have a discountedMarginUsd of 0. However this puts the protocol at risk of taking on bad debt as there is no requirement that the margin is able to cover liquidator fees as well as potential instantaneous collateral price decreases.

Furthermore, an issue arises with low price collateral assets. An attacker can deposit a very small amount of collateral (1 wei), and the validation for isMarginLiquidatable will return true, as the discountedCollateralUsd will be 0 when collateral price is below 1e18: discountedCollateralUsd += available.mulDecimal(discountedCollateralPrice);

The attacker will then liquidate the account and earn keeper fees, withdrawing sUSD from the V3 pool.

Change the definition of the isMarginLiquidatable function such that positions will be considered liquidatable by margin only if their discountedMarginUsd cannot cover liquidation keeper rewards, optionally as well as a safety bound for potential downward collateral price gaps.

Additionally, validate that positions hold enough collateral so that they are not liquidatable by margin only upon any action that would modify their position’s collateral.

Synthetix Team: The issue was resolved in PR#2116.

In the BFP market, sUSD collateral is deposited by traders and counted towards the creditCapacity of the market. This creditCapacity is ultimately compared against the minimumCredit to validate that there is sufficient backing liquidity to safely operate the market.

However trader collateral should not be included in this validation as it cannot be locked in the event that the market's isCapacityLocked validation fails, failing to safely back the market with a minimum amount of liquidity and adequately back trader’s positions. Additionally trader’s profits which are settled to sUSD ought to always be able to be covered by backing liquidity, however this amount is not accounted for in the minimumCredit.

Consider accounting for the trader’s deposited sUSD collateral when reporting the minimumCredit for the BFP market such that the creditCapacity must include the desired minimum amount in addition to the creditCapacity generated by the trader’s sUSD collateral.

Since the additional sUSD depositedCollateral will include all settled profits and only some of the realized losses which can be covered directly by sUSD collateral, it may also be pertinent to reduce the minimumCredit by the net debt losses for all accounts. However the most conservative validation would be to count only the depositedColalteral value for sUSD.

Otherwise consider a larger refactor of the way trader sUSD collateral is handled, perhaps restricting it’s withdrawal when the minimumCredit is breached or removing it from having an effect on the creditCapacity entirely.

Synthetix Team: The issue was resolved in PR#2134.

Traders are allowed to open and settle orders even when the existing market positions cannot be adequately supported by the liquidity backing the BFP market. Even if the existing minimumCredit is too large for the backing creditCapacity, traders can continue to increase the market size and therefore increase the uncovered gap between the minimumCredit and the lacking creditCapacity.

As a result the BFP market can easily become insolvent in the event that traders continue to open positions without consideration for the backing liquidity. This leads to a market state where sUSD collateral withdrawals are DoS'd as well as any settlement for positions in a profit.

Validate that orders that would create new positions or increase existing positions do not invalidate the minimumCredit validation for the market.

Synthetix Team: The issue was resolved in PR#2128.

In the Position.validateTrade function the params.fillPrice is used to compute the marginValues, which include the funding and utilization fees based upon that price. However the funding and utilization fees should not be based upon the fillPrice, as this price includes a premium/discount according to how the trade affects the market skew.

This will lead to shorts paying less fees when they push the skew increasingly short, or longs paying more fees when they push the skew increasingly long.

Additionally, when the currentFundingAccruedComputed and currentUtilizationAccruedComputed is accounted for the market with the recomputeUtilization and recomputeFunding functions these values are based upon the pythPrice. As a result traders will experience a discrepancy in the amount of funding and utilization fees paid to the market’s recorded funding and utilization accrued values.

Consider using the params.oraclePrice specifically for the funding and utilization fee calculations when computing the marginValues in the Position.validateTrade function.

Synthetix Team: The issue was resolved in commit 7c5d2fa.

When computing the current utilization in the PerpMarket.getUtilization function if the backing liquidity is over-utilized, e.g. delegatedCollateralValueUsd < 0, then a utilization of 100% is returned.

However when the backing liquidity is exactly 100% utilized, e.g. delegatedCollateralValueUsd == 0 then the function will attempt to divide the lockedCollateralUsd by the delegatedCollateralValueUsd, resulting in a divide by 0 panic revert.

However, the risk of DoS is unlikely as the utilization is unlikely to be able to get to exactly 100%.

Modify the if condition on line 191 such that the PerpMarket.getUtilization function early returns if delegatedCollateralValueUsd <= 0.

Synthetix Team: The issue was resolved in PR#2085.

In the splitAccount function the feature flag validation is performed with the Flags.MERGE_ACCOUNT feature, meanwhile the Flags.SPLIT_ACCOUNT feature ought to be used. This can allow an account which does not have permission to split their account to do so anyway.

Validate the feature flag based upon the Flags.SPLIT_ACCOUNT feature in the splitAccount function.

Synthetix Team: The issue was resolved in PR#2084.

In the validateNextPositionEnoughMargin function the Maintenance Margin is calculated through the getLiquidationMarginUsd function. However the invocation wrongly passes in the nextMarginUsd to compute the flagReward when it should be passing in collateralUsd.

The flagPosition function computes the flagReward based upon the collateralUsd of the position, therefore using nextMarginUsd to compute the flagReward inaccurately accounts for the amount in the liquidation check.

Provide the collateralUsd as the second to last parameter when invoking the getLiquidationMarginUsd function within the validateNextPositionEnoughMargin function.

Synthetix Team: The issue was resolved in PR#2097.

In the validateNextPositionIm function the getLiquidationMarginUsd function is used to compute the initial margin value that the position must uphold. However the initial margin value is computed based upon the newPosition.entryPrice rather than the oraclePrice.

This is in direct contradiction to the price used to calculate and validate the maintenance margin for the position in the validateNextPositionEnoughMargin function, which uses the oraclePrice.

This leads to a discrepancy in the validation performed on a position when validating a trade. The initial margin is validated based upon the entryPrice while the maintenance margin is validated based upon the oraclePrice.

In the validateNextPositionIm function, call getLiquidationMarginUsd with the oraclePrice instead of entryPrice.

Synthetix Team: The issue was resolved in PR#2097.

Proof of concept: PoC

When merging accounts, the function executes getMatchingMarketCollateral which returns the matching margin collateral equal to market. This function will return a matching synthMarketId and fromAccountCollateral.

If the fromId account has multiple collaterals, the getMatchingMarketCollateral function will only return the last collateral that matched. This means that the merge will transfer one collateral to the toId account, and leave the other 2 in the fromId account.

The issue is that the merge will then transfer a portion of the collateral but the whole position size. If the last collateral that matched is the smallest one in terms of USD, then the toPosition might invalidate the Initial Margin check and revert.

Transfer all collaterals with available balance to the toId account.

Synthetix Team: The issue was resolved in PR#2095.

Users can set their own keeperFeeBufferUsd and limitPrice, potentially forcing bad debt into the system. This is because IM and MM have minimum values as follows: IM = 2% * position.size + fixed = 2% * p.size + 50 MM = 1% * position.size + fixed + liqFlagReward + keeper reward = 1% * p.size + 50 + liqFlagReward + keeper reward While the maximum value for keeperFeeBufferUsd is 100 USD.

Currently, it's possible to place an order with keeperFeeBufferUsd > collateral > IM && MM, choosing the maximum keeperFeeBufferUsd and an unrealistic limitPrice, using it to cancel your order later and accrue bad debt of keeperFeeBufferUsd - collateral.

Similarly users can submit orders that would decrease their position size by a trivial amount and avoid the liquidatable checks upon settlement, allowing the keeper fee to place their position in a liquidatable or even insolvent state.

Consider raising the minMarginUsd such that it would not be possible for a keeper fee to exceed an account’s margin and cause bad debt to occur. Otherwise consider preventing orders from being created where the keeper fee would cause the account to ultimately become liquidatable.

Synthetix Team: The issue was resolved in PR#2117.

If skewScale is set to 0, arithmetic operations which divide by skewScale will revert. This occurs in the Order.getFillPrice and Position.validateLiquidation functions. While it is unlikely that skewScale will ever be set to 0, this is a possibility and is specifically handled in many areas of the codebase.

In getFillPrice and validateLiquidation handle the scenario for skewScale == 0 and avoid division by 0.

Synthetix Team: The issue was resolved in PR#2108.

Order execution with the settleOrder function requires that the priceUpdateData provided to parse the pythPrice is for the price update that satisfies the minimum and maximum times (commitmentTime + 12 seconds, commitmentTime + 60 seconds) as well as that the publish time of the price update that is sequentially previous to the provided price data took place before the minimum commitmentTime. Therefore only prices that satisfy these constraints may be used to execute an order while it is ready and not stale.

A malicious user may prevent an order from being executable in the block where the valid price data is accurate and only allow the order to go through once a significant period of time has passed and the user observes that the true current price of the index asset has moved in their favor. The order will only be executable with the outdated price, and therefore the user will realize a risk-free profit based upon how much price has diverged in the user’s favor since then.

The malicious user may prevent their order from being executable by registering a mergeAccounts hook as a callback where the user’s position as the fromAccount has sUSD collateral in addition to the market’s index as collateral and therefore reverts. When the malicious user wishes their order to be executable, e.g. they have determined that price has moved in a direction that is favorable to them, they may remove the sUSD collateral with the payDebt function, assuming they have a pre-existing position with debt, and execute their order with the outdated price.

Consider increasing the orderFee percentage that is taken to dissuade from any risk-free short term trades. Otherwise consider removing the possibility for users to control whether or not their orders are executable by way of callbacks through a try/catch wrapper.

Synthetix Team: The issue was resolved in PR#2095.

Every keeper operation is rewarded with a specific keeper fee, where the gas units are fixed and the fee is calculated on the spot with: gas * gasPrice + keeperProfit. However, some operations can use much more gas than others.

Example:

• Settling a normal order versus one with 3 hooks.
• Flagging a position with 1 collateral versus one with 10 collaterals.
• Liquidating a position where you don't need to loop through the window for previous liquidations

versus one where you need to loop through every block (using the while).

As a result some orders will be significantly more profitable than others, and some orders may end up being unprofitable entirely, even with a fee buffer, and as a result won't be executed.

Consider tracking the gas used during an execution with gasLeft and use this amount to compute the keeper’s fee with an added profit margin.

Synthetix Team: Partially Resolved.

In the mergeAccounts and splitAccounts functions the IM is validated against the non-discounted margin value. However to be as conservative as possible the IM ought to be validated against the discounted margin value of these accounts

Validate the accounts in the mergeAccounts and splitAccounts functions against the IM based upon the discounted value of the margin.

Synthetix Team: The issue was resolved in PR#2112.

Throughout the BFP market actions are allowed to take place when an account involved is liquidatable by margin only, e.g. the account has a zero discounted margin value and a nonzero collateral value.

Orders can be settled and positions can be merged and split to accounts that are liquidatable by margin only. This can lead to users errantly creating orders for accounts that are about to be liquidated, and thus having the orders cancelled.

This behavior also introduces additional attack surface by allowing these accounts to be involved in such actions, which could potentially lead to unexpected scenarios.

Consider validating that accounts are not liquidatable by margin only in the validateTrade, mergeAccounts, and splitAccount functions.

Synthetix Team: The issue was resolved in PR#2115,.

Currently the system uses parsePriceFeedUpdatesUnique to get the first unique price for the given time period.

Pyth provides instant prices, but because market price discovery takes time and happens gradually over all of the markets Pyth has implemented confidence in their system. For example, the returned price for ETH can be $2000 with confidence of +-20 USD. This means the real price of ETH can range from $1980 to $2020.

Currently there are no checks for confidence, which can lead to users not being liquidated in time, lowering the profits for LP providers, or putting them in debt.

Consider using the confidence intervals as described in Pyth’s best practices. If someone wants to open a derivative contract, their collateral may be valued at the lower price. However, if deciding whether someone's margin limits were violated, value their outstanding leveraged position at the higher price.

Synthetix Team: Acknowledged.

In validateLiquidation, liqKeeperFee is calculated based on liqSize which is expected not to exceed maxLiquidatableCapacity. Therefore, getLiquidationKeeperFee is expected to calculate iterations = 1 and return liquidationFeeInUsd * 1.

However, when an endorsed keeper performs a liquidation, liqSize may exceed maxLiquidatableCapacity. In this case, iterations could exceed 1, and the keeper will receive multiples of the keeperFee despite only performing one liquidation.

For example, if liqSize = 10 but maxLiquidatableCapacity = 2, then the keeper will receive five times the keeperFee.

Consider whether this is desired behavior. If it is not desired, then consider adding to getLiquidationKeeperFee:

if (ERC2771Context._msgSender() == globalConfig.keeperLiquidationEndorsed) {
     iterations = 1;
}

Synthetix Team: Acknowledged.

The documentation for the getUtilization function states that the collateralUtilization is between zero and one, however this is not true as the lockedCollateralUsd / delegatedCollateralValueUsd ratio is not guaranteed to be below one.

While delegated collateral is locked when it’s value drops below the minimumCredit (e.g. lockedCollateralUsd), this does not guarantee that the delegatedCollateralValueUsd will always be greater than the minimumCredit. The delegatedCollateralValueUsd may fall below the minimumCredit based on price action as well as increases in the minimumCredit by the introduction of new positions.

As a result the utilization can be above 1e18, which can lead to unexpected utilization fees as well as a DoS when computing the utilization rate with the getCurrentUtilizationRate function. If the utilization rate is allowed to hit > 3e18 then the highUtilizationRateInterest calculation is at risk of overflowing the uint128 and halting all order execution and liquidations in the BFP market.

Consider explicitly bounding the result from getUtilization below 1e18.

Synthetix Team: The issue was resolved in PR#2123.

In the test environment the maxKeeperFeeUsd is between $50 and $100, however this amount will be insufficient to incentivize the execution of orders and flagging of liquidatable positions during periods of high network usage and gas fees.

The maxKeeperFeeUsd ought to be raised to sufficiently incentivize timely order execution and liquidation.

Subsequently, some users may be unwilling to pay for extremely high execution costs during these times of high network usage. For these users it may be useful to have an order specific maximum keeper fee to limit the potential cost to their account’s margin.

Consider raising the maxKeeperFeeUsd to a value that does not constrict the incentive for keepers to execute orders and flag positions for liquidation during periods of high gas fees. Based on the keeperSettlementGasUnits of 1.2 million and assuming an aggressive base fee of 40 gwei at a current price of $3,500 per ETH, the maxKeeperFeeUsd ought to be assigned to roughly $200+ to allow for appropriate incentives for order execution during periods of high network usage.

Additionally consider implementing a maximum keeper fee value that is configurable on a per-order basis to allow users to set their tolerance for network fees.

Synthetix Team: Resolved.

Users can opt to use settle order hooks, called at the end of the settleOrder function. These hooks will be mainly used for splitting and merging accounts when settling orders. These hooks require explicit permissions from the account holders: _PERPS_MODIFY_COLLATERAL_PERMISSION.

If a user commits an order with one of this hooks, they can front run the keeper order and remove the account permissions, reverting the transaction.

Be aware and clearly document that this can be an issue for the keepers.

Synthetix Team: The issue was resolved in PR#2126.

Upon cancelling an order, the keeper is compensated with the same fee they would receive for executing the order. However the gas cost necessary to execute an order is far more than the gas cost to cancel an order.

Additionally, keepers will receive the keeperFeeBufferUsd as profit when cancelling an order, when this amount is meant instead to incentivize the execution of an order. As a result keepers receive far more profit for cancelling orders rather than executing them. Therefore, keepers will not only be overcompensated for cancellation, but highly incentivized to front-run users who are cancelling their own orders to cancel them on the user’s behalf and collect a fee from the user’s margin.

Consider implementing a fee calculation that is specific to cancellation remuneration rather than overcompensating keepers for the cancellation with the same fee they would receive from execution.

Synthetix Team: The issue was resolved in PR#2117.

New positions are validated when the order is settled by checking if the margin minus the fees will satisfy the initial and maintenance margin checks. This fees include order, keeper, flag and liquidation fees.

The issue is that the calculations of these fees rely heavily on the block.basefee and eth price, the only parameters that the protocol can't control. This base fee can range from 10-15 Gwei in a normal market scenario, up to >600 Gwei when volatility is high.

Therefore, users can open LONG positions when the block base fee is low, and get liquidated a few blocks later if the base fee increases, even if the market price moves in their favor. Potentially, this can cause an insolvent liquidation for small accounts since the base fee jump might be an unpredictable stepwise change.

Consider increasing the market minMarginUsd to the point that it can cover the volatility of the base fee and reduce the risk of an insolvent liquidation.

Synthetix Team: Resolved.

Similarly to M-09, it is possible to make a short term risk free trade by preventing keepers from executing an account’s order by including a splitAccount callback where no keeper is granted the necessary _PERPS_MODIFY_COLLATERAL_PERMISSION for the fromId account.

As a result, a malicious user may prevent an order from being executable in the block where the valid price data is accurate and only allow the order to go through once a significant period of time has passed and the user observes that the true current price of the index asset has moved in their favor.

The order will only be executable with the outdated price, and therefore the user will realize a risk-free profit based upon how much price has diverged in the user’s favor since then.

Consider increasing the orderFee percentage that is taken to dissuade from any risk-free short term trades. Otherwise consider removing the possibility for users to control whether or not their orders are executable by way of callbacks through a try/catch wrapper around the callbacks.

Synthetix Team: The issue was resolved in PR#2126.

In the BFP market users can leave accounts with realized debtUsd and enough collateral to support that debtUsd without ever repaying the debt. The debt inside these accounts will be reported in reportedDebt as profits for the LPs, however the sUSD will never be returned and never increase the creditCapacity for the market since the debt is not repaid.

Over time with many positions holding unpaid debt there is an increased risk of reducing the getWithdrawableMarketUsd to a point where traders are unable to withdraw their sUSD collateral or claim their sUSD profits.

Consider implementing an interest rate on unpaid debt, where traders pay a configurable rate on debt that has not yet been repaid in sUSD.

Synthetix Team: The issue was resolved in PR#2170.

Throughout the codebase there are many instances where the supportedSynthMarketIds list is iterated over and often expensive operations are performed. When configuring the supportedSynthMarketIds list with the setMarginCollateralConfiguration function there is no validation on the maximum length of the list.

As a result it may be possible for the length of the supportedSynthMarketIds to be assigned such that many operations are extremely gas intensive or entirely DoS’d as they would exceed the block gas limit.

This could cause significant harm if e.g. an account liquidation would require too much gas to be executed. However it is unlikely that the supportedSynthMarketIds list would grow extremely long as currently only three synthetic assets are planned to be supported.

Consider implementing validation in the setMarginCollateralConfiguration function such that the supportedSynthMarketIds cannot exceed an acceptable length.

Synthetix Team: The issue was resolved in PR#2084.

In the liquidateMarginOnly function the comment on line 370 indicates that the keeperReward will go to pay the flagger, however this amount is not paid to the flagger instead it is paid to the msg.sender.

Update the comment to be clear that this fee is intended to be sent to the msg.sender, not the flagger.

Synthetix Team: The issue was resolved in PR#2084.

On line 218 in the PerpAccountModule, toPosition is misspelled as toPostion.

Correct it to toPosition.

Synthetix Team: The issue was resolved in PR#2084.

In the reportedDebt function there is a short-circuit case which saves gas when market.skew == 0 && market.debtCorrection == 0 && market.totalTraderDebtUsd == 0.

The expensive operation being avoided is the market.getOraclePrice call, however this expensive operation can be avoided in the more general case where market.skew == 0.

In order to avoid the expensive market.getOraclePrice call in as many scenarios as possible, consider changing the short-circuit case to the following:

if (market.skew == 0)
    return totalCollateralValueUsd - market.debtCorrection.toUint() -
market.totalTraderDebtUsd;

Synthetix Team: The issue was resolved in PR#2084.

Changing any market parameters can instantly put users at risk of liquidation. This is because most parameters are closely tied to liquidation calculations, and altering just one can change the liquidation threshold for users. Examples include flag fees, maker and taker fees, and skew scale which affect the liquidation keeper fees, etc.

Consider introducing a timelock for modifying parameters that could cause positions to be at risk of liquidation.

Synthetix Team: Acknowledged.

In updateMarketPreLiquidation the following optimization can be made:

market.updateDebtCorrection(market.positions[accountId], newPosition); can be changed to market.updateDebtCorrection(oldPosition, newPosition);

Consider implementing the suggested optimization.

Synthetix Team: The issue was resolved in PR#2136.

In the getCurrentUtilizationRate function the utilization < utilizationBreakpointPercent case can be optimized to account for the case where utilization == utilizationBreakpointPercent.

This is because when the utilization is the same as the utilizationBreakpointPercent the resulting value is entirely based upon the lowUtilizationSlopePercent.

Consider updating the low utilization case to: utilization <= utilizationBreakpointPercent.

Synthetix Team: The issue was resolved in PR#2136.

Permission can be granted for arbitrary addresses to payDebt on behalf of other users. However, when another user pays back the account's debt, the system will still deduct from the account's sUSD collateral.

The accountId is checked for any sUSD, and if found, the system reduces the paid amount by this sUSD while also reducing the account collateral in the sUSD market. This behavior may be unexpected for the user who is making the debt payment and can unintentionally place positions at a greater risk of liquidation than intended.

Be sure to document this behavior to users as it may be unexpected. Otherwise consider adding a boolean parameter and use it to determine if sUSD from the account's margin should be used.

Synthetix Team: The issue was resolved in PR#2119.

The updatePythPrice internal function in the PerpMarket library is not used. It seems this was meant to be used to manually update pyth prices without the need to create or cancel an order.

Additionally, this functions creates extra deployment costs, besides causing confusion to the reader of the contract.

Remove the unused internal function or add the external function with a proper access control so that pyth prices can be updated manually.

Synthetix Team: The issue was resolved in PR#2119.

The getProportionalFundingElapsed function performs divDecimal between the block.timestamp - self.lastFundingTime and 1 days. Both of these values can be divided as uint values, however toInt is invoked on the time since the last funding. As a result the divDecimal overload accepting two int256 variables is used.

This produces no unexpected behavior with the values being used, however to maximize the allowed domain space for these values, uint values ought to be used instead.

Consider modifying the getProportionalFundingElapsed definition such that the divDecimal overload accepting uint values is used:

return (block.timestamp - self.lastFundingTime).divDecimal(1 days).toInt();

Synthetix Team: The issue was resolved in PR#2119.

keeperLiquidationEndorsed may not always be able to liquidate a user on the first attempt. This limitation arises because the condition allowing them to liquidate 100% of a user's position requires two criteria to be met: ERC2771Context._msgSender() == globalConfig.keeperLiquidationEndorsed and runtime.remainingCapacity == 0. This finding concerns the second criterion.

For the endorsed keeper to liquidate 100% of a user's position, remainingCapacity must be 0. However, this is not always the case as remainingCapacity restores (increases) with each block, due to the expiration of old liquidations.

In a busy market, it's common for there to be liquidations in every block. If our keeper is the first to attempt liquidation in the current block, remainingCapacity will **not be 0**. This results in the endorsed keeper liquidating only a tiny fraction of the user's position.

Consider the following example: maxLiquidatableCapacity = 80 ETH with a refresh rate of 30 seconds on Base (15 blocks). 1. The market is volatile and busy, with significant MEV and front-running activity. 2. Small liquidations occur every block, depleting the capacity. 3. A large position needs immediate liquidation as prices move quickly. 4. The keeper, being first to call liquidate, but remainingCapacity has replenished with a few ETH. 5. The keeper liquidates only a small fraction of the order. Under these circumstances, the endorsed keeper needs to call liquidate again in the same block, before remainingCapacity is replenished in the next.

Consider removing the runtime.remainingCapacity == 0 requirement for the endorsed keeper. If the endorsed keeper calls to liquidate a user, that user often must be liquidated in entirety in a timely manner.

Synthetix Team: Acknowledged. 59

When computing the liquidation flag reward in the getLiquidationFlagReward function the keeper receives a profit margin from two sources: the maximum of the keeperProfitMarginPercent or the keeperProfitMarginUsd as well as the liquidationRewardPercent.

This doubly incentivizes liquidators to flag positions, which may be intentional to ensure liquidations always occur in a timely manner, but raises an inconsistency with the way other keeper actions such as executing liquidations or settling orders are remunerated.

Consider if it is expected for liquidation flaggers to be compensated with profit margin twice, if it is not then adjust the getLiquidationFlagReward function accordingly.

Synthetix Team: Acknowledged.

The withdrawAllCollateral function prevents users from withdrawing their collateral if debtUsd is non-zero. If this user has previously given permissions to the merge settlement hook, a malicious user can front run a withdrawal, and settling an order with this hook, transferring some debtUsd (even 1 wei) to the target account, forcing the withdrawAllCollateral function call to revert.

Document this issue to users so they can revoke any permissions previously given to other contracts before they attempt to withdraw their collateral or otherwise accept the risk of this occurring.

Synthetix Team: The issue was resolved in PR#2126.

The owner address can configure the collaterals supported by the market using the setMarginCollateralConfiguration. This function will internally make an infinite approval to several addresses. Firstly an approval is made to the core Synthetix address, which is necessary to transfer market deposited collateral from traders.

Secondly, the spot market is approved however this approval amount is never utilized and unnecessarily introduces risk to the BFP market.

Thirdly, the BFP market address itself is approved to transfer all sUSD synths, however nowhere in the modules which will act on this address is this approval amount used.

If there is a reason for these approvals then clearly document them. Otherwise, remove the unnecessary approvals to the spot market and to the BFP market contract itself.

Synthetix Team: The issue was resolved in PR#2119.

Splitting an account with 100% proportion does not delete the fromId position from the positions mapping. Although this yields a position with size 0, the logic should coincide with the settleOrder logic, where the position is deleted from the positions mapping when size is zero.

Remove the from position from the positions mapping if it is found to have zero size after splitting.

Synthetix Team: The issue was resolved in PR#2119.

Users can grant permissions to other addresses so that they can interact with their accounts. The issue is that _PERPS_MODIFY_COLLATERAL_PERMISSION may be too lenient for the address, as this role can now deposit/withdraw collateral, pay debt, split and merge accounts.

This prevents users to give specific permissions, for example, if they only want to allow a user to deposit or pay debt, but not withdrawing collateral. Furthermore, if an address has this permission it can open a new position for the user with mergeAccount, which can be unexpected for users.

Consider creating more granular permissions for account access. Otherwise, clearly document this access control behavior to users.

Synthetix Team: The issue was resolved in PR#2140.

In the getWithdrawableMargin function when the size of the position is 0 the collateralUsd - debtUsd of the account is the uint value returned. However there is no guarantee that this resulting value will be a positive integer.

Ideally accounts will always remain solvent and be liquidated in a timely fashion before their debt is able to eclipse their collateral, but in the scenario where this does occur the getWithdrawableMargin function ought to not revert due to underflow and return a result of 0.

This way integrating systems relying on getWithdrawableMargin will not experience a DoS when positions enter an insolvent state.

Return the minimum of collateralUsd - debtUsd and 0.

Synthetix Team: The issue was resolved in PR#2135.

The liquidator keeper fee is calculated based on a block base fee, eth price and some market and global configs. When a position is flagged, this fee is accounted for in the maintenance margin. Depending on the position size and the remainingCapacity of the market for liquidations, a position can be either liquidated in a single transaction or multiple partial liquidation transactions. The iterations are calculated based on an ideal scenario where there is full capacity when liquidating the entire position.

Therefore, after a position is flagged, the keeper can actually earn multiple liqReward amounts: Let's take an example of a 10 ETH position that needs to be liquidated 1. remainingCapacity in window = 1. 2. Position flagged and partially liquidated (10-1 = 9). 3. Next block, call liquidatePosition again, depending on prev market liquidations in window, keeper either liquidates fully or partially again.

This means that a keeper may earn 2 or more liquidation fee rewards, when only 1 was accounted for in the margin requirement. In the worst case, a small account happens to be liquidated on the precipice of the liquidation capacity for a single block, and must be liquidated over two blocks. However the maintenance margin for this account did not factor this in, and therefore the additional liquidation fees cannot be covered by the margin, resulting in bad debt.

Consider raising the minimum required collateral for a position such that small positions cannot inadvertently cause bad debt through experiencing more liquidation fee iterations than expected. Additionally, the margin requirement threshold could pessimistically assume that some liquidation capacity will have been taken up in the block when the account is liquidated and account for an additional liquidation iteration when the position is above a certain size.

Alternatively, consider solutions which would account for the current used liquidation capacity, factoring in if a position would require more than one liquidation given the amount of capacity that is currently used up. Though such a solution may be dissatisfactory as it introduces a potential liquidation manipulation vector.

Finally, consider the use of a monitoring system that checks the iterations and liquidation sizes for flagged positions. This way endorsed keepers could be used in outlier scenarios to prevent overpaying liquidation fees by liquidating smaller accounts in a single transaction when they would otherwise incur bad debt.

Synthetix Team: Acknowledged.

The following functions have incorrect NatSpec:

• recomputeUtilization is using the documentation from the recomputeFunding function.
• getSettlementKeeperFee states that the fee is used for liquidations, which is incorrect.

Correct the NatSpec for these functions.

Synthetix Team: The issue was resolved in PR#2119.

In the comment describing the early return case in the getUtilization function, positions is misspelled as postions.

Correct postions to positions.

Synthetix Team: The issue was resolved in PR#2119.

In the Margin library the docstring for the getOracleCollateralPrice misspells oracleManager as oraclerManager.

Correct it to oracleManager.

Synthetix Team: The issue was resolved in PR#2119.

In the comment on line 283 in the settleOrder function getHealthData is misspelled as getHeathData.

Correct it to getHealthData.

Synthetix Team: The issue was resolved in PR#2119.

In the comment on line 269 in the validateTrade function, as is repeated twice.

Remove the second instance of the word as.

Synthetix Team: The issue was resolved in PR#2119.

The protocol is able to revoke access to the DEPOSIT feature, which prevents users from depositing more collateral. It is possible to be in a state such that adding collateral is prevented, yet liquidations can still occur. In such a scenario, users can be unfairly liquidated as they are unable to make their position healthy.

Consider also pausing liquidations for positions that become liquidatable or only allowing liquidations to be made by trusted addresses during this period. Otherwise, clearly document this behavior to users.

Synthetix Team: Resolved.