Boost _ Folks Finance 33353 - [Smart Contract - Low] Incorrect implementation of Time-Weighted Average Price for a Chainlink feed will lead to Incorrect Liquidation amount and breaks multiple price consumption based function

Submitted on Thu Jul 18 2024 14:05:39 GMT-0400 (Atlantic Standard Time) by @Tripathi for Boost | Folks Finance

Report ID: #33353

Report type: Smart Contract

Report severity: Low

Target: https://testnet.snowtrace.io/address/0xA758c321DF6Cd949A8E074B22362a4366DB1b725

Impacts:

• Protocol insolvency

• Direct theft of any user funds, whether at-rest or in-motion, other than unclaimed yield

Description

Brief/Intro

protocol checks historical data to calculate Time-Weighted Average Price for a Chainlink feed from all the prices between the latest round and the latest round before the time interval. But checking the historical data is incorrect according to the chainlink docs which can damage some serious logic with in the protocol. Since liquidation amount, borrow amount is calculated from fetching price of asset(i.e processPriceFeed)

Vulnerability Details

unction getTwapPrice(
        AggregatorV3Interface chainlink,
        uint80 latestRoundId,
        uint256 latestPrice,
        uint256 twapTimeInterval
    ) internal view returns (uint256 price) {
        uint256 priceSum = latestPrice;
        uint256 priceCount = 1;

        uint256 startTime = block.timestamp - twapTimeInterval;

        /// @dev Iterate over the previous rounds until reaching a round that was updated before the start time
        while (latestRoundId > 0) {
            try chainlink.getRoundData(--latestRoundId) returns (
                uint80,
                int256 answer,
                uint256,
                uint256 updatedAt,
                uint80
            ) {
                if (updatedAt < startTime) {
                    break;
                }
                priceSum += answer.toUint256();
                priceCount++;
            } catch {
                //@audit-issue roundId doesn't decrement one one 
                break;
            }
        }

        return priceSum / priceCount;
    }

https://github.com/Folks-Finance/folks-finance-xchain-contracts/blob/main/contracts/oracle/nodes/ChainlinkNode.sol#L47

But this is incorrect way of fetching historical data. chainlink docs say:

Oracles provide periodic data updates to the aggregators. Data feeds are updated in rounds. Rounds are identified by their roundId, which increases with each new round. This increase may not be monotonic. Knowing the roundId of a previous round allows contracts to consume historical data

so it is not mendatory that there will be valid data for currentRoundID-1. if there is not data for currentRooundId-1 then it will just return the weighted average till that time.

weighted average was mean to be from currentTimestamp to currentTimestamp-twapTimeInterval but it will end of returning spot price which is too close to currentTimestamp failing all the logic of using TWAP price.

check this - https://docs.chain.link/data-feeds/historical-data#solidity

roundId is NOT incremental. Not all roundIds are valid. You must know a valid roundId before consuming historical data.

Impact Details

HubPool::updatePoolWithDeposit()

HubPool::preparePoolForBorrow()

Liquidation::calcLiquidationAmounts()

LoanManager::executeDepositFToken()

etc. All these crucial function fetch balance from Chainlink oracles and due to above issue instead of using TWAP till twapTimeInterval they will end up consuming more spot price breaking logic of TWAP.

Recommendations

As chainlink docs says.

Increase in roundId may not be monotonic so loop through the previous roundID and fetch the previoous roundId data

 iterate (from roundId-1 to untill we get previous first data corressponding to roundID){
    if(data present for roundID){
        fetch the data and return
    }else{
        again iterate to get the data
    }
 }

Proof of concept

Proof of Concept

1. Replace MockChainlinkAggregator::getRoundData() with the below getRoundData() function. It reverts with a RoundId doesn't exist error, which is the root cause of the issue. Since Chainlink doesn't provide data for every roundID, there will be roundIDs for which getRoundData() reverts

    function getRoundData(
        uint80 _roundId
    )
        external
        view
        override
        returns (uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt, uint80 answeredInRound)
    {
        require(_prices[_roundId - 1] != 0, "RoundId doesn't exist");
        return (_roundId, _prices[_roundId - 1], 0, _updatedAt[_roundId - 1], roundId);
    }

1. Paste the below test in ChainlinkNode.test.ts, run the test, and it can be seen that the test shows that TWAP is the same as the spot price due to an issue in ChainlinkNode::getTwapPrice(), which validates the issue that Chainlink getTwapPrice() is returning the spot price instead of the TWAP price.

import { loadFixture } from "@nomicfoundation/hardhat-toolbox/network-helpers";
import { expect } from "chai";
import { ethers } from "hardhat";
import { MockChainlinkAggregator, NodeManager } from "../../../typechain-types";
import NodeType from "../assets/NodeType";
import { deployNodeManagerFixture } from "../bootstrap";
import { NodeManagerUtil } from "../utils/nodeManagerUtils";
import { PRECISION, abi, deployMockChainlinkAggregator, getOracleNodeId } from "../utils/utils";

describe("ChainlinkNode", async function () {
  let nodeManager: NodeManager;
  let mockChainlinkAggregator: MockChainlinkAggregator;
  let mockChainlinkAggregatorAddr: string;
  let deployBlockTimestamp: number;
  let decimals: number;
  let prices: [number, number, number, number, number];
  let timestampDeltas: [number, number, number, number, number];

  beforeEach("Deploy NodeManager and MockChainlinkAggregator contracts", async function () {
    ({ nodeManager } = await loadFixture(deployNodeManagerFixture));

    decimals = 6;
    prices = [420e5, 426e5, 429e5, 431e5, 432e5];
    timestampDeltas = [60 * 25, 60 * 20, 60 * 15, 60 * 10, 0];
    ({ mockChainlinkAggregator, deployBlockTimestamp } = await deployMockChainlinkAggregator(
      decimals,
      prices,
      timestampDeltas
    ));
    mockChainlinkAggregatorAddr = await mockChainlinkAggregator.getAddress();
  });

  describe("Register node", async function () {
    it("Should register a chainlink node", async function () {