code423n4 commented 3 years ago

Handle

leastwood

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 is the only registered token pair that exists alongside USDV.
Hence, calculating sumNative gives us an exchange rate that is denominated as USDV/SUSHI.
Similarly, sumUSD gives us the following denominated pair, SUSHI/USD.
I'd expect the result to equal sumUSD * token.decimals() * sumNative which should give us a USDV/USD denominated result. However, the protocol calculates it as (sumUSD * token.decimals()) / sumNative which gives us a SUSHI^2 / (USD*USDV) denominated result. This seems incorrect.

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:

Tools Used

Manual code review.

Recommended Mitigation Steps

To calculate the correct consultation of a given token, the result should return sumUSD * token.decimals() * sumNative instead to ensure the target token to consult is denominated in USD and contains the correct number of decimals.

SamSteinGG commented 3 years ago

The description seems slightly incorrect as it uses a power where multiplication is performed but the general idea is correct.

SamSteinGG commented 2 years ago

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

code-423n4 / 2021-11-vader-findings