Pyth Updates

In the _setPricesWithData function the _setLastUpdatedValues call assigns the latest block details as the last update which is used to determine the staleness of a price.

However the Pyth price can be from up to 60 seconds before the current block, based on Pyths default configured staleness check. Therefore the maxPriceUpdateDelay and priceDuration validations will be off by up to 60 seconds.

Consider adjusting the lastUpdatedAt to keep track of the publish time for each token’s price from Pyth. Otherwise adjust the maxPriceUpdateDelay and priceDuration to account for this potential 60 second lag time.

Pyth prices offer a confidence interval range for every price, this is because the exact price of an asset cannot be given due to difference across exchanges and reporting sources.

Pyth recommends that the confidence interval be used in two ways, which could be adapted to the use-case for the FastPriceFeed contract:

1. Opt to not use the Pyth price if the confidence range becomes too wide, as this is an indication of uncertain pricing which may lead to profitable arbitrage opportunities.
2. Apply the confidence range returned from Pyth as a spread, moving in the direction that would be disadvantageous for traders.

Consider adopting one or both of these use-cases for the pyth confidence interval to provide protection for uncertain pricing results.

In the getPrice function, the diffBasisPoints are calculated with round down precision loss which leads to hasSpread reporting false when the deviation has in fact exceeded the maximum allowable amount.

For example:

• maxDeviationBasisPoints = 100
• BTC reference price: 64,338
• BTC fastPrice: 64,987
• diffBasisPoints = 64,987 - 64,338 * 10000 / 64,338 = 100.873511766, truncated to 100
• The fastPrice is considered to be within the maxDeviation because a strict greater than comparison is used

As a result prices which are above the maxDeviationBasisPoints can be flagged as within the deviation range and will not have a spread applied.

Use a >= comparison rather than a strict > comparison for the maxDeviationBasisPoints check.

In the getPrice function a spread is used if the fastPrice is deemed to be unsatisfactory. In the event that the favorFastPrice function returns false a spread is applied where the fastPrice can be used if it is the worse price for traders.

However if the fastPrice has been flagged by watchers to be inaccurate or even malicious it should not be used in any capacity for the resulting price, even if this value is not desirable for traders.

Consider handling the disableFastPriceVoteCount spread separately, where the spread does not include the fastPrice. Additionally consider if the maxCumulativeDeltaDiffs spread should use the fastPrice as well.

The FastPriceFeed contract now utilizes the experimental ABIEncoderV2 to support the bytes[] calldata parameter for the _setPricesWithData function. There are several known bugs pertaining to the ABIEncoderV2 in Solidity 0.6.12.

Specifically, AbiReencodingHeadOverflowWithStaticArrayCleanup and ABIDecodeTwoDimensionalArrayMemory documented at https://docs.soliditylang.org/en/v0.8.25/bugs.html affect Solidity version 0.6.12.

Neither of these issues affect the FastPriceFeed contract currently, however future changes to this contract may introduce unexpected vulnerable behavior due to these known issues.

Additionally, there may be further undocumented or undiscovered bugs affecting the ABIEncoderV2 with Solidity version 0.6.12 which could affect the contracts in their present form.

Be aware of these known bugs when making future changes to the and consider upgrading to an up to date version of Solidity, where known ABIEncoderV2 bugs have been addressed.

The FastPriceFeed's getPrice function is called by the VaultPriceFeed's getPriceV2 function. In multiple cases, getPrice may return a price with a spread applied ontop of it. However, the getPriceV2 will apply another _spreadBasisPoints onto the returned price. Consequently, two spreads are applied on the price, exacerbating the impact on user's orders.

Furthermore, if VaultPriceFeed:getPrice is called, it will trigger function getPriceV2 and then apply one more spread — adjustmentBasisPoints. Three spreads are applied to a single price.

Do not apply another spread in the VaultPriceFeed. Otherwise, if intended, clearly document this behavior.

Sequential percent changes cannot be added to see the overall, cumulative price movement. Two 10% price movements is not 20% but 21%.

Consequently, the cumulativeRefDelta is less than the true move, leading to potentially not favoring the fast price, inaccurate event emission, and systems that rely on that data.

To get the true measure of price movement, multiply the percent changes.

Using block.number on Arbitrum returns the L1 Mainnet block close to when the sequencer received the transaction. This may unnecessarily prevent updates with the revert "FastPriceFeed: minBlockInterval not yet passed".

On Arbitrum, utilize ArbSys to fetch the Arbitrum block number.

In the FastPriceFeed contract the BITMASK_32 variable is unused.

Remove the BITMASK_32 variable.

In the _setPricesWithData function the pythPrice.publishTime validation uses a strict greater than comparison with the block.timestamp - _priceDuration to determine if a price is stale.

However because a strict greater than is used the full _priceDuration is not available for prices retrieved from pyth.

Consider using a >= comparison to allow the final second in the fresh range to be used.

In the PositionRouter Interface the first parameter for the executeIncreasePositions function is defined as the _count, however in the implementation for the executeIncreasePositions function this parameter is named _endIndex.

Consider standardizing on one parameter name across the interface and function implementation.

In the FastPriceFeed contract there are lacking validations for the trusted setter functions, therefore if trusted addresses are compromised they can assign values which would cause stale prices or even DoS the exchange.

For instance, if the maxPriceUpdateDelay is assigned to 0 and the spreadBasisPointsIfChainError is assigned to the max uint256, calls to the getPrice function will revert and DoS all interactions with the GMX vault.

Consider adding validations that the values assigned in the trusted setter functions are within an expected range.

The lastUpdatedAt can only be updated every minBlockInterval. If maxPriceUpdateDelay is less than time passes within the minBlockInterval, function getPrice will always return the price with spreadBasisPointsIfChainError added.

Ensure maxPriceUpdateDelay is larger than the time passed within the minBlockInterval.

The length of _tokens is not validated against the length of _maxCumulativeDeltaDiffs in setMaxCumulativeDeltaDiffs.

This may lead to unintended consequences such as a token being set with the wrong max delta diff.

Consider validating that the two arrays are the same length.