Lack of price validity check from Synthetix results in loss of funds while liquidating

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/ShortCollateral.sol#L133-L134 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/LiquidityPool.sol#L388

Vulnerability details

Impact

Lack of a validity check while liquidating results in loss of funds, as the price could be invalid momentarily from Synthetix due to high volatility or other issues.

Proof of Concept

There isn't a check for isInvalid in getMarkPrice and getAssetPrice, which must be false before closing the liquidation:

File: src/ShortCollateral.sol

134:         (uint256 markPrice,) = exchange.getMarkPrice();
135:         (uint256 collateralPrice,) = synthetixAdapter.getAssetPrice(currencyKey);

This check is used in other similar functions that fetch the price:

File: src/ShortCollateral.sol

194:         (uint256 markPrice, bool isInvalid) = exchange.getMarkPrice();
195:         require(!isInvalid);

205:         (collateralPrice, isInvalid) = synthetixAdapter.getAssetPrice(collateralKey);
206:         require(!isInvalid);

This must be present to ensure that the price fetched from Synthetix is not stale or invalid.

If this isn't the case, a liquidation could result in under-liquidation (a loss for the user) or over-liquidation (a loss for the protocol).

The same problem is also present in LiquidityPool:

File: src/LiquidityPool.sol

388:         (uint256 markPrice,) = exchange.getMarkPrice();

As the markPrice is not validated when calculating the orderFee.

Tools Used

Manual review

Recommended Mitigation Steps

Add a check to be sure that isInvalid is false in both markPrice and collateralPrice before liquidating.

[c4-judge]

JustDravee marked the issue as primary issue

[c4-sponsor]

rivalq marked the issue as disagree with severity

[c4-judge]

JustDravee changed the severity to QA (Quality Assurance)

[JustDravee]

Would like @rivalq 's thought on the severity and validity. Was there a reason for an absence on these checks? (Like a redundancy because it would revert somewhere on an invalid price).

It was also raised by the warden that an invalid price could be 0 through these links:

• https://github.com/Synthetixio/synthetix/blob/e311167832c5d5220b7e2c9a67a95be06ab2d1dd/contracts/PerpsV2MarketViews.sol#L45-L50
• https://github.com/Synthetixio/synthetix/blob/develop/contracts/PerpsV2Market.sol#L124-L126 How likely is this to happen?

[mubaris]

This seems like a miss from our side.

[c4-sponsor]

mubaris marked the issue as sponsor confirmed

[c4-judge]

This previously downgraded issue has been upgraded by JustDravee

[JustDravee]

Agreed on Medium severity

[c4-judge]

JustDravee marked the issue as selected for report