StrategyV5Chainlink Manual Review Findings

SVC-01M: Inexistent Prevention of Data Corruption

Type	Severity	Location
Input Sanitization		StrategyV5Chainlink.sol:L55-L59

Description:

The StrategyV5Chainlink::setPriceFeed function does not ensure that no previous entry exists for either the _address or _feed, allowing corruption of their respective data entries in the system.

Impact:

A severity of minor has been assigned due to the function's privileged nature.

Example:

Copy
49/**
50 * @dev Sets the validity duration for a single price feed
51 * @param _address The address of the token we want the feed for
52 * @param _feed The pricefeed address for the token
53 * @param _validity The new validity duration in seconds
54 */
55function setPriceFeed(address _address, IChainlinkAggregatorV3 _feed, uint256 _validity) public onlyAdmin {
56    feedByAsset[_address] = _feed;
57    decimalsByFeed[_feed] = feedByAsset[_address].decimals();
58    validityByFeed[feedByAsset[_address]] = _validity;
59}

Recommendation:

We advise the StrategyV5Chainlink::setPriceFeed function to ensure that the feedByAsset[_address] entry is zero, and to utilize a different variable to track whether the _feed has been configured to be validated as such.

Alleviation (59b75fbee1d8f3dee807c928f18be41c58b904e1):

The Astrolab DAO team evaluated this exhibit and has opted to purposefully not validate whether a pre-existing feed exists in the relocated ChainlinkProvider::_setFeed function as they wish to be able to set temporary "identity" feeds for bridged assets when proper feeds are not present.

As such, we consider this exhibit alleviated based on the fact that the Astrolab DAO team will responsibly employ the data feed configurations.

SVC-02M: Inexistent Validation of Prices

Type	Severity	Location
Logical Fault		StrategyV5Chainlink.sol:L178, L199

Description:

In direct contradiction with the ChainlinkUtils::getPriceUsd function, the referenced Chainlink queries do not ensure the yielded price is positive.

Impact:

The likelihood of a Chainlink oracle misbehaving is considered low, however, validation of the yielded price should always be performed as a fail-safe.

Example:

Copy
173function _usdToInput(
174    uint256 _amount,
175    uint8 _index
176) internal view returns (uint256) {
177    IChainlinkAggregatorV3 feed = feedByAsset[address(inputs[_index])];
178    (, int256 price, , uint256 updateTime, ) = feed.latestRoundData();
179    if (block.timestamp > (updateTime + validityByFeed[feed]))
180        revert InvalidOrStaleValue(updateTime, price);
181    return
182        _amount.mulDiv(
183            10 ** (uint256(decimalsByFeed[feed]) + inputDecimals[_index] - 6),
184            uint256(price)
185        ); // eg. (1e6+1e8+1e6)-(1e8+1e6) = 1e6
186}

Recommendation:

We advise such validation to be introduced, preventing invalid prices from being consumed as acceptable by the system.

Alleviation (59b75fbee1d8f3dee807c928f18be41c58b904e1):

The StrategyV5Chainlink implementation has been superseded by the ChainlinkProvider implementation, and the relevant statement is now located in the ChainlinkProvider::_toUsdBp function.

A validity check for the reported price has been introduced in the relocated code, properly alleviating this exhibit.