code-423n4 / 2024-05-predy-findings

10 stars 9 forks source link

Potential Use of Stale Prices in SqrtPrice Calculation in PriceFeed.sol #291

Closed howlbot-integration[bot] closed 4 months ago

howlbot-integration[bot] commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/PriceFeed.sol#L46-L52 https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/libraries/PositionCalculator.sol#L141-L147

Vulnerability details

Impact

This report identifies a potential vulnerability in the getSqrtPrice function of the protocol. The function retrieves price data from two separate oracles: a Chainlink oracle and an IPyth oracle. While the function verifies the validity of the IPyth price data by ensuring it's not older than a specified time period (VALID_TIME_PERIOD), it does not perform a similar check for the Chainlink oracle data. This omission could lead to the use of stale prices in the calculation of the square root price (sqrtPrice), potentially impacting various functionalities within the protocol.

Proof of Concept

The sqrtPrice calculated using potentially stale Chainlink data might not reflect the true market price. this is actively used in position calculation

  function calculateMinMargin(
        DataType.PairStatus memory pairStatus,
        DataType.Vault memory vault,
        DataType.FeeAmount memory feeAmount
    ) internal view returns (int256 minMargin, int256 vaultValue, bool hasPosition, uint256 sqrtOraclePrice) {
        int256 minValue;
        uint256 debtValue;

        sqrtOraclePrice = getSqrtIndexPrice(pairStatus);

 function getSqrtIndexPrice(DataType.PairStatus memory pairStatus) internal view returns (uint256 sqrtPriceX96) {
        if (pairStatus.priceFeed != address(0)) {
            return PriceFeed(pairStatus.priceFeed).getSqrtPrice();

The value obtained from the feed are used to Calculate minmargin which are also critical to other sensitive functions used to liquidate and check the vault's health. Using a stale price can prevent the liquidation of an account that should be liquidated as the vault will still be safe.


    function calculateMinMargin(
        DataType.PairStatus memory pairStatus,
        DataType.Vault memory vault,
        DataType.FeeAmount memory feeAmount
    ) internal view returns (int256 minMargin, int256 vaultValue, bool hasPosition, uint256 sqrtOraclePrice) {
        int256 minValue;
        uint256 debtValue;

        sqrtOraclePrice = getSqrtIndexPrice(pairStatus);
    function getIsSafe(
        DataType.PairStatus memory pairStatus,
        DataType.Vault memory _vault,
        DataType.FeeAmount memory feeAmount
    ) internal view returns (int256 minMargin, bool isSafe, bool hasPosition) {
        int256 vaultValue;

        (minMargin, vaultValue, hasPosition,) = calculateMinMargin(pairStatus, _vault, feeAmount);
 isSafe = vaultValue >= minMargin && _vault.margin >= 0;

Tools Used

Recommended Mitigation Steps

To mitigate this vulnerability, it's recommended to implement a similar timeliness check for the Chainlink oracle data as the one currently in place for the IPyth data. Here's an approach:

  1. Retrieve Latest Chainlink Data: Use the latestRoundData function of the AggregatorV3Interface to obtain the most recent price data from the Chainlink oracle.
  2. Verify Timestamp: Check the timestamp associated with the retrieved Chainlink data. This timestamp should be within an acceptable time limit ( VALID_TIME_PERIOD = 5 * 60).
  3. Reject Stale Data: If the timestamp indicates that the Chainlink data is older than the acceptable time limit, revert the function or throw an error, preventing the use of stale data.

Assessed type

Oracle

c4-judge commented 4 months ago

alex-ppg marked the issue as satisfactory