Summary
In the PerpMarket contract, the getMarketPrice function, doesn't calculate the given market's mark price correctly due to precision loss.
Vulnerability Details
In the PerpMarket::getMarketPricefunction, lies a precision loss due to division before multiplication when calculating the mark price of the give market.
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.
Proof Of Concept
Below's my optimized version of the PerpMarket::getMarkPrice function:
File: src/perpetuals/leaves/PerpMarket.sol
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
/// *** existing lines ***
++ import "forge-std/console.sol";
/// *** existing lines ***
function testFuzz_GivenTheresAMarketCreated(
uint128 marketId,
uint256 marginValueUsd,
bool isLong,
uint256 timeElapsed
)
external
{
/// *** existing code ***
++ console.log("expectedMarkPrice", expectedMarkPrice);
++ console.log("markPrice", markPrice.intoUint256());
// it should return the funding velocity
assertEq(markPrice.intoUint256(), expectedMarkPrice, "invalid mark price");
}
Run forge test --mc GetMarkPrice_Integration_Test -vv.
The test output shows a significant loss of precision in PerpMarket::getMarketPrice().
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 with an optimized one as shown in the POC.
