Synthetics V2, Review 5

Proof of concept: PoC

If the positionPnlUsd is positive but smaller than the priceImpactDiffUsd, the adjustedPositionPnlUsd is set to 0. However the condition for accounting for the pnlDiffAmount and making that amount claimable for the user is dependent on adjustedPositionPnlUsd > 0.

Therefore, cases where the priceImpactDiffUsd cannot be entirely fulfilled by the positionPnlUsd result in the user being unable to claim their pnl that was used to cover a portion of the priceImpactDiffUsd.

Change the condition to adjustedPositionPnlUsd ≥ 0 or make the incrementClaimableCollateralAmount call directly when the PnL is decreased by the priceImpactDiffUsd.

GMX Team: The logic in DecreasePositionCollateralUtils.sol was refactored.

In the processForceClose function, the amountForPool is the remainingCollateral minus the fundingFees.

However it is possible in some cases that this amountForPool includes amounts that were meant to be subtracted from the collateral for other beneficiaries other than the pool. For example the feeReceiver, uiFeeReceiver, and affiliate.

This situation can arise when the pendingCollateralDeduction is only slightly larger than the remaining collateral, and the exact deduction that put the collateral deduction over the remaining collateral threshold is one of these fees that should not be distributed to the pool.

Consider decrementing these fees from the amountForPool and crediting as much as possible to the rightful receivers.

GMX Team: The logic in DecreasePositionCollateralUtils.sol was refactored.

Proof of concept: PoC

The comment in the applyImpactFactor function states the following:

We divide by 2 here to more easily translate liquidity into the appropriate impactFactor values. For example, if the impactExponentFactor is 2 and we want to have an impact of 0.1% for $2 million of difference we can set the impactFactor to be 0.1% / 2 million, in factor form that would be 0.001 / 2,000,000 * (10 ^ 30)

However this additional divisor of 2 is redundant, especially in the given example.

Consider the diffUsd of 2,000,000 and an impactExponentFactor of 2 (ignoring units):

exponentValue = 2,000,000 * 2,000,000; impactFactor = 0.001 / 2,000,000

exponentValue * impactFactor = 2,000,000 * 2,000,000 * .001 / 2,000,000 / 2 = 2,000,000 * .001 / 2

Without the extra division by 2 we already have the result we're looking for, 2,000,000 * .001 = 2,000 since 2,000,000 and 1/2,000,000 cancelled the additional x2 introduced.

This directly contradicts the example given in the applyImpactFactor function.

Remove the redundant 1/2.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

The formula used to compute the executionPrice in the BaseOrderUtils.getExecutionPrice function does not agree with the priceImpactAmount calculation in the PositionPricingUtils.getPriceImpactAmount function during decrease orders.

When calculating the executionPrice, the priceImpactUsd is applied in a fraction with the sizeDeltaUsd. This agrees with the getPriceImpactAmount calculations for increase orders, as the sizeDeltaUsd and the executionPrice determine the trader’s sizeInTokens and therefore their immediate PnL.

However, when closing a position, the trader realizes PnL based on the sizeInTokens and executionPrice, not the sizeDeltaUsd and executionPrice. Therefore the effect that price impact has on the trader’s PnL is not accurately reflected by the calculation for the executionPrice. Ultimately because of this, the executionPrice and resulting trader’s PnL do not agree with the priceImpactAmount generated by the PositionPricingUtils.getPriceImpactAmount function.

Consider applying the priceImpactUsd directly to the trader’s PnL rather than manipulating the executionPrice for decrease orders.

GMX Team: The executionPrice logic was refactored.

The Impact pool pricing should use !maximize for the index token since impactPoolUsd is being deducted, however this calculation uses maximize.

Change maximize to !maximize for the index token valuation.

GMX Team: The recommendation was implemented.

In the OrderUtils.cancelOrder function, the orderVault.transferOut is executed before the order is removed from the dataStore.

Move the removal of the order from the dataStore to before the orderVault.transferOut call.

GMX Team: The recommendation was implemented.

In the event that a trader’s PnL is capped, the PnL they experience from price impact may not be accurately represented by the change in balance of the position impact pool, therefore perturbing the pool value.

For example: A trader is positively impacted but their PnL is capped. The capping of their PnL essentially changes their executionPrice and negates the positive impact they received.

However this positive impact is still removed from the position impact pool to offset the immediate gain in PnL the trader would have realized from the impact.

Therefore the trader does not actually experience the PnL boost from the price impact amount, but that amount is still credited towards the pool value with the removal from the position impact pool.

Consider computing what ought to be removed from the position impact pool after the trader’s PnL is capped.

GMX Team: The logic in DecreasePositionCollateralUtils.sol was refactored.

While converting the adjustedPriceImpactDiffUsd to a collateral token amount, the collateralTokenPrice.max is used. However the collateralTokenPrice.max will result in a smaller adjustedPriceImpactDiffAmount.

In scenarios where the max price has a nontrivial difference with the min price, e.g. a depeg event, this can lead to users paying significantly less for this capped price impact amount than they ought to.

Use the collateralTokenPrice.min when translating the adjustedPriceImpactDiffUsd to a adjustedPriceImpactDiffAmount.

GMX Team: The logic in DecreasePositionCollateralUtils.sol was refactored.

Although rare, there are cases where the pendingCollateralDeduction is smaller than the fees.funding.fundingFeeAmount, resulting in a revert upon the cache.remainingCostAmount calculation in the processForceClose function.

Consider the following:

• Position with 1 token of collateral
• Fees of total 11 tokens
• Funding fees of 3 tokens
• Profit of 9 tokens

In this case, the profit is used to cover 9 tokens of the fees.collateralCostAmount, so the remaining fees.collateralCostAmount is 2 tokens. Therefore the values.pendingCollateralDeduction will be larger than the values.remainingCollateralAmount and the execution will enter the processForceClose function.

However when the remainingCostAmount is computed, the funding fees (3 tokens) will be subtracted from the pending deduction (2 tokens) and revert.

Although this scenario will be rare, the percentage of funding fees that may be covered by position profit should be accounted for to avoid an underflow.

GMX Team: The logic in DecreasePositionCollateralUtils.sol was refactored.

During the force closure of a position during a liquidation or ADL order, the accounting for the position impact pool with applyDeltaToPositionImpactPool is skipped. However the effects of the price impact were already felt on the position’s resulting PnL.

This results in scenarios where a user is significantly negatively/positively impacted during a liquidation/ADL and this amount is not reflected by the position impact pool and so the pool value is asymmetrically effected.

For instance, a user’s PnL is positively impacted by $100 during a force close liquidation. This positive impact is translated to a decrease of the pool value by $100.

The positive impact is not offset by a decrease in the position impact pool, and therefore the pool realizes immediate losses from PI.

Vice-versa for the pool realizing immediate gains on negative price impact, although when a position is negatively impacted, it contributes to the collateral + pnl not being sufficient and therefore the necessary accounting becomes less straightforward. In these scenarios, the position impact pool ought to only be increased by the amount that the position actually experienced, as it wasn’t able to cover it’s entire losses/negative impact — effectively exactly offsetting whatever amount was “payable” (or actually was able to take effect) of the negative impact.

This is somewhat non-trivial to address in the negative impact case as mentioned above, however for the positive impact case, the full impact amount should be applied to the position impact pool, as this full amount is experienced by the trader.

GMX Team: The logic in DecreasePositionCollateralUtils.sol was refactored.

During the processForceClose function, the priceImpactDiffUsd is assigned to 0 in the returned values, however there may have been a nonzero priceImpactDiffUsd that was applied to the adjustedPositionPnlUsd.

At present, this priceImpactDiffUsd would be misrepresented as 0 in the emitPositionDecrease function call.

Compute the amount of priceImpactDiffUsd that was applied to the adjustedPositionPnlUsd and return that as a part of the DecreasePositionCollateralValues in the processForceClose function.

GMX Team: The logic in DecreasePositionCollateralUtils.sol was refactored.

In the event that a position is force closed and the secondaryOutput or remainingCollateralAmount is insufficient to cover the position costs, it may be helpful to emit an event indicating the remainingCostAmount that was left uncovered.

Emit an event at the end of the processForceClose function if the remainingCostAmount is greater than 0. Similar to the logic for emitInsufficientFundingFeePayment.

GMX Team: The logic in DecreasePositionCollateralUtils.sol was refactored.

The PnL of a user’s position is based upon their executionPrice, which deviates from the index price due to price impact. If we consider the scenario of a single trader in the market, when the user decreases their position, the position’s PnL will not be equal to pool’s PnL obtained from MarketUtils.getPnl.

As a result, this may lead to the pool’s PnL less likely to be capped when +PI is experienced since the pool’s PnL will be smaller. Similarly, this may lead to the pool’s PnL more likely to be capped when -PI is experienced since the pool’s PnL will be larger.

Be aware of this edge case, it may not be necessary to address it directly with a code change but is worth considering when configuring the PnL caps as well as other relevant variables.

GMX Team: The executionPrice logic was refactored.

The check if (!primaryPrices[reportInfo.token].isEmpty()) can be performed at the top of the for loop to save gas as it unnecessary to perform all the price processing for this check.

Implement the above recommendation.

GMX Team: The recommendation was implemented.

All calls to emitOraclePriceUpdated have parameter isPrimary as true. Thus, the parameter may be removed.

Implement the above recommendation.

GMX Team: The recommendation was implemented.

The InvalidPoolAdjustment error could be removed as it is never used.

Implement the above recommendation.

GMX Team: The recommendation was implemented.

The cache.collateralForLongs and cache.collateralForShorts amounts have been removed from the aggregate minTokenBalance logic and are now validated individually.

However these values are still added in the result for the getExpectedMinTokenBalance function and reside in the GetExpectedMinTokenBalanceCache.

Remove the cache.collateralForLongs and cache.collateralForShorts values from the summation in the getExpectedMinTokenBalance function as well as the GetExpectedMinTokenBalanceCache struct.

GMX Team: The recommendation was implemented.

The MAX_POSITION_IMPACT_FACTOR_FOR_LIQUIDATIONS key is duplicated in the _initAllowedBaseKeys function.

Remove one of the duplicated allowedBaseKeys[Keys.MAX_POSITION_IMPACT_FACTOR_FOR_LIQUIDATIONS] = true; lines.

GMX Team: The duplicated key was removed.

The msg.sender parameter is crowded on the same line as the oracleParams.

Put the msg.sender parameter on it’s own line.

GMX Team: The recommendation was removed.