c4-bot-10 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-05-bakerfi/blob/main/contracts/oracles/EthOracle.sol#L31

Vulnerability details

Description

Chainlink recommends that users using price oracles, check whether the Arbitrum Sequencer is active. If the sequencer goes down, the Chainlink oracles will have stale prices from before the downtime, until a new L2 OCR transaction goes through. Users who submit their transactions via the L1 Delayed Inbox will be able to take advantage of these stale prices. Use the Chainlink oracle recommended checks to determine whether the sequencer is offline or not, and don't allow operations to take place while the sequencer is offline.

Link to code:

    function getLatestPrice() public view override returns (IOracle.Price memory price) {
@-->    (, int256 answer, uint256 startedAt, uint256 updatedAt,) = _ethPriceFeed.latestRoundData();
        if ( answer<= 0 ) revert InvalidPriceFromOracle();        
        if ( startedAt ==0 || updatedAt == 0 ) revert InvalidPriceUpdatedAt();    

        price.price = uint256(answer);
        price.lastUpdate = updatedAt;
    }

Tools Used

Manual review

Recommended Mitigation Steps

Use the Chainlink oracle recommended checks to determine whether the sequencer is offline or not, and don't allow operations to take place while the sequencer is offline.

Assessed type

Other

stalinMacias commented 4 months ago

I'd like to add the next information with regards to the usage of Chainlink Oracles on this protocol.

At first sight, because the contracts in scope, one could think that the protocol plans to use Chainlink Oracles as the Oracles to pull data from, but, digging in the settings of the Strategies, it becomes clear that the Oracles the protocol will use are not Chainlink Oracles. Why? Because the strategies are going to work with COLLATERAL/USD oracles.

For example, by looking at the constants defined on the ServiceRegistry.sol contract, we can notice that all the Oracles are meant to be quoted as ASSET/USD
- By inspecting the variables used when the WSTETH strategy is initialized, we can notice that the Oracle that will be configured for the collateral is the WSTETH_USD_ORACLE_CONTRACT, which is pulled from the ServiceRegistry.sol contract.
And, by looking at the available Chainlink feeds, and taking as a reference the collaterals mentioned on the codebase (WSTETH, CBETH, RETH), we can notice that is impossible the protocol will use Chainlink Oracles because Chainlink doesn't offer COLLATERA/USD oracles on the chains where the protocol will be deployed to (Arbitrum,Optimism,Base,Ethereum). For example:
- CBETH/USD oracle: It is only supported on Base.
- WSTETH/USD oracle: It is only supported on Linea and Arbitrum.
- RETH/USD oracle: It is not supported on Any Chain.

This information is enough to deduct that the Chainlink Oracles won't be used by the protocol, but instead, the protocol will use the Pyth Oracles. Pyth Oracles are quoted on ASSET/USD. We can query the available Pyth feeds here, and get all the Pyth Oracle addresses across all the Chains here.

For example, by using the feedId of the 3 collaterals on any of the 4 chains we can notice that Pyth indeed supports Oracles for all the collaterals quoted in USD on all the chains where the protocol will be deployed


Price Feed Contract Addresses on EVM Networks   => https://docs.pyth.network/price-feeds/contract-addresses/evm
Pyth Oracle Address in Ethereum   => https://etherscan.io/address/0x4305fb66699c3b2702d4d05cf36551390a4c69c6#readProxyContract#F7
Pyth Oracle Address in Arbitrum   => https://arbiscan.io/address/0xff1a0f4744e8582df1ae09d5611b887b6a12925c#readProxyContract#F7
Pyth Oracle Address in Optimism   => https://optimistic.etherscan.io/address/0xff1a0f4744e8582df1ae09d5611b887b6a12925c#readProxyContract#F7
Pyth Oracle Address in Base   => https://basescan.org/address/0x8250f4aF4B972684F7b336503E2D6dFeDeB1487a#readProxyContract#F7

Price Feed IDs ===> https://pyth.network/developers/price-feed-ids 0x6df640f3b8963d8f8358f791f352b8364513f6ab1cca5ed3f1f7b5448980e784 <===> WSTETH/USD 0x15ecddd26d49e1a8f1de9376ebebc03916ede873447c1255d2d5891b92ce5717 <===> CBETH/USD 0xff61491a931112ddf1bd8147cd1b641375f79f5825126d665480874634fd0ace <===> ETH/USD



All in all, I think all issues related to Chainlink Oracles should belong to the QA category because they offer information to the protocol **in case they plan to update the Oracle's configuration to use Chainlink Oracles**, but, **at the moment of the audit, all the Oracle's setting is configured to work with Pyth Oracles**, thus, ***any issues related to Chainlink oracles are not a problem for this protocol.***

c4-judge commented 3 months ago

0xleastwood marked the issue as primary issue

hvasconcelos commented 3 months ago

We have protection to protect against stale prices

0xleastwood commented 3 months ago

All instances of the price oracle are in _getPosition() which uses governance defined settings for price queries and these rely on getSafeLatestPrice() which reverts when the price is stale.

c4-judge commented 3 months ago

0xleastwood marked the issue as unsatisfactory: Invalid

t0x1cC0de commented 3 months ago

Hey @0xleastwood, I would like to point out that:

Existence of a protection against stale prices shouldn't be the grounds to reject this finding. When a L2 sequencer is down, does the protocol intend to allow the users to work with stale prices if their age is within the allowed limit? Chainlink recommends in such cases that the operation itself should revert due to the following reason, which is not the current behaviour displayed by the protocol:

If a sequencer becomes unavailable, it is impossible to access read/write APIs that consumers are using and applications on the L2 network will be down for most users without interacting directly through the L1 optimistic rollup contracts. The L2 has not stopped, but it would be unfair to continue providing service on your applications when only a few users can use them.
Hence simply calling getSafeLatestPrice() is not a safe bet. Additionally, please note that users are allowed to directly call getLatestPrice(), thus bypassing getSafeLatestPrice().