Cvx3CrvOracle.sol _peek() returns wrong units

[code423n4]

Handle

sirhashalot

Vulnerability details

Impact

The Cvx3CrvOracle.sol contract claims it "provides current values for Cvx3Crv". When getting the current values, "only cvx3crvid and ethId are accepted as asset identifiers" for the base and quote parameters to the peek() and get() functions. peek() and get() functions both call the _peek() function, which does not calculate the price in terms of the provided base and quote tokens, but rather in 3CRV to stablecoins instead of the expected 3CRV to ETH. The resulting quoteAmount return value is therefore incorrect and quoted in terms of the wrong currencies, ignoring the base and quote currency input parameters.

Proof of Concept

The _peek() function calls threecrv.get_virtual_price() to set the price variable and uses the price variable to normalize the quoteAmount. The get_virtual_price() function is described in Curve docs as "The current price of the pool LP token relative to the underlying pool assets. Given as an integer with 1e18 precision.". In this case, it is the 3CRV LP priced in stablecoins, which should be roughly 1 x 10^18. The 3CRV value is here on coingecko. This means the base and quote asset identifiers don't make a difference in the following case, because the price variable is not relevant to the ETH or cvx3Crv base/quote values in the following code:

if (base == cvx3CrvId && quote == ethId) {
    quoteAmount = (baseAmount * price) / 1e18;
} else {
    quoteAmount = (baseAmount * 1e18) / price;
}

The units of quoteAmount should be ETH/cvx3CRV or cvx3CRV/ETH, but in actuality the units are 3CRV/stablecoins or stablecoins/3CRV.

Recommended Mitigation Steps

The base and quote input parameters are not used properly in the current code. An oracle lookup of the current price of ETH and of cvx3Crv should be performed in order to properly normalize the quoteAmount return parameter in the proper currencies. One way of doing this is to use a Chainlink asset price external adapter where the base and quote currencies can be entered.

[alcueca]

The get_virtual_price() function is described in Curve docs as "The current price of the pool LP token relative to the underlying pool assets.

1. That means that get_virtual_price() returns 3CRV/f(DAI, USDC, USDT)

2. cvx3CRV is exchangeable 1:1 with 3CRV (@iamsahu, link please). So from there get_virtual_price() can also be said to return cvx3CRV/f(DAI, USDC, USDT)

3. In our code we assume that f means "Choose the currency whose relative price to ETH is lower", meaning that get_virtual_price() returns one of these three: a. cvx3CRV/DAI if DAI has the lowest price relative to ETH b. cvx3CRV/USDC if USDC has the lowest price relative to ETH c. cvx3CRV/USDT if USDT has the lowest price relative to ETH

4. Finally, to get the cvx3Crv/ETH price, we detect whether we are in the a, b or c cases, and multiply get_virtual_price() for DAI/ETH, USDC/ETH or USDT/ETH accordingly.

I'm going to dispute this issue, although we have to confirm that the curve pool does what we think it does.

[iamsahu]

The get_virtual_price() function is described in Curve docs as "The current price of the pool LP token relative to the underlying pool assets.

1. That means that get_virtual_price() returns 3CRV/f(DAI, USDC, USDT)
2. cvx3CRV is exchangeable 1:1 with 3CRV (@iamsahu, link please). So from there get_virtual_price() can also be said to return cvx3CRV/f(DAI, USDC, USDT)
3. In our code we assume that f means "Choose the currency whose relative price to ETH is lower", meaning that get_virtual_price() returns one of these three: a. cvx3CRV/DAI if DAI has the lowest price relative to ETH b. cvx3CRV/USDC if USDC has the lowest price relative to ETH c. cvx3CRV/USDT if USDT has the lowest price relative to ETH
4. Finally, to get the cvx3Crv/ETH price, we detect whether we are in the a, b or c cases, and multiply get_virtual_price() for DAI/ETH, USDC/ETH or USDT/ETH accordingly.

I'm going to dispute this issue, although we have to confirm that the curve pool does what we think it does.

cvx3CRV is minted here 1:1, https://etherscan.io/address/0xF403C135812408BFbE8713b5A23a04b3D48AAE31#code#L971

[GalloDaSballo]

As per the discussion: -> CRV_LP to CVX_LP tokens are 1-1 -> CRV_LP tokens can be priced via the get_virtual_price and that price is in relation to all / any of the pool tokens (see stable swap invariant whitepaper)

Given that information, the pricing of the asset seems correctly done, and as such, I belive the finding to be invalid