code423n4 commented 2 years ago

Handle

0x1f8b

Vulnerability details

Impact

The oracle price use an unsecure calculation.

Proof of Concept

The contract Cvx3CrvOracle use the min price of dai, usdt and usdt instead of the average, so if an attacker is able to compromise the oracle end point, and change one of them, the contract will use the lower value instead of the average, a good practice could be revert the transaction if the price has a high slippage.

        uint256 minStable = min(
            uint256(daiPrice),
            min(uint256(usdcPrice), uint256(usdtPrice))
        );

Tools Used

Manual review.

Recommended Mitigation Steps

Use average instead of min and rever with high slippage.

devtooligan commented 2 years ago

The other side to this one is:

if any one of them break the peg, the market gets stuffed with it. You have to assume the market will be stuffed with the least valuable stable.

alcueca commented 2 years ago

However, with that min we intend to revert how Curve derives the price of the basket of stablecoins(DAI, USDC and USDT). We assume that for curve the value of that basket is equal to the minimal value of each of those three against ETH, is that right?

GalloDaSballo commented 2 years ago

This submissions is one of those where the warden should have spent extra time adding as much details and context as possible as it's very hard to defend them with the little detail given.

Let's start by describing what the oracle is trying to price: cvx3CRV which is the Convex deposited LP token for 3CRV which is the Curve DAI + USDC + USDT Pool.

Per the stableswap invariant, while the pool can offer different prices, the virtual_price is in reference to the price of any of the three assets.

To maintain a conservative estimate (good idea when dealing with collateralized positions and lending), the sponsor opted to use asset that has the lowest price (as priced by the chainlink price feed).

Due to how a Curve Pool is prize, there is zero doubt that the minimum prize is a conservative estimate.

I am failing to understand how this would be considered unsafe.

The only scenario I can imagine would be if this price was used for debt in a lending protocol and the price of one of the three stable coins where to break peg drastically.

However, in that case, the curve pool itself would also have issue as the pool is built to handle assets that have the same price and based on it's amplification-coefficient would adapt to shifts in it's internal balances, so ultimately even the pool itself would kind of break, while still maintaining the invariant that the virtual_price is in relation to all 3 input tokens.

The warden suggests using an average for the price of the tokens, however that's closer to a naive implementation, similarly to performing a swap where we offer infinite leverage (trade 1 to 1), this can be exploited by astute MEV catchers and is ultimately why curve is based on the stable swap invariant.

In conclusion, I personally think the sponsor chose a conservative pricing strategy, believe that it is consistent with how curve would be priced.

In lack of any proof of concept, any further detail, and any explanation as to why the choice is dangerous and how it can be exploited, I believe the finding to be invalid.

Funnily enough, the sponsor chosen implementation is the one suggested by chainlink: https://blog.chain.link/using-chainlink-oracles-to-securely-utilize-curve-lp-pools/

For the sake of completeness I'd also recommend the excellent summary by cmichel: https://cmichel.io/pricing-lp-tokens/

Who also reference the thorough work by Alpha Finance: https://blog.alphafinance.io/fair-lp-token-pricing/

code-423n4 / 2022-01-yield-findings