search

code-423n4 / 2023-11-kelp-findings

13 stars 11 forks source link

Security issues with chainlink price oracle #359

Closed c4-submissions closed 11 months ago

c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/oracles/ChainlinkPriceOracle.sol#L37-L39 https://github.com/code-423n4/2023-11-kelp/blob/main/src/oracles/ChainlinkPriceOracle.sol#L11-L13

Vulnerability details

Impact

High impact There are some security concerns that can lead a user to get more shares than it would

Security concerns

There are some security concerns that have not been taken in account with this chainlink oracle implementation.

  1. Usage of a deprecated chainlink price feed implementation

    function getAssetPrice(address asset) external view onlySupportedAsset(asset) returns (uint256) {
    return AggregatorInterface(assetPriceFeed[asset]).latestAnswer();
}

    This is an old implementation of the chainlink price feeds. If this implementation is finally used, chainlink can at some point stop feeding prices into this deprecated contracts and leave the protocol without any data price. It is highly recommended to use the AggregatorV3 implementation.

  2. There is no check for stale prices Chainlink data feeds can return stale pricing data for some reasons. You can check out some of them here: https://ethereum.stackexchange.com/questions/133242/how-future-resilient-is-a-chainlink-price-feed/133843#133843 To solve this problem, it is recommended to store the heartbeat of the specific price feed and check the latest update of it. Notice that each price feed can have different heartbeats, for example stETH/ETH has 12 hour heartbeat whereas rETH/ETH has 18 hour. Example:

    function getAssetPrice(address asset) external view onlySupportedAsset(asset) returns (uint256) { {
(, int256 price, , uint256 updatedAt, ) = AggregatorV3Interface(assetPriceFeed[asset]).latestRoundData();
require(block.timestamp - updatedAt <= heartbeatInterval, "stale price");
return uint256(price);
}

  3. Price changes can be frontrunned A user can monitor when a price feed is about to get updated and frontrun it by depositing assets. Imagine the following situations:

Situation with frontrunning:

Now the price of stETH is about to get updated and user knows the next price because is monitoring the change. If the price is going to drop, the user will obviously not going to deposit anything because he will lose value. However, if he sees that the value of the reserves of the protocol are gonna increase, if he deposit assets before hand, he will get the benefit from this increase. Imagine that the price of stETH will increase to 200 ETH The user deposits this amount before and will get 33 shares of the pool Now the price gets updated

User's shares are worth approximately 1309 ETH. User obtained 309 ETH of profit without taking any risk

Situation without frontrunning: If the user would decided to deposit his rETH when the price of stETH has already been changed this would be the situation:

User will receive only 25 shares of the pool. Hence, he will not get any benefit from the price increase. This issue can be mitigated by locking depositAsset function to some amount of timestamp before the heartbeat.

  1. Price feeds can have different amounts of decimals It is true that almost all price assets in ETH have 18 decimals of precision, but there are some other that have less decimals. Also, in the future, chainlink can add new price feeds for assets that may be implemented by EigenLayer with different amounts of decimals. That would lead to miscalculations with the computation of how much worth are the reserves in terms of ETH, because some assets would have been calculated with 18 decimals whereas other with less. This issue can be mitigated by normalizing all price feeds to 18. The protocol can check the decimals function in the chainlink price feed and adjust the result of the specific asset to 18 decimals. Example: 
    (, int256 price, , , ) = AggregatorV3Interface(assetPriceFeed[asset]).latestRoundData();
uint256 normalizedPrice = uint256(price) * 10 ** (18 - AggregatorV3Interface(assetPriceFeed[asset]).decimals());

Tools Used

Manual Review

Recommended Mitigation Steps

For the function to obtain the asset price:

    function getAssetPrice(address asset) external view onlySupportedAsset(asset) returns (uint256 normalizedPrice) { 
        (, int256 price, , uint256 updatedAt, ) = AggregatorV3Interface(assetPriceFeed[asset]).latestRoundData();
        require(block.timestamp - updatedAt <= heartbeatInterval, "stale price");
        normalizedPrice = uint256(price) * 10 ** (18 - AggregatorV3Interface(assetPriceFeed[asset]).decimals());
    }

In this function, the decimals concern and the stale price check is solved. To solve frontrunning, we need to add the following function and variables in the oracle:

    uint256 constant public LOCKING_TIME = 30;      // 30 seconds of locking time
    mapping(address asset => uint256 heartBeat) public heartbeats;

    function lockFrontrunning() external view {
        address[] memory supportedAssets = lrtConfig.getSupportedAssetList();
        uint256 supportedAssetCount = supportedAssets.length;

        for (uint16 asset_idx; asset_idx < supportedAssetCount;) {

            address asset = supportedAssets[asset_idx];
            (, , , uint256 updatedAt, ) = AggregatorV3Interface(assetPriceFeed[asset]).latestRoundData();
            require(block.timestamp - updatedAt <= heartbeats[asset] - LOCKING_TIME);

            unchecked {
                ++asset_idx;
            }
        }
    }

This function checks if any of the prices of the assets is going to be updated in the following 30 seconds. If any of the price feeds is going to be updated within this period, it reverts. Notice that the locking time can be any amount of time, so it's up to you. I would recommend to make if a number greater than 15 seconds because the current average Ethereum Mainnet block time is about 12 seconds. Now it is only needed to add this check inside the depositAsset function in LRTDepositPool.sol.

    function depositAsset(
        address asset,
        uint256 depositAmount
    )
        external
        whenNotPaused
        nonReentrant
        onlySupportedAsset(asset)
    {
        // checks
        if (depositAmount == 0) {
            revert InvalidAmount();
        }
        if (depositAmount > getAssetCurrentLimit(asset)) {
            revert MaximumDepositLimitReached();
        }
        // New check
        address lrtOracleAddress = lrtConfig.getContract(LRTConstants.LRT_ORACLE);
        ILRTOracle lrtOracle = ILRTOracle(lrtOracleAddress);
        lrtOracle.lockFrontrunning();
        //

        if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
            revert TokenTransferFailed();
        }

        // interactions
        uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

Assessed type

Oracle

c4-pre-sort commented 11 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 11 months ago

raymondfam marked the issue as duplicate of #32

c4-pre-sort commented 11 months ago

raymondfam marked the issue as not a duplicate

c4-pre-sort commented 11 months ago

raymondfam marked the issue as duplicate of #843

c4-judge commented 11 months ago

fatherGoose1 marked the issue as unsatisfactory: Invalid