PriceFeed Manual Review Findings

PFD-01M: Potentially Insecure Multiplication

Type	Severity	Location
Mathematical Operations		PriceFeed.sol:L273

Description:

The referenced multiplication that is wrapped in an unchecked code block is not guaranteed to be safe.

Impact:

In most circumstances, the calculation will be safely performed. We advise its validity to be evaluated out of an abundance of caution as it is not guaranteed by on-chain code but rather off-chain assumptions that can break at any point.

Example:

Copy
269unchecked {
270    if (_priceDigits > TARGET_DIGITS) {
271        return _price / (10 ** (_priceDigits - TARGET_DIGITS));
272    } else if (_priceDigits < TARGET_DIGITS) {
273        return _price * (10 ** (TARGET_DIGITS - _priceDigits));
274    }
275}

Recommendation:

We advise it to be validated as safe by ensuring that the result of the calculation is greater than the _price, preventing an uncaught overflow from occurring.

Alleviation (f6f1ad0b8f24a96ade345db1dd05a1878eb0f761):

The Tren Finance team evaluated this exhibit but opted to acknowledge it in the current iteration of the codebase.

PFD-02M: Inexistent Support of Positive Exponents

Type	Severity	Location
Standard Conformity		PriceFeed.sol:L225

Description:

The Pyth Network oracles support exponent values in the inclusive [-12,12] range whereas the system solely supports negative exponents.

Impact:

Although the system is unlikely to integrate with oracles that support positive exponents, it is still advisable to prevent their configuration altogether to prevent accidentally set oracles from being accepted as valid.

Example:

Copy
208/**
209 * @dev Fetches the price and its updated timestamp from Pyth oracle.
210 * @param _oracleAddress The address of Pyth oracle.
211 */
212function _fetchPythOracleResponse(
213    address _oracleAddress,
214    bytes32 _priceFeedId
215)
216    internal
217    view
218    returns (uint256 price, uint256 timestamp)
219{
220    IPyth pythOracle = IPyth(_oracleAddress);
221    PythStructs.Price memory pythResponse = pythOracle.getPriceUnsafe(_priceFeedId);
222
223    timestamp = pythResponse.publishTime;
224    price = (uint256(uint64(pythResponse.price)) * (10 ** 18))
225        / (10 ** uint8(uint32(-1 * pythResponse.expo)));
226}

Recommendation:

We advise positive and negative exponents to be supported accordingly, or the code to mandate that the exponent is negative as in its current implementation positive exponents would be processed without yielding an error.

Alleviation (f6f1ad0b8f24a96ade345db1dd05a1878eb0f761):

The code was updated to properly support all types of exponents that the Pyth Network oracles may yield alleviating this exhibit.

PFD-03M: Potentially Insecure Error Handling

Type	Severity	Location
Logical Fault		PriceFeed.sol:L190-L196

Description:

The PriceFeed::_fetchChainlinkOracleResponse function is meant to gracefully handle all possible errors of the _oracleAddress to ensure that the PriceFeed::fetchPrice function can properly continue execution and invoke the fallback oracle if needed.

The current implementation will not be able to gracefully handle a failed call to the Chainlink oracle that yields a payload that cannot be decoded, resulting in a fatal error that halts execution.

Impact:

An unusual error of the Chainlink oracle that yields a payload less than the expected types will result in an uncaught revert that will bubble up and thus prevent the invocation of the fallback oracle even if it were functioning properly.

Example:

Copy
181/**
182 * @dev Fetches the price and its updated timestamp from Chainlink oracle.
183 * @param _oracleAddress The address of Chainlink oracle.
184 */
185function _fetchChainlinkOracleResponse(address _oracleAddress)
186    internal
187    view
188    returns (uint256 price, uint256 timestamp)
189{
190    try ChainlinkAggregatorV3Interface(_oracleAddress).latestRoundData() returns (
191        uint80 roundId,
192        int256 answer,
193        uint256, /* startedAt */
194        uint256 updatedAt,
195        uint80 /* answeredInRound */
196    ) {
197        if (roundId != 0 && updatedAt != 0 && answer != 0) {
198            price = uint256(answer);
199            timestamp = updatedAt;
200        }
201    } catch {
202        // If call to Chainlink aggregator reverts, return a zero response
203        price = 0;
204        timestamp = 0;
205    }
206}

Recommendation:

We advise the system to invoke the Chainlink oracle using low-level assembly calls and to decode the yielded payload solely if its length satisfies the expected types, preventing a decoding error from bubbling up to the top level call. For more information, consult the first information section of the relevant Solidity documentation.

Alleviation (f6f1ad0b8f24a96ade345db1dd05a1878eb0f761):

The Tren Finance team evaluated this exhibit and stated that they are confident in the oracles being utilized yielding a properly decodable payload and that they have decided to acknowledge this exhibit.

PFD-04M: Potentially Unhandled Price Timestamps

Type	Severity	Location
Mathematical Operations		PriceFeed.sol:L178

Description:

The PriceFeed::_isStalePrice function does not properly handle a _priceTimestamp that is in the future, resulting in a revert error instead.

Per the documentation of the API3 system, oracle measurements have a timestamp that represents the off-chain node's time rather than the blockchain's time and deviancy may occur.

In such a case, even a one second deviancy would result in the code reverting when it should continue unto the fallback oracle's invocation.

Impact:

In the unlikely scenario that an API3 oracle measurement yields a timestamp data point even one second in the future, the PriceFeed::fetchPrice function will fail to continue execution and invoke the fallback oracle for a price measurement.

Example:

Copy
165/**
166 * @dev Returns the flag to indicate if it is the latest or stale price.
167 * @param _priceTimestamp The latest timestamp the price was updated.
168 * @param _oracleTimeoutSeconds The maximum period that lasts a stale price.
169 */
170function _isStalePrice(
171    uint256 _priceTimestamp,
172    uint256 _oracleTimeoutSeconds
173)
174    internal
175    view
176    returns (bool)
177{
178    return block.timestamp - _priceTimestamp > _oracleTimeoutSeconds;
179}

Recommendation:

We advise the system to instead return true when the _priceTimestamp is greater than the block.timestamp as the price should not be consumed by the system in such an instance.

Alleviation (f6f1ad0b8f):

While an isPriceInFuture boolean has been introduced, the code will still perform a subtraction of _priceTimestamp from the current block.timestamp that will result in an uncaught underflow and thus cause the function to revert.

We advise the code to evaluate the conditionals independently, ensuring the function yields true when the _priceTimestamp is in the future without performing the reverting subtraction.

Alleviation (73b9546eb9):

The function was updated as advised, yielding true if the _priceTimestamp is greater than the block.timestamp and evaluating the subtraction and associated conditional otherwise.

While we would like to note that the overall code can be optimized further by yielding a single conditional directly, we still consider the exhibit fully addressed.

PFD-05M: Inconsistent Handling of Oracle Responses

Type	Severity	Location
Logical Fault		PriceFeed.sol:L190-L205, L221, L237

Description:

The Chainlink integration point will opportunistically query the price measurement from Chainlink and gracefully handle an error by yielding 0, however, the API3 and Pyth Network oracle queries are performed without such safety guards and would lead to an unhandled failure of they are set as a primary oracle and malfunction when a fallback oracle exists and operates properly.

Impact:

A system configuration that consists of a primary API3 or Pyth Network oracle malfunctioning and a valid fallback oracle will fail to delegate calls to the fallback oracle and fatally fail.

Example:

Copy
181/**
182 * @dev Fetches the price and its updated timestamp from Chainlink oracle.
183 * @param _oracleAddress The address of Chainlink oracle.
184 */
185function _fetchChainlinkOracleResponse(address _oracleAddress)
186    internal
187    view
188    returns (uint256 price, uint256 timestamp)
189{
190    try ChainlinkAggregatorV3Interface(_oracleAddress).latestRoundData() returns (
191        uint80 roundId,
192        int256 answer,
193        uint256, /* startedAt */
194        uint256 updatedAt,
195        uint80 /* answeredInRound */
196    ) {
197        if (roundId != 0 && updatedAt != 0 && answer != 0) {
198            price = uint256(answer);
199            timestamp = updatedAt;
200        }
201    } catch {
202        // If call to Chainlink aggregator reverts, return a zero response
203        price = 0;
204        timestamp = 0;
205    }
206}
207
208/**
209 * @dev Fetches the price and its updated timestamp from Pyth oracle.
210 * @param _oracleAddress The address of Pyth oracle.
211 */
212function _fetchPythOracleResponse(
213    address _oracleAddress,
214    bytes32 _priceFeedId
215)
216    internal
217    view
218    returns (uint256 price, uint256 timestamp)
219{
220    IPyth pythOracle = IPyth(_oracleAddress);
221    PythStructs.Price memory pythResponse = pythOracle.getPriceUnsafe(_priceFeedId);
222
223    timestamp = pythResponse.publishTime;
224    price = (uint256(uint64(pythResponse.price)) * (10 ** 18))
225        / (10 ** uint8(uint32(-1 * pythResponse.expo)));
226}

Recommendation:

We advise all oracle interactions to be performed using safe invocation mechanisms, ensuring that the primary oracle's failure is gracefully handled in all circumstances.

Alternatively, we advise the PriceFeed::setOracle function to prevent the configuration of a primary oracle that is a Pyth Network or an API3 one to ensure that the primary oracle's error is always gracefully handled.

Alleviation (f6f1ad0b8f):

The Pyth Network oracle call is now performed using a try-catch approach, however, the API3 call remains directly invoked rendering this exhibit partially addressed.

Alleviation (73b9546eb9):

The code was updated to perform a try-catch invocation of the API3 oracle getter function, thereby ensuring that fallback functions are adequately invoked in most circumstances.

We would like to note that the try-catch invocation style is not sufficient for all cases, and a misconfiguration of an oracle with an empty fallback function may cause decoding errors that would be unhandled.

As such, we would like to recommend the calls to be performed with yielded payload length checks as well.

Alleviation (13f0ca88ab):

The Tren Finance team evaluated our follow-up recommendation and opted to acknowledge it, rendering the original vulnerability resolved albeit without adhering to our full recommendations.

PFD-06M: Inexistent Validation of Confidence Interval

Type	Severity	Location
Logical Fault		PriceFeed.sol:L221

Description:

The PriceFeed::_fetchPythOracleResponse function does not validate that the confidence interval of the reported Pyth Network price measurement is acceptable, permitting price measurements in periods of extreme volatility to be utilized that may be inaccurate.

Impact:

The Pyth Network oracle integration does not process the confidence interval at all, permitting price measurements that are significantly volatile (i.e. 20%+ inaccurate) to be used in the sensitive Tren system.

Example:

Copy
208/**
209 * @dev Fetches the price and its updated timestamp from Pyth oracle.
210 * @param _oracleAddress The address of Pyth oracle.
211 */
212function _fetchPythOracleResponse(
213    address _oracleAddress,
214    bytes32 _priceFeedId
215)
216    internal
217    view
218    returns (uint256 price, uint256 timestamp)
219{
220    IPyth pythOracle = IPyth(_oracleAddress);
221    PythStructs.Price memory pythResponse = pythOracle.getPriceUnsafe(_priceFeedId);
222
223    timestamp = pythResponse.publishTime;
224    price = (uint256(uint64(pythResponse.price)) * (10 ** 18))
225        / (10 ** uint8(uint32(-1 * pythResponse.expo)));
226}

Recommendation:

We advise the system to impose a restriction on the maximum permitted volatility (i.e. confidence interval) of the Pyth Network oracle measurements, continuing execution to a fallback oracle if the volatility is deemed too high.

Alleviation (f6f1ad0b8f):

The Tren Finance team evaluated this exhibit and opted to acknowledge it, however, we do not consider acknowledgement to be an appropriate alleviation for this exhibit. Multiple lending and borrowing systems that integrate with the Pyth Network impose confidence interval restrictions, and it is in the best interest of the Tren Finance protocol to impose them as well.

Alleviation (13f0ca88ab):

An upper bound to the confidence interval has been enforced in the PriceFeed contract implementation, ensuring that the price measurements of the Pyth Network oracle are relatively stable for the purposes of the Tren Finance team.

PFD-07M: Inexistent Validation of Multi-Oracle Agreement

Type	Severity	Location
Logical Fault		PriceFeed.sol:L102-L116

Description:

The PriceFeed::fetchPrice function will not ensure that the primary and fallback oracles agree in the price they report, undermining the security of a price measurement from the PriceFeed oracle to a single-point-of-failure.

Impact:

The operational security of the oracle price measurements in the Tren system is lower than that of its Liquity counterpart due to trusting each oracle's measurement independently.

Example:

Copy
101/// @inheritdoc IPriceFeed
102function fetchPrice(address _token) public view virtual returns (uint256) {
103    // Tries fetching the price from the oracle
104    OracleRecord memory oracle = oracles[_token];
105    uint256 price = _fetchOracleScaledPrice(oracle);
106    if (price != 0) {
107        return oracle.isEthIndexed ? _calcEthIndexedPrice(price) : price;
108    }
109    // If the oracle fails (and returns 0), try again with the fallback
110    oracle = fallbacks[_token];
111    price = _fetchOracleScaledPrice(oracle);
112    if (price != 0) {
113        return oracle.isEthIndexed ? _calcEthIndexedPrice(price) : price;
114    }
115    revert PriceFeed__InvalidOracleResponseError(_token);
116}

Recommendation:

We advise the system to fetch both price measurements if possible and to mandate a percentage deviation threshold between the primary and fallback oracle that indicates whether a price is to be trusted or not, significantly increasing the security of the PriceFeed oracle.

Alleviation (f6f1ad0b8f24a96ade345db1dd05a1878eb0f761):

The Tren Finance team evaluated this exhibit and opted to acknowledge it. We maintain that the current approach significantly undermines the reliability of the oracle price measurements, however, we consider this to be a protocol preference thus rendering this exhibit acknowledged.