search

sherlock-audit / 2024-05-midas-judging

13 stars 6 forks source link

serial-coder - `DataFeed::getDataInBase18()` can report stale prices, allowing depositors to deposit USDC amounts under the protocol's requirement #150

Closed sherlock-admin2 closed 5 months ago

sherlock-admin2 commented 6 months ago

serial-coder

medium

DataFeed::getDataInBase18() can report stale prices, allowing depositors to deposit USDC amounts under the protocol's requirement

Summary

The hardcoded stale period of 3 days is too large, eventually causing the DataFeed contract to report stale prices. Subsequently, depositors can deposit less USDC than required, breaking the DepositVault::deposit()'s core invariant.

Vulnerability Detail

In the DataFeed contract, the _HEALTHY_DIFF constant is set to 3 days (@1 in the snippet below). In other words, the DataFeed::_getDataInBase18() (@2) will consider the EUR/USD price data fed by Chainlink's price feed aggregator to be stale only after the last update time has elapsed 3 days.

Since the Midas protocol supports Ethereum and Arbitrum chains, consider the EUR/USD oracles on both chains.

As you can see, the hardcoded 3 days of a stale period is too large for both chains. If the aggregator cannot properly feed the price data, the DataFeed contract cannot detect stale prices, eventually causing the DataFeed::getDataInBase18() (@3) to return stale prices.

    contract DataFeed is WithMidasAccessControl, IDataFeed {
        ...

        //@audit -- The DataFeed contract uses a hardcoded stale period of 3 days to verify the staleness of price data.
@1      uint256 private constant _HEALTHY_DIFF = 3 days;

        ...

        function getDataInBase18() external view returns (uint256 answer) {
            //@audit -- As per @2, the getDataInBase18() can feed its caller stale EUR/USD prices.
@3          (, answer) = _getDataInBase18();
        }

        ...

        function _getDataInBase18()
            private
            view
            returns (uint80 roundId, uint256 answer)
        {
            uint8 decimals = aggregator.decimals();
            (uint80 _roundId, int256 _answer, , uint256 updatedAt, ) = aggregator
                .latestRoundData();
            require(_answer > 0, "DF: feed is deprecated");
            require(
                // solhint-disable-next-line not-rely-on-time

                //@audit -- The _HEALTHY_DIFF constant (3 days) is used to verify the staleness of the fed EUR/USD price data.
                //          However, the heartbeat of the EUR/USD price feed is only 24 hours on Ethereum and 1 hour on Arbitrum. 
                //          If the aggregator cannot feed the price data properly, the DataFeed contract cannot detect stale prices, 
                //          causing the price caller to consume stale prices.
@2              block.timestamp - updatedAt <= _HEALTHY_DIFF,

                "DF: feed is unhealthy"
            );
            roundId = _roundId;
            answer = uint256(_answer).convertToBase18(decimals);
        }
    }

The DepositVault::minAmountToDepositInUsd() can consume the stale EUR/USD price to calculate the minAmountToDepositInUsd variable (@4 in the snippet below). If the stale price is less than the actual (non-updated) price, depositors can deposit less USDC than required (@5).

As a result, the stale price can break the DepositVault::deposit()'s core invariant (@6).

    function minAmountToDepositInUsd() public view returns (uint256) {
        return
            //@audit -- The stale price can be consumed by the minAmountToDepositInUsd() to calculate the minAmountToDepositInUsd variable.
@4          (minAmountToDepositInEuro * eurUsdDataFeed.getDataInBase18()) /
            10**18;
    }

    ...

    function _validateAmountUsdIn(address user, uint256 amountUsdIn)
        internal
        view
    {
        if (totalDeposited[user] != 0) return;
        require(
            //@audit -- If the stale price is less than the actual (non-updated) price, depositors can deposit less USDC than required.
@5          amountUsdIn >= minAmountToDepositInUsd(),

            "DV: usd amount < min"
        );
    }

    ...

    function deposit(address tokenIn, uint256 amountUsdIn)
        external
        onlyGreenlisted(msg.sender)
        whenNotPaused
    {
        address user = msg.sender;

        _requireTokenExists(tokenIn);

        lastRequestId.increment();
        uint256 requestId = lastRequestId.current();

        if (!isFreeFromMinDeposit[user]) {
            //@audit -- The stale price can break the deposit()'s core invariant.
@6          _validateAmountUsdIn(user, amountUsdIn);
        }
        require(amountUsdIn > 0, "DV: invalid amount");

        totalDeposited[user] += amountUsdIn;
        _tokenTransferFromUser(tokenIn, amountUsdIn);

        emit Deposit(requestId, user, tokenIn, amountUsdIn);
    }

Impact

The stale EUR/USD prices can allow depositors to deposit USDC amounts under the required minAmountToDepositInUsd, breaking the DepositVault::deposit()'s core invariant.

Code Snippet

Tool used

Manual Review

Recommendation

The heartbeat of the EUR/USD price feed aggregator is ~24 hours on Ethereum and ~1 hour on Arbitrum. Hence, apply an appropriate stale period (i.e., _HEALTHY_DIFF) for each chain.

Duplicate of #110

WangSecurity commented 5 months ago

This report will be invalidated based on the escalation on #110.