Synthetics V2, Review 6

Proof of concept: PoC

When the fees.totalCostAmountExcludingFunding is paid with any amount of secondary tokens the fees object is replaced with an empty instance.

In this case however there can be a position that remains, and it will be stamped with a fundingFeePerSize value of 0 from this empty fees instance. Therefore the position must now pay all funding fees since the inception of the market.

In all likelihood the position will then be immediately liquidated leading to an immediate significant loss of assets for the position that would have remained.

Do not reset the fees.funding.funding.latestFundingFeeAmountPerSize on the fees object when the position can remain, e.g. outside of any insolvent close.

GMX Team: The fees.funding.funding.latestFundingFeeAmountPerSize is no longer zeroed out.

The willPositionCollateralBeSufficient validation aims to decide whether or not the collateral amount that remains for a position will be sufficient for it's leverage.

However in many cases this validation allows decrease orders which will put the position’s collateral below the “sufficient threshold”. This is because the willPositionCollateralBeSufficient validation excludes fees that will be subtracted from the position's collateral as well as negative price impact that can be applied to the position's collateral.

Account for fees and potentially even negative price impact in the willPositionCollateralBeSufficient so that the validation cannot be circumvented in these cases.

GMX Team: Acknowledged.

Orders will remain in the order store when the OracleBlockNumbersAreSmallerThanRequired error occurs during execution. Therefore a malicious user can cause the keeper to continuously expend gas to attempt to execute an order without requiring any additional executionFee.

Consider the following scenario:

• A malicious user frontrun’s the keepers execution tx and updates their order, updating the orderUpdatedAtBlock.
• Now the keeper's tx goes through all of the price setting and order execution logic up to the oracle block number validation.
• Then the order execution tx reverts, the keeper has spent a significant amount of gas but the order still remains, with the same executionFee still attached.
• The malicious user may continue to do this and continue to gas grief the keeper.
• The malicious user can then cancel their order at any time to receive their executionFee back.

The scenario can be exacerbated with an order that requires many prices to be set where the malicious user includes a maximum length swapPath that requires many prices.

Require a non-refundable WNT payment upon order updates to disincentivize such attacks. Otherwise consider validating the oracle block numbers for the order early on in the order execution to minimize the amount of gas that can be wasted.

GMX Team: Acknowledged.

Proof of concept: PoC

Users who have accumulated a large amount of borrowing fees can avoid paying these fees as long as the cumulativeBorrowingFactor has not yet been incremented and skipBorrowingFeeForSmallerSide is enabled.

Consider the following scenario:

• Long OI > Short OI
• Trader A has a large long position that has accumulated a significant amount of borrowing fees. These borrowing fees have not been “solidified” by the update of any other long position. Therefore the cumulativeBorrowingFactor has not yet been incremented to account for Trader A’s accumulated borrowing fees since the last recorded cumulativeBorrowingFactorUpdatedAt for longs.
• Trader A opens another short position shifting the larger side to be the opposite of the first position.
• Trader A then closes the first position and pays 0 borrowing fees.

Do not allow previously accumulated borrowing fees to be skipped in the event that a trader forces a side to have the smaller OI. This can be achieved by updating both the long and short borrowing fees upon position increase and decrease.

GMX Team: The recommendation was implemented.

The price impact amount represented by the executionPrice may not match the price impact amount calculated. This is because the index price used to calculate the price impact amount may differ from the index price used in getExecutionPriceForIncrease.

Consider the following scenario where a trader increases a long position:

Price Impact USD = -$100; Index Price = ($50, $100)

In IncreasePosition:

index price = indexTokenPrice.min = $50
price impact amount = -$100 / $50 = -2 tokens

In BaseOrderUtils:

index price = indexTokenPrice.pickPriceForPnl(isLong, true) = $100
price impact amount = -$100 / $100 = -1 tokens

The discrepancy is because $100 is used for the index price in getExecutionPriceForIncrease rather than $50. As a result, the executionPrice doesn’t reflect the price impact amount which is added to the baseSizeDeltaInTokens.

executionPrice can simply be calculated as:

executionPrice = params.order.sizeDeltaUsd() / cache.sizeDeltaInTokens

to reflect the price impact amount used and then emitted in the event.

Proof of concept: PoC

When depositing, swap impact is calculated for the longTokenUsd and shortTokenUsd being deposited.

However in the case of markets where longToken == shortToken, all the value being deposited will be in the longTokenUsd.

The market will always be considered balanced to begin with since the poolAmount is simply divided by two for both sides. Therefore every deposit will receive negative impact because each deposit is treated as if it is adding all its value to the long side and therefore unbalancing the pool.

Skip price impact calculations when depositing into markets where longToken == shortToken.

GMX Team: The price impact factors for homogenous markets should always be set to 0.

During an insolvent close, the priceImpactDiffUsd is paid at a higher priority than the negative price impact.

This means there are scenarios where the position is liquidated or ADL'd and the account is credited with claimable tokens for price impact that was capped, meanwhile the base price impact, that was the uncapped portion, goes unpaid.

Ultimately this benefits the user and hurts the protocol because these funds will become claimable for the user rather than going towards the positionImpactPool and poolAmount to cover as much of the price impact amount as possible.

Pay the priceImpactUsd before the priceImpactDiffUsd.

GMX Team: The recommendation was implemented.

When a decrease order contains a decreasePositionSwapType other than NoSwap it will execute an additional swap in the current market. However this additional swap is not accounted for in the estimateExecuteDecreaseOrderGasLimit gas estimation.

Therefore these orders will consume gas for an extra swap that is not accounted for in the estimated gas cost.

Additionally if this extra swap were to be accounted for by default in the base decreaseOrderGasLimit, it would be requiring users with a NoSwap to put down more initial executionFee than necessary.

Account for an additional gasPerSwap for decrease orders that have a decreasePositionSwapType other than NoSwap.

GMX Team: The recommendation was implemented.

In the usdToMarketTokenAmount function when the supply of market tokens is 0 and the poolValue is nonzero, the resulting market token amount is the poolValue + usdValue.

However in the _executeDeposit function, the marketTokensSupply and poolValue variables are cached and passed to the usdToMarketTokenAmount function twice in a row when there is positive impact.

When the supply of market tokens is zero and there is some dust leftover in the poolAmount it is possible to be positively impacted and end up with:

2 * poolValue + (positiveImpactAmount.toUint256() * _params.tokenOutPrice.max)
+ (fees.amountAfterFees * params.tokenInPrice.min) market tokens

Thus resulting in a market token amount that double counts the poolValue. The case where this market token amount inflation occurs is rare and there is no immediate financial loss or gain.

However, this market token inflation is unexpected and goes against the documented goal of a 1 USD value per market token in the usdToMarketTokenAmount function. This unexpected and undocumented behavior could be used to exploit systems building on top of GMX V2.

Consider re-calculating the updated poolValue and market token supply after the positive impact application if the initial market token supply was 0. Otherwise document this unexpected behavior.

GMX Team: Price impact is now set to zero if it is positive and there is a market token supply of 0.

The saved callback contract is set anytime an IncreaseOrder is processed, therefore the following unexpected scenario may arise:

1. A trader creates a limit increase order for their position without a callback contract.
2. The trader now sends a market increase order with callback contract A to save contract A.
3. The trader’s limit increase order then executes.
4. The saved Callback contract is now overwritten with address(0), and upon liquidation or ADL, no callback action occurs.

This may cause unexpected results for the trader considering the callback may be performing a useful action such as closing out a hedge/position on another platform. Additionally, the trader might only want the callback to be executed for their increase, and not on any subsequent liquidation or ADL.

Remove the setSavedCallbackContract function from the IncreaseOrder flow so it can be set explicitly with a separate configuration function. Additionally, clearly document that there can only be 1 saved callback contract per market e.g. if a user has two positions long and short in the same market both will use the same callback contract.

GMX Team: The recommendation was implemented.

When the result.poolValue is negative it is impossible to deposit into a market to make it useable again. There can be leftover index tokens in the position impact pool which are subtracted from the pool value. Consequently, it is possible to achieve a state where the supply of market tokens is 0, but the pool value is negative.

Such a scenario would shut down the market, preventing inflow of any deposits and further usage. However, it is important to note that such a scenario would be rare.

Clearly document that such a scenario can occur, and monitor impact factors to help prevent such a situation from arising.

GMX Team: Documentation will be added.

Equities will potentially be supported for trading, as long as they have a price feed. Potential issues arise in the case of forward stock splits, where the price per share is halved and the number of shares a user owns double. This may require an update to the size in tokens for existing positions or bespoke price logic. Other scenarios include reverse stock splits, mergers and acquisitions, etc.

Document protocol behavior in such scenarios and carefully monitor markets where such an event is approaching as they are announced in advance.

GMX Team: Acknowledged.

In the payForCost function, the collateral token is prioritized over the secondary token when making payments. As a result, a user’s collateral could be unexpectedly reduced when their is substantial PnL or price impact in the secondary token available to cover the cost.

This can be especially unexpected for costs such as negative price impact which would commonly be thought of as affecting the execution price and being deducted from the PnL.

Consider prioritizing secondary token amounts over collateral when paying amounts such as negative price impact. Otherwise, document that the user’s collateral will be prioritized over secondary PnL.

GMX Team: Documentation will be added.

In the event that OI is balanced for longs/shorts, shorts will arbitrarily pay longs because of the definition of result.longsPayShorts:

result.longsPayShorts = cache.longOpenInterest > cache.shortOpenInterest

Normally, the fundingUsd would be 0 in this case as the resulting fundingFactorPerSecond is 0 when the OI is balanced. However when there is a stable funding factor configured the fundingFactorPerSecond will be nonzero when the OI is balanced.

Therefore when there is a stable funding factor configured, shorts will arbitrarily pay longs the stable funding factor. This causes an incentive to stop shorting and join the long side to collect funding fees rather than pay them. Even though OI is already balanced, which undermines the point of funding fees.

Skip the update per size delta logic when there is a stable funding factor present and the long open interest is equivalent to the short open interest.

GMX Team: Acknowledged.

When the index token is the same as the PnL token, the positive price impact amount deducted from the position impact pool is maximized (round up division & divide by min price) meanwhile the deduction amount for the pool is minimized (round down division & divide by max price).

Similarly, when the index token is the same as the collateral token, the negative price impact amount added to the position impact pool is minimized (multiplied by the min price and divided by the max price) while the pool amount delta is not.

This creates a tendency for positive price impact to err on the side of increasing the pool value as stated in the comment on line 171. Because of the unequal effects on the impact pool amount and the pool amount there is an immediate non-zero impact on the pool value.

This behavior opens up the possibility for market depositors to manipulate price impact and absorb the position impact pool value. Such a manipulation can be straightforward when the index token is the same as either the collateral token or PnL token.

When the index token is the same as the collateral token or the PnL token consider using the same prices to convert usd values to token values during both positive and negative price impact accounting.

GMX Team: Acknowledged.

The GasUtils.getExecutionGas function sets aside a minHandleErrorGas amount to handle the subsequent error logic. However, this amount does not take into account the configured callback gas limit for an order in the event of a cancellation/freezing.

Include the configured callback gas limit for the order being executed in the minHandleErrorGas result.

GMX Team: The minHandleErrorGas will be adjusted to account for this.

Saved callback contracts will be executed on liquidation and ADL orders which are entirely funded by the keeper/protocol without any remuneration from the user. Additionally, the saved callback contract will be given the maximum callback gas limit upon liquidation or ADL.

This way malicious traders may grief the keeper by creating numerous positions that become liquidatable simply to force the keeper to expend the maximum callback gas limit. Notice that a malicious trader may also be able to directly extract value from the keeper with gas tokens upon a liquidation or ADL callback since this execution gas is subsidized by the protocol.

Be aware of the potential for significant uncovered gas expenditure. Consider implementing a mechanism to remunerate the keeper for excessive callback gas expenditure from the user’s remaining collateral in the case of liquidation and ADL orders.

GMX Team: Keeper remuneration will be implemented in a future iteration.

In the event that a configured Chainlink price feed is outdated the order execution transaction will not occur due to a PriceFeedNotUpdated revert.

A malicious trader may observe that the price feed is outdated and submit a market order that includes a token requiring that price feed. The trader’s order will not be executed as the price feed is outdated.

The trader can then observe that the chainlink prices have been updated with a transmit transaction and choose to cancel their order if price has not moved favorably in the time that the price feed was outdated.

Carefully monitor outdated Chainlink price feeds and consider implementing logic to freeze/cancel orders that cannot be executed.

GMX Team: Acknowledged.

In the claimCollateral function the claimableFactor is the maximum of the claimableFactorForTime and claimableFactorForAccount. Therefore in the case where capped negative price impact is manipulated and claimable collateral ought to be used at the protocol's discretion to punish the manipulator, the controls will be insufficient.

Consider the following:

• A malicious trader manipulates reference prices to take advantage of negative PI capping.
• Many other traders are capped in the same timekey as this one malicious trader.
• The other traders ought to be able to claim their collateral at a later date, but the malicious trader should never have their claimableFactor updated.

The current controls are not well suited for this scenario since every non-malicious trader would have to have a manual per-account claimableFactorForAccount configured. In the case where there are many innocent traders in this timekey this may be impractical, especially in times of market volatility. The controls should be flipped such that the single malicious trader can be punished with a more constrictive claimableFactorForAccount.

Alter the claimableFactorForAccount logic such that it is a more constrictive threshold rather than a less constrictive one.

This would likely be accompanied by a isClaimableFactorForAccountEnabled boolean value to indicate whether the claimableFactorForAccount ought to be used in the case that it is the default value of zero.

GMX Team: If others traded during the manipulation it may make sense to cap their pnl as well.

There is no lower bound on how negative price impact can be during swaps or position orders due to the virtual inventory. In virtual inventories where there are many markets, it is possible to build up large imbalances over time as users may be incentivized with positive impact from their direct pool to make deposits or orders that ultimately increase the disparity in the virtual inventory.

The resulting extreme disparity in the virtual inventory will lead to significant negative impact without bound for unsuspecting users. The most likely outcome is that deposits and orders that would compute price impact from the virtual inventory will simply be cancelled due to their acceptable price or minimum amounts being unfulfillable.

Consider implementing a lower bound on the magnitude of negative price impact that can be applied from the virtual inventory. Alternatively, consider creating separate price impact factors that can apply specifically to the virtual inventory calculations as the virtual inventory imbalances can be significantly larger than normal markets.

GMX Team: Will implement capping of negative price impact in a future iteration if needed.

In decreasePosition there exists a cache.collateralTokenPrice which is stored at the beginning of the function execution on line 73.

However this price is retrieved for a second time with getCachedTokenPrice on line 148.

Reuse the cache.collateralTokenPrice.

GMX Team: The recommendation was implemented.

The comment if there is a stable funding factor then used that instead of the open interest misspells “use” as “used”.

Replace “used” with “use”.

GMX Team: The recommendation was implemented.

The same if condition relying on result.longsPayShorts is repeated back to back. The logic in both can be deduplicated into a single if condition.

Consolidate the contents of each if (result.longsPayShorts) condition into a single if case.

GMX Team: Acknowledged.

When emitting the emitInsufficientFundingFeePayment event, it may be helpful to include the amountPaidInSecondaryOutputToken as this is useful when determining how much collateral token needs to be deposited from an insurance fund or other into a market.

Consider adding the amountPaidInSecondaryOutputToken as a piece of data emitted with the emitInsufficientFundingFeePayment function call.

GMX Team: The recommendation was implemented.

In the payForCost function, if the collateral token amounts are insufficient to pay for the cost, the secondary price is fetched to convert the remaining cost into a secondary token amount.

However the secondary token price, which is always the pnl token price, is re-fetched upon every payForCost call.

Meanwhile there is an existing cache.pnlTokenPrice on the PositionUtils.DecreasePositionCache memory cache parameter.

Accept the pnlTokenPrice as an argument to the payForCost function and pass the cache.pnlTokenPrice as the value upon each payForCost call.

GMX Team: Since it is reading from memory, it should not have a large effect on gas usage.

The comment on line 485 notes that “the priceImpactDiffUsd has been deducted from the output amount or the position's collateral”, but in fact the priceImpactDiffUsd can be deducted from the secondaryOutputAmount in a subset of cases.

Additionally, there are several cases in which the priceImpactDiffUsd is deducted from a subset/combination of the three.

In cases where the priceImpactDiffUsd is deducted in any part from either the outputAmount or the secondaryOutputAmount subtracting the full priceImpactDiffAmount from the initialCollateralDeltaAmount does not accurately reflect the portion of the priceImpactDiffUsd that was deducted directly from the collateral. Therefore this adjustment does not make the effect on the collateral amount predictable.

Consider adjusting the initialCollateralDeltaAmount by the exact amount deducted from the collateral (or even just the amount paid in the collateral token to be somewhat accurate, while maintaining simplicity) when paying for the priceImpactDiffUsd on lines 388-428.

GMX Team: Comment added.

Throughout the codebase, there are instances of EventData being declared and being returned empty.

For example, IncreaseOrderUtils.processOrder returns empty event data. In comparison DecreaseOrderUtils.processOrder and SwapOrderUtils.processOrder return populated event data.

Ensure that event data is populated where necessary.

GMX Team: The eventData is intentionally empty.

The comment on line 101 states that “then priceImpactUsd would be $20”, however this should refer to the priceImpactDiffUsd instead.

Replace priceImpactUsd with priceImpactDiffUsd in the comment.

GMX Team: The recommendation was implemented.

Since the base PnL is calculated, now independent of price impact, the getPositionPnlUsd function no longer needs to use a separate index token price to compute the poolPnl for capping calculations.

The executionPrice can be renamed to the indexTokenPrice and this price can be used for all calculations.

GMX Team: The recommendation was implemented.

The if (cache.sizeDeltaInTokens < 0) case will never be satisfied as the cache.sizeDeltaInTokens is a uint256 value.

Remove this superfluous if case.

GMX Team: The cache.sizeDeltaInTokens was made an int.

When users are depositing, the tokens in the poolAmount during the getPoolValueInfo function are valued at the maximum price, meanwhile the user’s deposits are valued at the minimum price.

When users are withdrawing, the tokens in the poolAmount during the getPoolValueInfo function are valued at the minimum price, meanwhile the longTokenPoolUsd, shortTokenPoolUsd and totalPoolUsd are valued at the maximum price.

Therefore users are negatively impacted by large spreads when depositing and withdrawing from a market.

Additionally, it is noteworthy that this will in some cases leave behind a non-trivial amount of tokens in the poolAmount after all depositors have withdrawn.

It should be well documented that depositors and withdrawers are negatively impacted by the price spread.

GMX Team: Documentation will be added.

Several functions throughout the codebase are missing NatSpec documentation:

• getExecutionPrice
• payForCost
• handleEarlyReturn
• getEmptyFees

Add the relevant documentation to functions where NatSpec is lacking.

GMX Team: Will fix NatSpec in a future iteration.

In the event where a stable funding factor is set or unset it may cause positions to unexpectedly become liquidatable due to a stepwise increase/decrease in the factor and the resulting funding fees calculated.

Document that positions may unexpectedly experience an increase/decrease in funding fees due to the modification of the stable funding factor.

GMX Team: Will add to docs.

Modulo 2 is used to determine if the length of the arr array is odd, however & 1 is a more efficient alternative.

Modify the check from arr.length % 2 == 1 to arr.length & 1 == 1.

GMX Team: Acknowledged.

Currently there is no additional fee that a trader incurs when they are liquidated. The trader is simply forced to settle their fees/losses. This way a liquidation is roughly equivalent to a stop loss.

This may allow traders to open high leverage positions and gain an advantage from the fact that they will simply be “stopped out” when they are liquidatable.

Additionally traders may gain an advantage by shifting the monetary impact of an insolvent close onto the protocol in the event that price gaps significantly and a high leverage position is immediately under water. Traders are not negatively impacted by liquidations so they are more likely to take on these positions and push these losses onto the pool depositors in the case of insolvent closes.

Consider adding a liquidation fee to make liquidations sufficiently unattractive for traders. The fee may be allocated to the pool to offset the potential financial impact of insolvent closes.

The liquidation fee may be configured to 0 in the vast majority of cases. However it may prove useful in the event of high leverage position manipulations.

GMX Team: A liquidation fee will be added in a future iteration if required.

During execution of increase orders, a swap is performed to receive the position’s collateral, shifting the pool amounts. This swap is performed before the borrowing factor is updated with updateFundingAndBorrowingState which will rely on the balance of backing tokens in the pool to compute the current borrowing fees.

A trader can use the collateral token swap during increase to manipulate the balance of backing tokens and therefore minimize the borrowing fees that are recorded for their position.

Monitor the swap fee to dissuade any borrowing fee manipulation and document such behavior.

GMX Team: Acknowledged.

Throughout the codebase, the .sol files use a floating pragma with ^0.8.0. There is a list of known bugs in many 0.8 Solidity versions that were fixed in subsequent releases:

https://github.com/ethereum/solidity/blob/develop/docs/bugs.json

Furthermore, if compiled with 0.8.20 there may be unexpected reverts when deployed as many chains still do not support the PUSH0 opcode.

Use a static pragma.

GMX Team: The Solidity version will be set in the hardhat config instead..

During the increase position logic, the funding perSize values are stamped on a new position with zero size. However, the entire if case and perSize initialization is unnecessary as the funding fees and claimable amounts will be calculated by multiplying the position size with the diffFactor.

Initially the position size will be zero, resulting in 0 funding fees and claimable amounts, even with a non-zero diffFactor. The funding fees perSize values will then be set for the position later on (lines 170-172). Therefore the initial stamping of the funding perSize values for a brand new position are unnecessary

Remove the if case where a new position with 0 sizeInUsd has the perSize values stamped.

GMX Team: Acknowledged.

At the end of a swap, the priceImpactUsd is emitted with the emitSwapInfo function.

However the priceImpactUsd value may not be accurate to how much price impact was actually applied to the swap, depending on if the impact amount was capped by the size of the swap impact pool or not.

Either modify the value passed to the event or add a parameter for priceImpactAmount.

GMX Team: The recommendation was implemented.

According to the README, the formula for price impact calculations should be:

(initial imbalance) ^ (price impact exponent) * (price impact factor / 2) - (next imbalance) ^ (price impact exponent) * (price impact factor / 2)

However with latest changes to PricingUtils.applyImpactFactor(), the / 2 division was removed from the formula.

Update the formula in the documentation.

GMX Team: The documentation was updated.

The validateSwapPath function does not include validation for homogenous markets, which would cause the swap to fail automatically upon execution.

Similarly, there is no validation for duplicate markets in the provided swapPath until the swap is attempted during execution.

In the validateSwapPath function, verify that there are not any single token markets or duplicate markets in the swapPath.

GMX Team: Now single token markets are validated, duplicate market validation may be too gas intensive.

The current value of the uintValues[key] is cached as the uint256 currValue stack variable. However, the currValue variable is only referenced once on the very next line and so therefore can be replaced with a direct access of the mapping.

Access the mapping directly when calculating nextUint rather than storing the currValue.

GMX Team: Acknowledged.

Throughout the ExchangeRouter several claimedAmounts lists are declared of with a length that is subsequently looped over. However this length is recomputed for both the list declaration as well as the for loop. This length can be cached to save gas on both the list declaration and upon each iteration of the for loop.

Cache the array length and use this cached value in the for loop and array declaration.

GMX Team: Acknowledged.

Users can submit valid swap orders that have an empty swapPath.

Validate that the swapPath length is nonzero for swap orders upon order creation.

GMX Team: Acknowledged.

In the get, areEqualTo, areGreaterThan, areGreaterThanOrEqualTo, areLessThan, areLessThanOrEqualTo functions, the length of the arr array is not cached during the for loop execution.

Additionally, the increment of i can be replaced with an unchecked block wrapping ++i.

Implement the recommended optimizations.

GMX Team: Acknowledged.

Price impact for deposits is calculated based on the pool amount imbalance from the deposited amounts before fees are accounted for.

However price impact for swaps is calculated after fees are taken from the swapped in amount.

The known issues in README.md mention that Calculation of price impact values do not account for fees, however this is directly in contradiction to the price impact calculations for swaps.

Consider standardizing on one or the other for parity across deposits and swaps or document this key difference.

GMX Team: Calculation of price impact for swaps now excludes fees as well.

When computing and applying the price impact amounts in both deposits and swaps, the else case handles instances where there is $0 of price impact.

However, the accounting logic and corresponding event emissions are unnecessary when the priceImpactUsd is computed to be 0.

The priceImpactUsd will often be zero when depositing balanced long and short token amounts, depositing into homogenous markets, or making swaps that imbalance the pool as much as it is balanced.

Exclude cases where the priceImpactUsd is 0 from the else branch to avoid superfluous logic.

GMX Team: Can be optimized in a future iteration.

The willPositionCollateralBeSufficient function is invoked in IncreasePositionUtils as well as DecreasePositionUtils where there is a cache.collateralTokenPrice available in both cases.

However the willPositionCollateralBeSufficient function redundantly fetches the collateral token price with getCachedTokenPrice.

Accept the collateralTokenPrice as a parameter for the willPositionCollateralBeSufficient function to avoid fetching it again.

GMX Team: Acknowledged.