c4-submissions commented 8 months ago

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/oracles/ChainlinkPriceOracle.sol#L38

Vulnerability details

Impact

According to Chainlink's documentation, lastestAnswer() is deprecated and should no be used. Furthermore, the use of this function does not allow Kelp DAO to check the freshness of the price gotten. This function will not revert if no answer was reached by the oracle and will return 0 instead. This will affect Kelp if LRTOracleuses ChainlinkPriceOracle as the "PriceFetcher". If no answer is found by the oracle, getAssetPrice(asset) will return 0. The main area of concern is with getRSETHPrice() function:

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTOracle.sol#L66-L79

If assetER = 0 then the return value of the getRSETHPrice() function will also be 0, reverting that specific deposit in LRTDepositPool as it is impossible to divide by 0: https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L109

see: https://docs.chain.link/data-feeds/api-reference#latestanswer

Proof of Concept

// SPDX-License-Identifier: UNLICENSED

pragma solidity 0.8.21;

import { BaseTest } from "./BaseTest.t.sol";
import { LRTDepositPool } from "src/LRTDepositPool.sol";
import { RSETHTest, ILRTConfig, UtilLib, LRTConstants } from "./RSETHTest.t.sol";
import { ILRTDepositPool } from "src/interfaces/ILRTDepositPool.sol";
import "forge-std/console.sol";
import { LRTOracle } from "src/LRTOracle.sol";
import {IPriceFetcher} from "src/interfaces/IPriceFetcher.sol";

import { TransparentUpgradeableProxy } from "lib/openzeppelin-contracts/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import { ProxyAdmin } from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";

contract LRTDepositPoolTest is BaseTest, RSETHTest {
    LRTDepositPool public lrtDepositPool;
    LRTOracle public lrtOracle;
    address public rETHAddress; 
    IPriceFetcher public priceFetcher; 
    ILRTDepositPool public depositPool;

    function setUp() public virtual override(RSETHTest, BaseTest) {
        super.setUp();

        rETHAddress = address(rETH);
        // deploy LRTDepositPool
        ProxyAdmin proxyAdmin = new ProxyAdmin();
        LRTDepositPool contractImpl = new LRTDepositPool();
        TransparentUpgradeableProxy contractProxy = new TransparentUpgradeableProxy(
            address(contractImpl),
            address(proxyAdmin),
            ""
        );
        LRTOracle lrtOracleImpl = new LRTOracle();
        TransparentUpgradeableProxy lrtOracleProxy = new TransparentUpgradeableProxy(
            address(lrtOracleImpl),
            address(proxyAdmin),
            ""
        );

        lrtDepositPool = LRTDepositPool(address(contractProxy));

        // initialize RSETH. LRTCOnfig is already initialized in RSETHTest
        rseth.initialize(address(admin), address(lrtConfig));
        vm.startPrank(admin);
        // add rsETH to LRT config
        lrtConfig.setRSETH(address(rseth));
        // add oracle to LRT config
        lrtOracle = LRTOracle(address(lrtOracleProxy));
        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(lrtOracle));

        // add minter role for rseth to lrtDepositPool
        rseth.grantRole(rseth.MINTER_ROLE(), address(lrtDepositPool));

        vm.stopPrank();

    }

    function test_OracleBug() external {
        vm.mockCall(address(priceFetcher), abi.encodeWithSelector(IPriceFetcher.getAssetPrice.selector), abi.encode(1 ether)); //Assume LST token price is 1 ether
        vm.mockCall(address(depositPool), abi.encodeWithSelector(ILRTDepositPool.getTotalAssetDeposits.selector), abi.encode(0)); // assume first deposit.

        lrtOracle.initialize(address(lrtConfig));
        lrtDepositPool.initialize(address(lrtConfig));

        vm.startPrank(alice);
        deal(address(rETH), alice, 1000 ether);

        rETH.approve(address(lrtDepositPool), 1000 ether);
        lrtDepositPool.depositAsset(rETHAddress, 3 ether);

        vm.mockCall(address(priceFetcher), abi.encodeWithSelector(IPriceFetcher.getAssetPrice.selector), abi.encode(0)); //If chainlink oracle does not find an answer it will return 0
        vm.mockCall(address(depositPool), abi.encodeWithSelector(ILRTDepositPool.getTotalAssetDeposits.selector), abi.encode(9 ether));

        vm.expectRevert();
        lrtDepositPool.depositAsset(rETHAddress, 3 ether); //alice deposits again and it will revert

    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Use latestRoundData() function instead.