Dynamic Funding Fees and Position Impact

Proof of concept: PoC

When computing the sign of the resulting bounded value, the original value is divided by the magnitude.

However the magnitude has already been adjusted to be within the min or max bounds, therefore when the magnitude has been adjusted the resulting sign variable value is no longer a unit vector indicating sign.

This results in the bounded value, in this case the nextSavedFundingFactorPerSecond, being in fact unbounded.

For example:

• value = 800
• Min = 250
• Max = 400
• Magnitude is capped to 400
• Sign = 800 / 400 = 2

The returned value is 400 * 800 / 400 = 800 which is outside of the defined max As a result, dynamic funding fees cannot be capped, leading to any market being entirely bricked within hours of the dynamic funding fees being activated.

If the value is < 0 return -magnitude.toInt256() and if the value is >=0 return magnitude.toInt256()

Proof of concept: Video

The adaptive funding rate mechanism is prone to strategic gaming as the resulting fundingFactorPerSecond that a market experiences is affected by the amount of time that passes before the funding fees are updated. There are several ways the funding fees may be gamed as a result.

Firstly, consider a trader, Bob, who holds a short position in the following scenario:

• Longs currently pay shorts, long OI > short OI, savedFundingFactorPerSecond > 0.
• Long OI becomes < short OI and the OI diff is within the NoChange range.
• The fundingFactorPerSecond will begin decreasing, as the skew has changed sides.
• As soon as the fundingFactorPerSecond crosses the threshold from positive to negative and becomes -1, Bob updates his order by removing 1 wei of collateral.
• Now the fundingFactorPerSecond sticks to that minimum magnitude rate, dependent on when Bob updated his position.

The terminal fundingFactorPerSecond that the market ends up experiencing is dependent on when the funding fees are updated, therefore Bob updates the funding fees at a time that is most advantageous to him and results in minimal funding paid.

Secondly, consider the following occurs from where we left off with Bob:

• Alice is a long trader, she notices that the funding rate was stopped at the minimum magnitude, so she attempts to increase it.
• Alice opens another temporary long position just large enough to flip the skew back towards longs > shorts, as soon as the fundingFactorPerSecond crosses the threshold and becomes 1, Alice closes her temporary long position.
• Now the fundingFactorPerSecond begins decreasing again. Alice is hoping that Bob, or anyone else, will not update the funding — ideally until the fundingFactorPerSecond reaches the maximum magnitude.

In a perfectly competitive GMX V2 market, traders on both the long and short side would out-maneuver each other such that the minimum amount of funding fees is paid to the other side at all times. In reality, some traders will abuse these behaviors against uninformed traders to gain an unfair advantage and benefit from substantially less funding fees paid and substantially more funding fees collected.

Consider assigning a fundingFactoPerSecond that is halfway between the min and max magnitude upon switching skews and allowing that middle value to remain, increase, or decrease based on the OI diff.

Additionally, Instead of allowing minute changes to the open interest (e.g. 1 wei) to impact the rate, consider setting a threshold for OI changes to adjust the funding rate. Furthermore, carefully monitor the market for funding fee manipulation and adjust funding factors appropriately.

In the boundMagnitude function, when the provided value is 0 the provided min fails to give a lower bound to the resulting value.

• value = 0, min = 10
• sign = value / magnitude = 0 / 10
• Returned value = magnitude * sign = 10 * 0 = 0

When the value is 0 return the min with a sign informed by a boolean parameter. When the FundingRateChangeType is Increase, the sign of the resulting min value should be whichever direction the increase is heading in.

The funding factor per second returned from the getNextFundingFactorPerSecond function may be unable to cross from negative to positive or from positive to negative in the event that the minFundingFactorPerSecond is set > 0 and orders are executed often.

• Original savedFundingFactorPerSecond = 10
• minFundingFactorPerSecond = 10
• nextSavedFundingFactorPerSecond = 7
• Bounded nextSavedFundingFactorPerSecond = 10

The nextSavedFundingFactorPerSecond cannot flip signs and continue to make progress to reach the funding factor it ought to be on the other side unless it can make a large enough jump to cross the gap from [0, minFundingFactorPerSecond], which may be unlikely if orders are consistently updating the secondsSinceFundingUpdated.

Additionally, if the gap from [0, minFundingFactorPerSecond] is crossed successfully, the resulting nextSavedFundingFactorPerSecond will be increased in magnitude to the minFundingFactorPerSecond, resulting as -minFundingFactorPerSecond. This jump will go above the configured fundingIncreaseFactorPerSecond rate and may cause unexpected results.

Be wary when configuring the minFundingFactorPerSecond to be != 0, if the minFundingFactor is ever != 0 it should have a minimal magnitude to limit these behaviors.

The nextSavedFundingFactorPerSecond can be decreased to 0 in the event that the cache.savedFundingFactorPerSecondMagnitude == decreaseValue. This can result in the funding side increasing rather than decreasing since it isn’t set to 1 or -1:

• Long OI < short OI
• Original nextSavedFundingFactorPerSecond = -10
• Original longsPayShorts = false
• decreaseValue = 10
• nextSavedFundingFactorPerSecond = 0
• Long OI < Short OI
• nextSavedFundingFactorPerSecond = 0
• isSkewTheSameDirectionAsFunding = false
• increaseValue = 13
• nextSavedFundingFactorPerSecond = -13

Therefore the fundingFactorPerSecond was meant to decrease in magnitude but increased instead.

Avoid this cycling by assigning the nextSavedFundingFactorPerSecond to 1 or -1 in the if case on line 1316 by changing the cache.savedFundingFactorPerSecondMagnitude < decreaseValue to cache.savedFundingFactorPerSecondMagnitude <= decreaseValue.

There is no validation that the THRESHOLD_FOR_DECREASE_FUNDING is less than THRESHOLD_FOR_STABLE_FUNDING. If the config keeper were to invert the thresholds, the funding factor may be decreasing when it should be stable or increasing when it should be decreasing.

Furthermore, there is no validation that the MIN_FUNDING_FACTOR_PER_SECOND is less than the MAX_FUNDING_FACTOR_PER_SECOND. As a result, boundMagnitude will bound the value incorrectly.

Add validation in the Config to ensure THRESHOLD_FOR_DECREASE_FUNDING is less than THRESHOLD_FOR_STABLE_FUNDING.

Add validation in the Config to ensure MIN_FUNDING_FACTOR_PER_SECOND is less than MAX_FUNDING_FACTOR_PER_SECOND. A check for the min and max bounds may be included in boundMagnitude as well or the documentation should mention that the validation is done elsewhere.

tx.gasprice != 0 is not a bulletproof means of filtering out non-estimateGas calls. The Keeper can assign a non-zero gasPrice and there has been discussion about getting the tx.gasprice to be the basefee even when the gasPrice = 0 in an estimateGas call.

Use tx.origin as no one can transact from the zero address.

A keeper may have to use prices from several blocks ago to execute an order, regardless of if realtime feeds or the default oracle system is being used. In such a case, the validateRefPrice would be comparing the latest Chainlink aggregator oracle price against an earlier price.

Depending on how large the MAX_ORACLE_REF_PRICE_DEVIATION_FACTOR is and how volatile the asset, the execution with these earlier block numbers/prices may revert, preventing them from being used.

Carefully assign the MAX_ORACLE_REF_PRICE_DEVIATION_FACTOR considering that it might be necessary in some cases to allow prices from blocks previous to when the latestAnswer in the Chainlink aggregator was updated.

Based on the startingGas and the estimatedGasLimit, the execution gas is validated to ensure the startingGas is greater than the estimatedGasLimit and some variable, additional gas for execution. After calling GasUtils.validateExecutionGas(dataStore, startingGas, estimatedGasLimit), the gas for execution is presumed to be enough for execution of the order as it has been validated.

However, uint256 executionGas = GasUtils.getExecutionGas(dataStore, startingGas) is called right afterwards which reduces the startingGas by the gas needed for error handling. The amount of gas validated for execution is different than the amount given for execution, which may now be insufficient.

Consider making the gas validation more restrictive by calling validateExecutionGas() on the result of getExecutionGas() or ensure the the minAdditionalGasForExecution is large enough to cover the gas forwarded to handle the execution error.

Proof of concept: PoC

It is possible for shorts to get paid when longs should pay shorts (long OI > short OI) and vice versa.

This is because it may take a certain duration until the savedFundingFactorPerSecond flips payment sides and accurately represents payment direction. Ultimately this functionality misaligns incentives during this period, where traders who imbalance the pool aren’t discouraged by funding fee payments.

Consider resetting the fundingFactorPerSecond entirely when the OI imbalance direction changes.

Because the increaseValue is dependent on durationInSeconds, when a period of time goes by without updates the cache.nextSavedFundingFactorPerSecond can spike to the maximum bound. This can occur when the skew changes, causing an increase to a large fundingFactoPerSecond regardless of the new OI diff.

The situation also may arise if the savedFundingFactorPerSecond is at 0 and the duration is large, regardless of whether the previous fundingRateChangeType was an increase or no change.

It's crucial to track the min/max limits and make adjustments as needed. Additionally, consider refactoring the funding such that these spikes do not occur when the skew is switching sides or the savedFundingFactorPerSecond is 0.

Proof of concept: PoC

Traders don't experience an incremental increase in the fundingFactorPerSecond. Instead, they receive funding based on the final funding factor applicable for the entire duration of their position. Consequently, a user who updates their position after X seconds will receive funding fees based on the rate at that specific moment.

This could lead to a scenario where updating a position at the end of X seconds may yield more in funding fees than updating midway at X/2 and closing out another X/2 later, even though the total duration is the same.

Clearly document this behavior so that traders are aware how frequent updates can affect funding fee payments.

Proof of concept: PoC

The claimableFundingFeeAmount is asserted to be less than the balance of the MarketToken contract at all times, however this may not always be the case and can lead to the halting of liquidations, orders, ADLs, and withdrawals as a result.

Firstly, users may decrease the collateral backing their position to 0 given that the willPositionCollateralBeSufficient validation ignores fees and price impact. All funding fees accumulated by positions with 0 collateral will not have corresponding ERC20 tokens backing that funding fee amount. Therefore the funding fees will grow without a corresponding backing value in the balance of the MarketToken.

A malicious actor may then swap all (or most) of the backing poolAmount to the other token in the market. This way the attacker can reduce the amount of ERC20 tokens that can count towards the balance in the validateMarketTokenBalance validation to just over the claimableFundingFeeAmount. The funding fees will then continue to increase and surpass the balance of the contract. Thereby causing orders for positions that would collect these funding fees as a claimableFundingFeeAmount to revert.

This exploit is unlikely as it requires a market with low open interest and would require time to execute as well as favorable funding rates. Additionally it could be resolved by manually plugging the hole by sending an amount of ERC20 tokens directly to the MarketToken. However it could be leveraged by an attacker for a short period of time to cause grief to other users in the same market before tokens are swapped back.

Consider removing the claimableFundingFeeAmount validation in the validateMarketTokenBalance function entirely. Otherwise carefully monitor for any such manipulation.

If a position is deemed liquidatable before validating against info.minCollateralUsdForLeverage, then any LiquidatablePosition events will emit 0 as the minCollateralUsdForLeverage since info.minCollateralUsdForLeverage has yet to be set.

Document such behavior or calculate the minCollateralUsdForLeverage if needed.

In the getNextFundingFactorPerSecond function, when the fundingIncreaseFactorPerSecond is not configured the savedFundingFactorPerSecond is assigned to 0.

However for markets where the default funding is used before activating the dynamic funding it may serve as a smoother transition to assign the savedFundingFactorPerSecond to the current resulting fundingFactorPerSecond.

Provide the resulting fundingFactorPerSecond as the returned nextSavedFundingFactorPerSecond value on line 1268.

In the getNextFundingFactorPerSecond function, only configCache.fundingIncreaseFactorPerSecond is validated against in if (configCache.fundingIncreaseFactorPerSecond == 0).

It is possible for the fundingIncreaseFactorPerSecond to be non-zero, but the fundingDecreaseFactorPerSecond to be zero. As a result, the decreaseValue would be be zero and the saved funding factor would not be adjusted -- mimicking a stable behavior when it should really decrease.

This could return a different result than simply calculating the funding factor per second using Precision.applyFactor(cache.diffUsdToOpenInterestFactor, cache.fundingFactor).

Validate the funding factor depending on the adjustment direction and/or document this behavior.

In the distributePositionImpactPool function the distributionAmount is emitted with the “PositionImpactPoolDistributed” event.

However it may be helpful for consumers of the “PositionImpactPoolDistributed” event to have access to the resulting value of the position impact pool as well.

Consider adding the nextPositionImpactPoolAmount to the information emitted with the “PositionImpactPoolDistributed” event.

The current NatSpec does not include the new Order.Props memory order parameter.

Update the documentation.

During cancellation, the Order/Deposit/Withdrawal is read from the store just to validate the request cancellation, and then the appropriate cancel function is called in a Utils library. In Utils, the Order/Deposit/Withdrawal is read once again from the store.

Consider passing the objects as parameter which matches the updates in the rest of the codebase.

Positive position impact is a key incentive for actors to rebalance the long and short open interest of a market. However positive position impact is capped by the amount of virtual index tokens in the position impact pool.

When the position impact pool is distributed, the maximum net cap for positive impact is reduced. If the minPositionImpactPoolAmount is not configured high enough it could lead to lacking incentives for users to rebalance the open interest.

Ensure that the minPositionImpactPoolAmount is configured to a reasonable amount, allowing for sufficient incentivization for aeros to balance the open interest.

In the validateExecutionErrorGas function, the revert reasonBytes is required to be of length 0 to initiate the validation. However the length of the reasonBytes may be 0 in many circumstances other than an out of gas error. For example if an empty revert() or require() is used without a revert string, or another type of error such as an INVALID opcode.

Be aware that the reasonBytes.length == 0 check does not ensure that the execution reverted with an out of gas error and monitor for any potential manipulations because of this assumption.

The minMarketTokensForFirstDeposit function does not follow the established naming convention for functions used to retrieve keys. The expected name for this function would be minMarketTokensForFirstDepositKey.

Rename the minMarketTokensForFirstDeposit function to minMarketTokensForFirstDepositKey.