search

code-423n4 / 2022-09-y2k-finance-findings

3 stars 1 forks source link

A design flaw in the case of using 2 oracles (aka PegOracle) #283

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/oracles/PegOracle.sol#L67-L71

Vulnerability details

Impact

A design flaw in the case of using 2 oracles (aka PegOracle)

Proof of Concept

Chainlink provides price feeds denominated either in ETH or USD. But some assets don't have canonical value accessed on-chain. An example would be BTC and it's many on-chain forms like renBTC, hitBTC, WBTC, aBTC etc... For example in the case of a market on renBTC depegging from BTC value, probably a pair like renBTC/WBTC would be used (leveraging PegOracle). But even if renBTC perfectly maintains it's value to BTC, the depeg event can be triggered when WBTC significantly depreciates or appreciates against BTC value. This depeg event will be theoretically unsound since renBTC behaved as expected. The flaw comes from PegOracle because it treats both assets symmetrically.

This is also true for ETH pairs like stETH/aETH etc.. or stablecoin pairs like FRAX/MIM etc.. Of course, it should never be used like this because Chainlink provides price feeds with respect to true ETH and USD values but we have found that test files include PegOracle for stablecoin pairs...

Tools Used

Manual

Recommended Mitigation Steps

Support markets only for assets that have access to an oracle with price against canonical value x/ETH or x/USD.

HickupHH3 commented 2 years ago

From what I understand, the warden is arguing that if the underlying asset is itself a pegged asset, then it wouldn't be a very good measure against the "canonical price".

Eg. MIM (pegged) -> USDC (underlying), and USDC de-pegs, even though MIM is close to $1, the protocol would recognise this as a de-peg event.

I agree with the issue, but disagree with the severity. The choice of the underlying token is quite obviously important; I think the sponsor can attest to this.

@3xHarry thoughts? Perhaps low severity is more appropriate because it isn't a technical vulnerability per-se, more of the choice of underlying to be used.

HickupHH3 commented 2 years ago

Keeping high severity even though there are a couple of prerequisites:

I classify this as indirect loss of assets from a valid attack path that does not have hand-wavy hypotheticals

3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).