0xMR0 - In `ERC4626Price.sol`, `getPriceFromUnderlying()` function is vulnerable to price manipulation

[sherlock-admin]

0xMR0

high

In ERC4626Price.sol, getPriceFromUnderlying() function is vulnerable to price manipulation

Summary

In ERC4626Price.sol, getPriceFromUnderlying() function is vulnerable to price manipulation.This allows an attacker to increase or decrease the price to carry out various attacks against the protocol.

Vulnerability Detail

In ERC4626Price.sol, getPriceFromUnderlying() function is vulnerable to price manipulation because the price can be increased or decreased within a single transaction/block.

    function getPriceFromUnderlying(
        address asset_,
        uint8 outputDecimals_,
        bytes calldata
    ) external view returns (uint256) {
        // Check output decimals
        if (outputDecimals_ > BASE_10_MAX_EXPONENT) {
            revert ERC4626_OutputDecimalsOutOfBounds(outputDecimals_, BASE_10_MAX_EXPONENT);
        }
        uint256 outputScale = 10 ** outputDecimals_;

        // We assume that the asset passed conforms to ERC4626        @audit // vaults are EIP4626 compliant
        ERC4626 asset = ERC4626(asset_);
        address underlying = address(asset.asset());

        if (underlying == address(0)) {
            revert ERC4626_UnderlyingNotSet(asset_);
        }

        uint256 assetScale;
        {
            uint8 assetDecimals = asset.decimals();
            uint8 underlyingDecimals = ERC20(underlying).decimals();
            if (assetDecimals != underlyingDecimals) {
                revert ERC4626_AssetDecimalsMismatch(assetDecimals, underlyingDecimals);
            }

            if (assetDecimals > BASE_10_MAX_EXPONENT) {
                revert ERC4626_AssetDecimalsOutOfBounds(assetDecimals, BASE_10_MAX_EXPONENT);
            }

            assetScale = 10 ** assetDecimals;
        }

@>        uint256 underlyingPerShare = asset.convertToAssets(assetScale).mulDiv(
            outputScale,
            assetScale
        );

        uint256 underlyingPrice = _PRICE().getPrice(underlying);

        uint256 assetPrice = underlyingPrice.mulDiv(underlyingPerShare, outputScale);

        return assetPrice;
    }

As seen in the getPriceFromUnderlying(), the price of the asset of an ERC 4626 vault is dependent on the ERC4626 vault function i.e asset.convertToAssets(assetScale) and outputScale. If the value returned by uint256 underlyingPerShare = asset.convertToAssets(assetScale).mulDiv(outputScale,assetScale); can be manipulated within a single transaction/block, the price of the asset of an ERC4626 vault is considered to be vulnerable to price manipulation.

Per the EIP-4626 security considerations:

The preview methods return values that are as close as possible to exact as possible. For that reason, they are manipulable by altering the on-chain conditions and are not always safe to be used as price oracles. This specification includes convert methods that are allowed to be inexact and therefore can be implemented as robust price oracles. For example, it would be correct to implement the convert methods as using a time-weighted average price in converting between assets and shares.

It means that previewRedeem and convertToAssets can not be used for oracle pricing, instead TWAP should be used.

The vaults to be used in contracts is supposed to be compliant to EIP-4626(referencing L-96).

Most of the such vault contracts use openzeppelin contracts as it is widely used in contracts instead of creating own as openzeppelin ERC4626.sol is EIP4626 compliant.

If we see previewRedeem() and convertToAssets() functions from openzeppelin ERC4626.sol,(We are discussing about these functions to get the context of TWAP requirement per EIP4626)

    function previewRedeem(uint256 shares) public view virtual override returns (uint256) {
        return _convertToAssets(shares, Math.Rounding.Down);
    }

and

    function convertToAssets(uint256 shares) public view virtual override returns (uint256) {
        return _convertToAssets(shares, Math.Rounding.Down);
    }

Both of these functions return _convertToAssets(shares, Math.Rounding.Down) which looks as below,

    function _convertToAssets(uint256 shares, Math.Rounding rounding) internal view virtual returns (uint256) {
        return shares.mulDiv(totalAssets() + 1, totalSupply() + 10 ** _decimalsOffset(), rounding);
    }

Within the _convertToAssets(), the number of assets per share is calculated based on the current total assets and current supply that can be increased or decreased within a single block/transaction by calling the vault's deposit, mint, withdraw or redeem functions. This allows the attacker to artificially inflate or deflate the price within a single block/transaction.

Reference: Similar issue found as Medium severity at sherlock audit and the reference while drafting this issue can be checked here

Impact

The attacker could perform price manipulation to make the apparent value of an asset to be much higher or much lower than the true value of the asset. OR another impact is the flash loan may skew the price oracle, leading to the mispricing of assets, leading to loss of funds.

Code Snippet

https://github.com/sherlock-audit/2023-11-olympus/blob/main/bophades/src/modules/PRICE/submodules/feeds/ERC4626Price.sol#L125

Tool used

Manual Review

Recommendation

Consider implementing TWAP so that the price cannot be inflated or deflated within a single block/transaction or within a short period of time.

Note: This is also a recommendation of EIP-4626 mentioned in security considerations.

[0xJem]

FYI this is a configuration issue - getPrice() on the underlying asset needs to have a pool-level TWAP or internal moving average

[nevillehuang]

Invalid, in fact to add, the security considerations highlighted in EIP4626 is exactly mentioning that the convert methods i.e. convertToAssets can be implemented as price oracle which is exactly what olympus uses, on the condition that no mechanisms allow price manipulation as sponsor mentioned.