DeFiFoundry
50,000 USDC
View results
Submission Details
Severity: low
Invalid

Precision Loss Due to Unscaled Integer Division in VaultReader's getPriceImpactInCollateral

Summary

VaultReader contains a logic error in getPriceImpactInCollateral that stems from unscaled integer division. This approach allows key values (scaled to 1e30) to be truncated when smaller than their divisor. As a result, values that should be fractional become zero or extremely inaccurate, affecting subsequent calculations such as price impact or collateral adjustments. This can lead to significant miscalculations within the protocol’s risk and fee management logic.

Vulnerability Details

The vulnerability resides in the VaultReader contract, specifically within the getPriceImpactInCollateral function. This function performs integer division on values presumed to be scaled by 1e30 without adjusting for the scale, causing truncation when the numerator is smaller than the denominator. As a result, calculations that should produce a fractional outcome become zero or severely inaccurate. This miscalculation directly impacts subsequent logic that relies on price impact data, posing a risk to the protocol’s collateral and fee computations.

Impact

By using truncated values instead of precise calculations, this flaw can lead to inaccurate assessments of required collateral, fees, and price impact. These inaccuracies could result in undercollateralized positions or incorrect fee distributions, potentially making the protocol susceptible to arbitrage, unexpected liquidations, or other adverse outcomes that compromise the system's economic integrity.

Proof of Concept

This logical error arises because the function performs integer divisions on values presumably scaled by 1e30 without adjusting for precision, resulting in truncated outputs that can become zero or severely inaccurate.

Code Analysis

Below is the relevant snippet highlighting the integer division issue. Irrelevant code has been removed for clarity:

function getPriceImpactInCollateral(
bytes32 positionKey,
uint256 sizeDeltaInUsd,
uint256 prevSizeInTokens,
MarketPrices memory prices
) external view returns (int256) {
// (!) expectedSizeInTokensDelta is derived by integer division
uint256 expectedSizeInTokensDelta = sizeDeltaInUsd / prices.indexTokenPrice.min;
uint256 curSizeInTokens = getPositionSizeInTokens(positionKey);
uint256 realSizeInTokensDelta = curSizeInTokens - prevSizeInTokens;
int256 priceImpactInTokens =
expectedSizeInTokensDelta.toInt256() - realSizeInTokensDelta.toInt256();
// Another integer operation without precision adjustment
int256 priceImpactInCollateralTokens =
priceImpactInTokens * prices.indexTokenPrice.min.toInt256()
/ prices.shortTokenPrice.min.toInt256();
return priceImpactInCollateralTokens;
}

Explanation

When sizeDeltaInUsd / prices.indexTokenPrice.min is performed without any scaling factors, significant decimals are lost if both values are in 1e30. Consequently, if sizeDeltaInUsd is smaller than prices.indexTokenPrice.min, the result becomes zero, skewing the subsequent price impact calculation.

Vulnerable Scenario

  • sizeDeltaInUsd is scaled to 1e30 (e.g., 1.5 * 1e30, representing 1.5 USD).

  • prices.indexTokenPrice.min is also 1e30 (e.g., 2 * 1e30).

  • Integer division 1.5e30 / 2e30 yields 0, causing expectedSizeInTokensDelta to be 0.

  • This feeds into the remaining logic, producing an incorrect final result. Any function relying on this calculation (fees, collateral adjustments, price impact, etc.) may thus rely on inaccurate data.

Such a scenario may lead to mismatches in fee or collateral calculations, potentially opening avenues for arbitrage or other unintended risk management behaviors.

Test and Result

The test deposits collateral, opens a position to obtain a valid positionKey, and then sets up a scenario where integer division causes truncation. It compares the actual priceImpact result (-5913539007855679092) to the expected truncated value, confirming that the division scales are not handled and leading to the precise truncated outcome. The test passes, demonstrating that the function indeed exhibits this precision loss.

  • Add the following test to test/PerpetualVault.t.sol

function testPrecisionLossInPriceImpactCalculation() external {
// Deposit some collateral from Alice to establish a valid context for opening a position
address alice = makeAddr("alice");
depositFixture(alice, 1e10);
// Open a position to ensure a valid positionKey is stored in the DataStore
MarketPrices memory initialPrices = mockData.getMarketPrices();
bytes[] memory data = new bytes[](1);
data[0] = abi.encode(3380000000000000); // Example parameter for GMX
address keeper = PerpetualVault(vault).keeper();
vm.prank(keeper);
PerpetualVault(vault).run(true, false, initialPrices, data);
GmxOrderExecuted(true);
vm.prank(keeper);
PerpetualVault(vault).runNextAction(initialPrices, new bytes[](0));
// Retrieve the positionKey and confirm it is not zero
bytes32 positionKey = PerpetualVault(vault).curPositionKey();
require(positionKey != bytes32(0), "Invalid position key");
// Define specific prices that cause integer division truncation in getPriceImpactInCollateral
// - indexTokenPrice.min = 2e30
// - shortTokenPrice.min = 1e30
// - sizeDeltaInUsd = 1e30
// Expected outcome due to integer division: priceImpactInCollateralTokens = -5913539007855679092
MarketPrices memory truncationPrices = MarketPrices({
indexTokenPrice: PriceProps({min: 2e30, max: 2e30}),
longTokenPrice: PriceProps({min: 1e30, max: 1e30}),
shortTokenPrice: PriceProps({min: 1e30, max: 1e30})
});
uint256 sizeDeltaInUsd = 1e30;
uint256 prevSizeInTokens = 0;
// Invoke the function under test
int256 priceImpact = reader.getPriceImpactInCollateral(
positionKey,
sizeDeltaInUsd,
prevSizeInTokens,
truncationPrices
);
console.logInt(priceImpact);
// Verify that the result matches the expected truncated value
int256 expectedPriceImpact = -5913539007855679092;
assertEq(
priceImpact,
expectedPriceImpact,
"Precision loss did not result in the expected truncated value"
);
}
[PASS] testPrecisionLossInPriceImpactCalculation() (gas: 4270697)
Logs:
-5913539007855679092

Confirmation

The logical error is confirmed by the absence of proper scaling in the integer division step, which can truncate otherwise non-zero values and disrupt core logic in the contract’s calculations.

Tools Used

Manual Code Review:
A comprehensive line-by-line inspection of the contract was performed to identify potential discrepancies and confirm integer division issues without proper scaling. This manual analysis highlighted the truncation problem in critical calculation paths.

Recommendations

Use proper scaling to handle 1e30 values before performing integer division. Multiply by a precision factor, then divide. For example:

- uint256 expectedSizeInTokensDelta = sizeDeltaInUsd / prices.indexTokenPrice.min;
+ uint256 expectedSizeInTokensDelta = (sizeDeltaInUsd * PRECISION) / prices.indexTokenPrice.min;

This approach preserves fractional precision and avoids truncation issues in subsequent calculations.

Updates

Lead Judging Commences

n0kto Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

invalid_prices_decimals

GMX github documentation: “Prices stored within the Oracle contract represent the price of one unit of the token using a value with 30 decimals of precision. Representing the prices in this way allows for conversions between token amounts and fiat values to be simplified, e.g. to calculate the fiat value of a given number of tokens the calculation would just be: token amount * oracle price, to calculate the token amount for a fiat value it would be: fiat value / oracle price.” Sponsor confirmed the keeper does the same, so price decimals change in function of the token, to be sure the above rule is true. Example for USDC (6 decimals): Prices will have 24 decimals → 1e6 * 1e24 = 1e30. Just a reminder for some submissions: shortToken == collateralTokens, so the decimals is 1e24 for shortToken prices.

Support

FAQs

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