Summary
In the PerpMarket contract, the getMarketPrice function, doesn't calculate the given market's mark price correctly due to precision loss.
Vulnerability Details
There's a precision loss in Perpmarket.::gettMarketPrice when determining the mark price due to divisions before multiplication and divisions before addition. Multiplication is just a series of additions. So divisions before additions as well can result in precision loss.
function getMarkPrice(
Data storage self,
SD59x18 skewDelta,
UD60x18 indexPriceX18
)
internal
view
returns (UD60x18 markPrice)
{
SD59x18 skewScale = sd59x18(self.configuration.skewScale.toInt256());
SD59x18 skew = sd59x18(self.skew);
SD59x18 priceImpactBeforeDelta = skew.div(skewScale);
SD59x18 newSkew = skew.add(skewDelta);
SD59x18 priceImpactAfterDelta = newSkew.div(skewScale);
SD59x18 cachedIndexPriceX18 = indexPriceX18.intoSD59x18();
UD60x18 priceBeforeDelta =
cachedIndexPriceX18.add(cachedIndexPriceX18.mul(priceImpactBeforeDelta)).intoUD60x18();
UD60x18 priceAfterDelta =
cachedIndexPriceX18.add(cachedIndexPriceX18.mul(priceImpactAfterDelta)).intoUD60x18();
markPrice = priceBeforeDelta.add(priceAfterDelta).div(ud60x18Convert(2));
}
Let's breakdown each line from bottom to top.
markPrice = priceBeforeDelta.add(priceAfterDelta).div(ud60x18Convert(2));
= cachedIndexPriceX18.add(cachedIndexPriceX18.mul(priceImpactBeforeDelta)).intoUD60x18().add(
cachedIndexPriceX18.add(cachedIndexPriceX18.mul(priceImpactAfterDelta)).intoUD60x18()
).div(ud60x18Convert(2));
@--> = cachedIndexPriceX18.add(cachedIndexPriceX18.mul(skew.div(skewScale))).intoUD60x18().add(
@--> cachedIndexPriceX18.add(cachedIndexPriceX18.mul(newSkew.div(skewScale))).intoUD60x18()
).div(ud60x18Convert(2));
As we can see, we divide the skew by the skewScale when calculating the priceImpactBeforeDelta and so do we to the newSkew when calculating the priceImpactAfterDelta. We can also notice that, we add priceBeforeDelta to the priceAfterDelta before dividing by 2. But both variables themselves are composed as a result of a multiplication and a division. Knowing multiplication is just a series of additions, hence, markPrice is a result of a division ( division of skew, newSkew by skewScale before an addition ( priceBeforeDelta.add(priceAfterDelta))
Proof of concept
in foundry.toml, increase the fuzz runs to 10_000
[profile.default]
auto_detect_solc = false
block_timestamp = 1_680_220_800 # March 31, 2023 at 00:00 GMT
bytecode_hash = "none"
cbor_metadata = false
evm_version = "shanghai"
-- fuzz = { runs = 1_000 }
++ fuzz = { runs = 10_000 }
gas_reports = ["*"]
libs = ["lib"]
optimizer = true
optimizer_runs = 1000
out = "out"
script = "script"
solc = "0.8.25"
src = "src"
test = "test"
And then we run the test `forge test --mc GetMarkPrice_Integration_Test -vvv` This will execute the test found at
/test/integration/perpetuals/perp-market-branch/getMarkPrice/getMarkPrice.t.sol::testFuzz_GivenTheresAMarketCreated
The test passes because using the current implementation of PerpMarket::getMarkPricemethod,
assertEq(markPrice.intoUint256(), expectedMarkPrice, "invalid mark price") will always resolve to true because markPrice.IntoUint256() will always equal expectedMarkPrice.
After we've run the above test, in src/perpetuals/leaves/PerpMarket.sol modify the getMarPrice method as follows.
function getMarkPrice(
Data storage self,
SD59x18 skewDelta,
UD60x18 indexPriceX18
)
internal
view
returns (UD60x18 markPrice)
{
SD59x18 skewScale = sd59x18(self.configuration.skewScale.toInt256());
SD59x18 skew = sd59x18(self.skew);
SD59x18 newSkew = skew.add(skewDelta);
SD59x18 cachedIndexPriceX18 = indexPriceX18.intoSD59x18();
markPrice = cachedIndexPriceX18.add(
cachedIndexPriceX18.mul(skew.add(newSkew)).div(sd59x18Convert(2).mul(skewScale))
).intoUD60x18();
}
Modify the GetMarkPrice_Integration_Test::testFuzz_GivenTheresAMarketCreated test function as follows:
file /test/integration/perpetuals/perp-market-branch/getMarkPrice/getMarkPrice.t.sol
function testFuzz_GivenTheresAMarketCreated(
uint128 marketId,
uint256 marginValueUsd,
bool isLong,
uint256 timeElapsed
)
external
{
MarketConfig memory fuzzMarketConfig = getFuzzMarketConfig(marketId);
marginValueUsd = bound({
x: marginValueUsd,
min: USDC_MIN_DEPOSIT_MARGIN,
max: convertUd60x18ToTokenAmount(address(usdc), USDC_DEPOSIT_CAP_X18)
});
timeElapsed = bound({ x: timeElapsed, min: 1 seconds, max: 365 days });
deal({ token: address(usdc), to: users.naruto.account, give: marginValueUsd });
uint128 tradingAccountId = createAccountAndDeposit(marginValueUsd, address(usdc));
openPosition(
fuzzMarketConfig,
tradingAccountId,
ud60x18(fuzzMarketConfig.imr).mul(ud60x18(1.001e18)).intoUint256(),
marginValueUsd,
isLong
);
skip(timeElapsed);
UD60x18 indexPriceX18 = ud60x18(fuzzMarketConfig.mockUsdPrice);
PerpMarket.Data memory perpMarket =
PerpMarketHarness(address(perpsEngine)).exposed_PerpMarket_load(fuzzMarketConfig.marketId);
SD59x18 skewScale = sd59x18(uint256(perpMarket.configuration.skewScale).toInt256());
SD59x18 priceImpactBeforeDelta = sd59x18(perpMarket.skew).div(skewScale);
SD59x18 newSkew = sd59x18(perpMarket.skew).add(SD59x18_ZERO);
SD59x18 priceImpactAfterDelta = newSkew.div(skewScale);
UD60x18 priceBeforeDelta =
indexPriceX18.intoSD59x18().add(indexPriceX18.intoSD59x18().mul(priceImpactBeforeDelta)).intoUD60x18();
UD60x18 priceAfterDelta =
indexPriceX18.intoSD59x18().add(indexPriceX18.intoSD59x18().mul(priceImpactAfterDelta)).intoUD60x18();
uint256 expectedMarkPrice = priceBeforeDelta.add(priceAfterDelta).div(ud60x18Convert(2)).intoUint256();
UD60x18 markPrice = perpsEngine.getMarkPrice(fuzzMarketConfig.marketId, indexPriceX18.intoUint256(), 0);
-- UD60x18 markPriceOptimized = perpsEngine.getMarkPriceOptimized(fuzzMarketConfig.marketId, indexPriceX18.intoUint256(), 0);
-- // it should return the funding velocity
++ assertLt(markPrice.intoUint256(), expectedMarkPrice + 495, "invalid mark price");
-- assertEq(markPrice.intoUint256(), expectedMarkPrice, "invalid mark price");
-- assertEq(markPriceOptimized.intoUint256(), expectedMarkPrice, "invalid mark price");
}
Run forge test --mc GetMarkPrice_Integration_Test -vvv.
The test output shows a significant loss of precision in PerpMarket::getMarketPrice().
The test fails because foundry was able to find a case where, the markPricefrom the new optimized implementation was greater than expectedMarkPriceby at least 495. Meaning that, the optimized implementation has less precision loss as compared to the unoptimized one used by the Zaros team.
Impact
This precision loss leads to a wrong calculation of the market's mark price and affect every feature/function that uses the PerpMarket::getMarketPrice function from liquidation to order filling.
Tools Used
Manual review.
Recommendations
Change the PerpMarket::getMarketPrice function to the below implementation
function getMarkPrice(
Data storage self,
SD59x18 skewDelta,
UD60x18 indexPriceX18
)
internal
view
returns (UD60x18 markPrice)
{
SD59x18 skewScale = sd59x18(self.configuration.skewScale.toInt256());
SD59x18 skew = sd59x18(self.skew);
SD59x18 newSkew = skew.add(skewDelta);
SD59x18 cachedIndexPriceX18 = indexPriceX18.intoSD59x18();
markPrice = cachedIndexPriceX18.add(
cachedIndexPriceX18.mul(skew.add(newSkew)).div(sd59x18Convert(2).mul(skewScale))
).intoUD60x18();
}
