Liquidity Management
Gamma
DeFiFoundry
50,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Precision Mismatch Between KeeperProxy and GMX OrderHandler Leads to Incorrect Position Sizing and Leverage

krisrenzo

Summary

The PerpetualVault contract assumes a fixed (30) decimal precision for prices when calculating position sizes, while GMX's OrderHandler uses dynamic price decimals. This mismatch can result in incorrect position sizing, potentially leading to reverts, over-leveraged positions or under-utilized capital.

Vulnerability Details

The vulnerability stems from an architectural assumption in the position size calculation logic. When creating increase position orders, the contract calculates sizeDelta using:

uint256 sizeDelta = prices.shortTokenPrice.max * amountIn * leverage / BASIS_POINTS_DIVISOR;

This calculation assumes prices are always in a fixed decimals (1e30) as enforced by the KeeperProxy contract.

function _check(address token, uint256 price) internal view {
// https://github.com/code-423n4/2021-06-tracer-findings/issues/145
(, int chainLinkPrice, , uint256 updatedAt, ) = AggregatorV2V3Interface(dataFeed[token]).latestRoundData();
require(updatedAt > block.timestamp - maxTimeWindow[token], "stale price feed");
@> uint256 decimals = 30 - IERC20Meta(token).decimals();
price = price / 10 ** (decimals - 8); // Chainlink price decimals is always 8.
require(
_absDiff(price, chainLinkPrice.toUint256()) * BPS / chainLinkPrice.toUint256() < priceDiffThreshold[token],
"price offset too big"
);
}

However, GMX's OrderHandler accepts prices with variable decimal precision through its price setting functions:

GMX sets the price for its token using the Oracle::setPrices, Oracle::setPrimaryPrice or Oracle::setPricesForAtomicAction in orderHandler (0xe68CAAACdf6439628DFD2fe624847602991A31eB).

See setPrices():

function setPrices(
OracleUtils.SetPricesParams memory params
) external onlyController {
OracleUtils.ValidatedPrice[] memory prices = _validatePrices(params, false);
​
_setPrices(prices);
}
​

See setPrimaryPrice():

function setPrimaryPrice(address token, Price.Props memory price) external onlyController {
_setPrimaryPrice(token, price);
}

The Contract validates these set prices using _validatePrices.
The key thing to notice in _validatePrices is no specific decimal precision is enforced for the prices.
On the other hand, the KeeperProxy enforces a price decimal of 30. We know the price is always in 30 decimals because of the validation in KeeperProxy::_check
Note that there's no place where the GMX documentation order code explicitly state or enforce that it's prices will be in 30 decimal precisions

The Issue

When IncreasePositionUtil::increasePosition --> PositionUtils::getExecutionPriceForIncrease is called during order execution, the returned values ((cache.priceImpactUsd, cache.priceImpactAmount, cache.sizeDeltaInTokens, cache.executionPrice)) will not match the expected values used for creating our order request, if GMX price precision is not 30.

One of the following will happen :

  • If price decimal of KeeperProxy is greater than the price decimal of GMX OrderHandler, our sizeDeltaUsd will be exaggerated, and our order will either revert due to insufficient collateral or open an over leveraged position exposing the vault to higher risk than intended.

  • If price decimal of KeeperProxy is less than the price decimal of GMX OrderHandler, our sizeDeltaUsd will be less than intended, and we will end up with a position with a leverage lower than intended, effectively limiting the vaults upside

Proof of Concept

Consider the following scenario:

  1. KeeperProxy sends price with 30 decimals: 1 ETH = 2000 * 1e30

  2. GMX OrderHandler has price with 8 decimals: 1 ETH = 2000 * 1e8

  3. User deposits 1 ETH with 2x leverage

  4. PerpetualVault calculates sizeDelta:

sizeDelta = (2000 * 1e30) * 1e18 * 20000 / 10000 = 4000 * 1e45

  1. When executed by GMX:

    • Expected: 2000)

    • Actual: Much larger position due to decimal mismatch

Impact

The impact depends on the relative decimal precision between KeeperProxy and GMX.
If KeeperProxy has more decimals than GMX, the position size becomes larger than intended, leading to higher leverage and increased liquidation risk. Additionally, orders may revert due to insufficient collateral.
If KeeperProxy has fewer decimals than GMX, the position size is smaller than expected, resulting in lower leverage, reduced capital efficiency, and lower potential returns.

Tools Used

Manual Review

Recommendations

Dynamically sync price decimals with GMX prices.

function _createIncreasePosition(
bool _isLong,
uint256 acceptablePrice,
MarketPrices memory prices
) internal {
// Get GMX price decimals
uint256 gmxDecimals = vaultReader.getPriceDecimals(market);
// Adjust price to match GMX decimals
uint256 adjustedPrice = prices.shortTokenPrice.max * 10**gmxDecimals / 1e30;
// Calculate size with matched decimals
uint256 sizeDelta = adjustedPrice * amountIn * leverage / BASIS_POINTS_DIVISOR;
...
}

Also, consider implementing a price normalization layer that handles decimal conversions between systems.

Updates

Lead Judging Commences

n0kto Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational or Gas

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point.

Admin is trusted / Malicious keepers

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point. Keepers are added by the admin, there is no "malicious keeper" and if there is a problem in those keepers, that's out of scope. ReadMe and known issues states: " * System relies heavily on keeper for executing trades * Single keeper point of failure if not properly distributed * Malicious keeper could potentially front-run or delay transactions * Assume that Keeper will always have enough gas to execute transactions. There is a pay execution fee function, but the assumption should be that there's more than enough gas to cover transaction failures, retries, etc * There are two spot swap functionalies: (1) using GMX swap and (2) using Paraswap. We can assume that any swap failure will be retried until success. " " * Heavy dependency on GMX protocol functioning correctly * Owner can update GMX-related addresses * Changes in GMX protocol could impact system operations * We can assume that the GMX keeper won't misbehave, delay, or go offline. " "Issues related to GMX Keepers being DOS'd or losing functionality would be considered invalid."

Suppositions

There is no real proof, concrete root cause, specific impact, or enough details in those submissions. Examples include: "It could happen" without specifying when, "If this impossible case happens," "Unexpected behavior," etc. Make a Proof of Concept (PoC) using external functions and realistic parameters. Do not test only the internal function where you think you found something.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.