Uniswap oracle should not be used as a source of pricing on L2s

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/V3Oracle.sol#L35-L42

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/V3Oracle.sol#L35-L42

    enum Mode {
        NOT_SET,
        CHAINLINK_TWAP_VERIFY, // using chainlink for price and TWAP to verify
        TWAP_CHAINLINK_VERIFY, // using TWAP for price and chainlink to verify
        CHAINLINK, // using only chainlink directly
        TWAP // using TWAP directly
    }

Evidently, we can see that protocol uses 5 different modes for their pricing logic, also note that after setting to any of these modes other than NOT_SET it can not be reset to NOT_SET

Now the TWAP and TWAP_CHAINLINK_VERIFY modes both utilize Uniswap as the primary source of their prices and CHAINLINK_TWAP_VERIFY uses it as a secondary source, whereas this logic is well placed for the mainnet this is wrong when it comes to L2s (protocol have clearly stated that they would deploy to any EVM compatible chain) cause Uniswap advises developers not to use Uniswap oracle on L2s as price feeds are very much easier to be manipulated.

This quoted statement made from the Uniswap team is regarding integration on L2 Optimism, where every transaction is a different block, however, it is also valid for Arbitrum, as the average block time on Arbitrum is ~0.25s.

Oracles Integrations on Layer 2 Rollups

Optimism On Optimism, every transaction is confirmed as an individual block. The block.timestamp of these blocks, however, reflect the block.timestamp of the last L1 block ingested by the Sequencer. For this reason, Uniswap pools on Optimism are not suitable for providing oracle prices, as this high-latency block.timestamp update process makes the oracle much less costly to manipulate. In the future, it's possible that the Optimism block.timestamp will have much higher granularity (with a small trust assumption in the Sequencer), or that forced inclusion transactions will improve oracle security. For more information on these potential upcoming changes, please see the Optimistic Specs repo. For the time being, usage of the oracle feature on Optimism should be avoided.

Impact

Easily manipulated oracle prices, leading to multiple cases, for e.g verification could now go wrong cause the price returned from the Uniswap oracle is way different, i.e when the TWAP is not in use, and when it is the TWAP mode in use then this would cause protocol to integrate with tokens on way wrong prices.

Recommendation

Do not use Uniswap's oracle on L2s like Optimism/Arbitrum for the time being.

Assessed type

Uniswap

[c4-pre-sort]

0xEVom marked the issue as primary issue

[c4-pre-sort]

0xEVom marked the issue as sufficient quality report

[c4-pre-sort]

0xEVom marked the issue as insufficient quality report

[0xEVom]

The README states:

The protocol should be able to be deployed on any EVM compatible chain - by using chain specific config values

Which is possible by setting the mode to CHAINLINK.

Analysis

[c4-judge]

jhsagd76 marked the issue as unsatisfactory: Out of scope

[Bauchibred]

Hi @jhsagd76, we'd like to request more details on how this is OOS.

Aren't chain specific values more about timestamps and the rest and not the modes? That's to say in the V3Oracle contract, for the “Config” that’s to be chain specific, we'd be talking more about values for variables listed here: https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/V3Oracle.sol#L43-L53, i.e the specific feed address or token decimals or feed decimals for that chain would be different.

We argue that this should be reasseessed as a medium severity report, If at all protocol/sponsors do not know about uniswap TWAP not being a right source of pricing on L2s which we assume is so since this wasn't stated in the Known Issues section, cause without this report, protocol would only realize to set the mode as Chainlink mode after they’ve noticed some users being affected by this, additionally, would be key to note that Uniswap tend to have way more tokens supported than chainlink, hinting that tokens originally set to only support Uniswap for pricing would constantly be vulnerable to this, considering this check: https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/V3Oracle.sol#L211 , I.e the inability to unset the mode, and in this case protocol can’t set it to Chainlink mode cause it’s unavailable for such tokens.

[jhsagd76]

Please refer to our current rules and explanations regarding speculation on future code/deployments. https://github.com/code-423n4/org/issues/72

[Bauchibred]

Please refer to our current rules and explanations regarding speculation on future code/deployments. code-423n4/org#72

This was the verdict the supreme court ruled for that case:

Any issue that is not exploitable within the scope of the contest is defined as speculating on future code. Any such speculation only has the potential to be valid if the root cause is demonstrated to be in the contest scope. Warden may make an argument on why a future code change that would make the bug manifest is reasonably likely. Based on likelihood considerations, the Judge may assign a severity rating in any way they see fit.

But this is in-scope for this contest, since they've clearly stated to deploy to any EVM compatible chain in the contest's readMe

However the supreme court verdict also had this attached

If the exploitability relies on a particular 3rd party integration, the likelihood must factor in a competent integrator who has done due diligence.

Quoting from our escalation

We argue that this should be reaseessed as a medium severity report, If at all protocol/sponsors do not know about uniswap TWAP not being a right source of pricing on L2s which we assume is so since this wasn't stated in the Known Issues section.

Shouldn't this be the dependent on the integrator(sponsor/protocol) now?

Here is the link to the verdict: https://docs.google.com/document/d/1Y2wJVt0d2URv8Pptmo7JqNd0DuPk_qF9EPJAj3iSQiE/edit#heading=h.mzj60e8gk7n1