Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The lack of flexibility of getOracleAssetPrice() would prevent the listing of some widely known assets #60

Open
hats-bug-reporter bot opened this issue Jul 3, 2023 · 1 comment
Labels
bug Something isn't working wontfix This will not be worked on

Comments

@hats-bug-reporter
Copy link

Github username: @abhishekvispute
Submission hash (on-chain): 0xf7b114c1b3f218a7c9032ddefce0231b918afcab67bf1064112a54c230ac75d4
Severity: low severity

Description:
Description
VMEX uses the Chainlink price feed to obtain the latest price data and get a base price for all assets. However, there are multiple instances where price derivation needs to occur explicitly. For instance, there is no Chainlink price feed for WBTC/USD on Mainnet, so you need to derive it using WBTC/BTC and BTC/USD. Similarly, there is no dedicated price feed for Coinbase Wrapped Staked ETH (CBETH) on Optimism, so you need to derive it using CBETH/ETH and ETH/USD.

function getOracleAssetPrice(address asset) internal returns (uint256){
        IChainlinkPriceFeed source = _assetsSources[asset].feed;
        if (address(source) == address(0)) {
            return _fallbackOracle.getAssetPrice(asset);
        } else {
            (
                /* uint80 roundID */,
                int256 price,
                /*uint startedAt*/,
                uint256 updatedAt,
                /*uint80 answeredInRound*/
            ) = IChainlinkPriceFeed(source).latestRoundData(); // @audit wont work for tokens like WBTC
            IChainlinkAggregator aggregator = IChainlinkAggregator(IChainlinkPriceFeed(source).aggregator());
            if (price > int256(aggregator.minAnswer()) && 
                price < int256(aggregator.maxAnswer()) && 
                block.timestamp - updatedAt < _assetsSources[asset].heartbeat
            ) {
                return uint256(price);
            } else {
                return _fallbackOracle.getAssetPrice(asset);
            }
        }
}

The current code of getOracleAssetPrice is not flexible enough to allow for these derivations, forcing VMEX to either drop these widely used assets or deploy their own price feed contract that mimics Chainlink data.

Attack Scenario
NA

Recommendation
One solution is to define a separate contract for each asset and create a price feed per asset. This can be fetched inside the VMEX oracle. However, this approach could be stretch to implement for all assets.

Another option is to have a switch between a simple chainlink fetch and a derived one.

Alternatively, you can leave it as it is and, for certain assets, have a separate contract with the same function signatures as the chainlink aggregator and can then derive the prices from real chainlink price feeds there.

@hats-bug-reporter hats-bug-reporter bot added the bug Something isn't working label Jul 3, 2023
@ksyao2002
Copy link

Thanks for the submission. For the cases where we want to add a new oracle that our current framework can't cover, our protocol was to upgrade the VMEXOracle contract and include a new DataTypes.ReserveAssetType to handle. For now, we won't fix this since it depends on what assets we choose to include.

@ksyao2002 ksyao2002 added the wontfix This will not be worked on label Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

1 participant