cccz - priceLiquidity() may not work if PriceFeed.aggregator() is updated

[github-actions]

cccz

medium

priceLiquidity() may not work if PriceFeed.aggregator() is updated

Summary

priceLiquidity() may not work if PriceFeed.aggregator() is updated

Vulnerability Detail

In the constructor of the DepositReceipt_* contract, the value of minAnswer/maxAnswer in priceFeed.aggregator() is obtained and assigned to *MinPrice/*MaxPrice as the maximum/minimum price limit when calling the getOraclePrice function in priceLiquidity, and *MinPrice/*MaxPrice can not change.

        IAccessControlledOffchainAggregator  aggregator = IAccessControlledOffchainAggregator(priceFeed.aggregator());
        //fetch the pricefeeds hard limits so we can be aware if these have been reached.
        tokenMinPrice = aggregator.minAnswer();
        tokenMaxPrice = aggregator.maxAnswer();
...
            uint256 oraclePrice = getOraclePrice(priceFeed, tokenMaxPrice, tokenMinPrice);
...
    function getOraclePrice(IAggregatorV3 _priceFeed, int192 _maxPrice, int192 _minPrice) public view returns (uint256 ) {
        (
            /*uint80 roundID*/,
            int signedPrice,
            /*uint startedAt*/,
            uint timeStamp,
            /*uint80 answeredInRound*/
        ) = _priceFeed.latestRoundData();
        //check for Chainlink oracle deviancies, force a revert if any are present. Helps prevent a LUNA like issue
        require(signedPrice > 0, "Negative Oracle Price");
        require(timeStamp >= block.timestamp - HEARTBEAT_TIME , "Stale pricefeed");
        require(signedPrice < _maxPrice, "Upper price bound breached");
        require(signedPrice > _minPrice, "Lower price bound breached");

But in the priceFeed contract, the address of the aggregator can be changed by the owner, which may cause the value of minAnswer/maxAnswer to change, and the price limit in the DepositReceipt_* contract to be invalid, and priceLiquidity() can not work.

  function confirmAggregator(address _aggregator)
    external
    onlyOwner()
  {
    require(_aggregator == address(proposedAggregator), "Invalid proposed aggregator");
    delete proposedAggregator;
    setAggregator(_aggregator);
  }


  /*
   * Internal
   */

  function setAggregator(address _aggregator)
    internal
  {
    uint16 id = currentPhase.id + 1;
    currentPhase = Phase(id, AggregatorV2V3Interface(_aggregator));
    phaseAggregators[id] = AggregatorV2V3Interface(_aggregator);
  }
  ...
    function aggregator()
    external
    view
    returns (address)
  {
    return address(currentPhase.aggregator);
  }

Impact

Code Snippet

https://github.com/sherlock-audit/2022-11-isomorph/blob/main/contracts/Velo-Deposit-Tokens/contracts/DepositReceipt_ETH.sol#L66-L80
https://github.com/sherlock-audit/2022-11-isomorph/blob/main/contracts/Velo-Deposit-Tokens/contracts/DepositReceipt_USDC.sol#L60-L64
https://github.com/sherlock-audit/2022-11-isomorph/blob/main/contracts/Velo-Deposit-Tokens/contracts/DepositReceipt_ETH.sol#L107-L109
https://github.com/sherlock-audit/2022-11-isomorph/blob/main/contracts/Velo-Deposit-Tokens/contracts/DepositReceipt_ETH.sol#L134-L135
https://github.com/sherlock-audit/2022-11-isomorph/blob/main/contracts/Velo-Deposit-Tokens/contracts/DepositReceipt_USDC.sol#L90-L91
https://github.com/sherlock-audit/2022-11-isomorph/blob/main/contracts/Velo-Deposit-Tokens/contracts/DepositReceipt_USDC.sol#L113-L114
https://github.com/sherlock-audit/2022-11-isomorph/blob/main/contracts/Velo-Deposit-Tokens/contracts/DepositReceipt_Base.sol#L164-L176
https://etherscan.io/address/0xc7de7f4d4C9c991fF62a07D18b3E31e349833A18#code
https://etherscan.io/address/0x72002129A3834d63C57d157DDF069deE37b08F24#code

Tool used

Manual Review

Recommendation

Consider getting latest priceFeed.aggregator().minAnswer()/maxAnswer() in priceLiquidity()

[kree-dotcom]

Chainlink documents state: "you can call functions on the aggregator directly, but it is a best practice to use the AggregatorV3Interface to run functions on the proxy instead so that changes to the aggregator do not affect your application. Read the aggregator contract only if you need functions that are not available in the proxy."

So the auditor is right that we should not assume the AccessControlledOffchainAggregator is static. We have moved these calls to occur on every call rather than in setup.

[kree-dotcom]

Fixed kree-dotcom/Velo-Deposit-Tokens@58b8f3e

Note there are two fixes in this commit relating to the priceLiquidity function. the other issue is #72 , the code for these changes doesn't overlap so should be clear.

[IAm0x52]

Fixes look good. minPrice and maxPrice are now called dynamically from the aggregator