Incorrect Accrual Of `sumNative` and `sumUSD` In Producing Consultation Results

[code423n4]

Handle

leastwood

Vulnerability details

Vulnerability details

Impact

The TwapOracle.consult() function iterates over all token pairs which belong to either VADER or USDV` and then calculates the price of the respective asset by using both UniswapV2 and Chainlink price data. This helps to further protect against price manipulation attacks as the price is averaged out over the various registered token pairs.

Let's say we wanted to query the price of USDV, we would sum up any token pair where USDV == pairData.token0.

The sum consists of the following:

• Price of USDV denominated in terms of token1 (USDV/token1).
• Price of token1 denominated in terms of USD (token1/USD).

Consider the following example:

• SUSHI and UNISWAP are the only registered token pairs that exist alongside USDV.
• Hence, calculating sumNative gives us an exchange rate that is denominated as the sum of USDV/SUSHI and USDV/UNISWAP.
• Similarly, sumUSD gives us the following denominated pairs, SUSHI/USD and UNISWAP/USD.
• Summing sumUSD and sumNative produces an entirely incorrect result as compared to multiplying the two results first and then summing.
• The issue is equivalent to the same issue as performing (p1 + p2)*(q1 + q2) as compared to (p1*q1 + p2*q2). Obviously, these two results are not equivalent, however, the consult() function treats them as such.
• If we multiply the native price and Chainlink oracle results, then we can correctly calculate the price as such; (SUSHI/USD * USDV/SUSHI + UNISWAP/USD * USDV/UNISWAP) / 2, which should correctly give us the correct denomination and average price. However, the protocol calculates it as ((SUSHI/USD + UNISWAP/USD) * token.decimals()) / (USDV/SUSHI + USDV/UNISWAP) which gives us an incorrectly denominated result.

I'd classify this issue as high risk as the oracle returns false results upon being consulted. This can lead to issues in other areas of the protocol that use this data in performing sensitive actions.

Proof of Concept

https://github.com/code-423n4/2021-11-vader/blob/main/contracts/twap/TwapOracle.sol#L115-L157

Similar working implementation listed below:

• https://github.com/gg2001/dpx-oracle/blob/master/contracts/UniswapV2Oracle.sol#L184-L211
• https://github.com/gg2001/dpx-oracle/blob/master/contracts/UniswapV2Oracle.sol#L291-L304

Tools Used

Manual code review.

Recommended Mitigation Steps

To calculate the correct consultation of a given token, the returned result should consist of a sum of priceUSD * token.decimals() * priceNative divided by the number of calculations. This should correctly take the average token pair price.

The following snippet of code details the relevant fix:

    function consult(address token) public view returns (uint256 result) {
        uint256 pairCount = _pairs.length;

        for (uint256 i = 0; i < pairCount; i++) {
            PairData memory pairData = _pairs[i];

            if (token == pairData.token0) {
                //
                // TODO - Review:
                //   Verify price1Average is amount of USDV against 1 unit of token1
                //

                priceNative = pairData.price1Average.mul(1).decode144(); // native asset amount
                if (pairData.price1Average._x != 0) {
                    require(priceNative != 0);
                } else {
                    continue; // should skip newly registered assets that have not been updated yet.
                }

                (
                    uint80 roundID,
                    int256 price,
                    ,
                    ,
                    uint80 answeredInRound
                ) = AggregatorV3Interface(_aggregators[pairData.token1])
                        .latestRoundData();

                require(
                    answeredInRound >= roundID,
                    "TwapOracle::consult: stale chainlink price"
                );
                require(
                    price != 0,
                    "TwapOracle::consult: chainlink malfunction"
                );
                priceUSD = uint256(price) * (10**10);
                result += ((priceUSD * IERC20Metadata(token).decimals()) * priceNative);
            }
        }
        require(sumNative != 0, "TwapOracle::consult: Sum of native is zero");
        return result;
    }

[SamSteinGG]

The TWAP oracle module has been completely removed and redesigned from scratch as LBTwap that is subject of the new audit.