Gravita-Protocol / Gravita-SmartContracts

GNU General Public License v3.0
48 stars 31 forks source link

[PFD-01M] Reduction of Price Feed Security #377

Closed 0xfornax closed 1 year ago

0xfornax commented 1 year ago

PFD-01M: Reduction of Price Feed Security

Type Severity Location
Language Specific PriceFeed.sol:L120-L133

Description:

The original PriceFeed implementation contained logic that permitted certain deviation thresholds between price measurements to ensure that the Gravita Protocol can react to extreme market events.

The new implementation has removed these checks and solely implements a time-based validation mechanism.

Impact:

The removal of this security feature may have been performed due to a valid reason if the Gravita Protocol supplements sufficient research material indicating that it was ineffective.

If the Gravita Protocol team fails to supply such material, the severity of this exhibit will be upgraded to "medium".

Example:

    uint256 oraclePrice;
    uint256 priceTimestamp;
    if (oracle.oracleAddress == address(0)) {
        revert PriceFeed__UnknownAssetError();
    }
    if (ProviderType.Chainlink == oracle.providerType) {
        (oraclePrice, priceTimestamp) = _fetchChainlinkOracleResponse(oracle.oracleAddress);
    }
    if (oraclePrice != 0 && !_isStalePrice(priceTimestamp, oracle.timeoutMinutes)) {
        return _scalePriceByDigits(oraclePrice, oracle.decimals);
    }
    return 0;
}

Recommendation:

We advise the original threshold-based security measures to be re-instated as they are crucial to the operational integrity of the PriceFeed contract.

spider-g commented 1 year ago

Thank you for your report. We have thoroughly researched whether the burden of comparing rounds justifies the gas costs. Our findings, based on Chainlink's documentation, indicate that a new price or round is triggered not only after the aggregators' heartbeats have passed but also when there is a price deviation threshold, typically around 1%.

In volatile markets, this means that multiple subsequent rounds will be triggered within short periods instead of larger price changes in each round/heartbeat. Therefore, in the case of a significant price drop, simply comparing two rounds would not provide much additional safety as the difference between them is likely to be close to the triggering threshold.

Additionally, we examined how other DeFi protocols handle oracle data. Most of them rely on Chainlink's responses with minimal sanity checks, while we have implemented a few checks such as considering staleness or zero answers. As references, we can cite the following protocols:

Aave: [link to Aave's AaveOracle.sol](https://github.com/aave/aave-v3-core/blob/master/contracts/misc/AaveOracle.sol)
Euler: [link to Euler's WSTETHOracle.sol](https://github.com/euler-xyz/euler-contracts/blob/master/contracts/oracles/WSTETHOracle.sol)
Mai: [link to Mai's shareOracle.sol](https://github.com/0xlaozi/qidao/blob/main/oracles/shareOracle.sol)

Taking into account the above information and considering Chainlink's expertise and reputation for providing quality data with many integrity checks, we concluded that adding an extra step of comparing rounds would be redundant and unnecessary. Therefore, we opted for a simpler approach, which seems to be the standard among other protocols.