c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-11-betafinance/blob/main/Omni_Protocol/src/OmniOracle.sol#L45-L47

Vulnerability details

Impact

The real market's value of the borrows and deposits will be messed up because the existing implementation of the OmniOracle will request the price of the ETH/USD pair when the address of the contract requesting the price matches the address assigned as the WETH contract in the bnb chain, instead of requesting the price of the BNB/USD pair, the contract request the price of the ETH/USD pair, this will end up causing that the price for 1 BNB to be considered as if it were the price of 1 ETH in the real market.

Proof of Concept

The problem is located in the OmniOracle::gePrice() function, when the config.provider is the Band Oracle and the underlying asset requesting the price is the address assigned to the WETH contract (The address of the wrapped native token), for mainnet, this is okay because in mainnet the native token is ETH, so, requesting the price of the ETH/USD price when the underlying is the weth contract will correctly returns the price of the native token (ETH), but, for the BNBChain, the native token is not ETH, its BNB, the existing implementation will incorrectly request the price of the ETH/USD pair, instead of requesting the price of the BNB/USD pair, this will cause that all the deposited and borrowed BNB to be priced at the price of the ETH asset, instead of the price of the BNB asset.
- As stated in the Readme's contest, these contracts will be deployed to the BNB chain too.
Let's analyze the code of the function that requests the price.

OmniOracle contract

//@audit-info => Returns the price in a scale of 1e36 of the given _underlying asset
function getPrice(address _underlying) external view returns (uint256) {
OracleConfig memory config = oracleConfigs[_underlying];
if (config.provider == Provider.Band) {
IStdReference.ReferenceData memory data;
if (_underlying == WETH) {
//@audit-issue => If the underlying asset matches the address of the wrapped native token contract, it will request the price of the ETH/USD pair
//@audit-info => For mainnet, it's okay to request the price of ETH/UST
//@audit-issue => In the BNB chain, requesting the price of ETH/USD will cause each BNB to be considered worth 1ETH
//@audit-issue => In the BNB chain is required to request the price of the BNB/USD pair.
data = IStdReference(config.oracleAddress).getReferenceData("ETH", USD);
} else {
data = IStdReference(config.oracleAddress).getReferenceData(IERC20Metadata(_underlying).symbol(), USD);
}
...
}
...
...
}

As demonstrated in the above code, in the BNB Chain, when requesting the price of the wrapped native token, the returned price will be the price of the ETH/USD pair, instead of the price of the BNB/USD pair, this will end up inflating the real value of each BNB and will mess up the borrow and deposits accounting.
A problem that can arise from this bug is users who deposit BNB as collateral will be able to borrow more than they should because of the price difference between ETH and BNB, so, these users can take bigger loans in other markets using the BNB as collateral. Also, for users who borrow BNB, their debt will be worth more than what it really should, thus, demanding more collateral to not be liquidated because their account becomes unhealthy.

Tools Used

Manual Audit

Recommended Mitigation Steps

The suggested mitigation for this problem, and to not increase the gas consumption each time the price of the native token is requested, the suggestion is to create a new constant similar to the current WETH contract address, where it needs to be explicitly updated the value to the correct native token (similar to the address of the WETH contract, where depending on the chain, the correct contract address needs to be updated)
```
contract OmniOracle is IOmniOracle, AccessControl, Initializable {
uint256 public constant PRICE_SCALE = 1e36; // Gives enough precision for the price of one base unit of the token, as most tokens have at most 18 decimals
string public constant USD = "USD";
address public constant WETH = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2; // Need to hardcode WETH address per network deployed for Band
```
string public constant NATIVE_TOKEN_TICKET = "ETH"; //Need to hardcode the ticket of the native token per network deployed, for BNB chain, the ticket must be "BNB"
...

function getPrice(address _underlying) external view returns (uint256) { OracleConfig memory config = oracleConfigs[_underlying]; if (config.provider == Provider.Band) { IStdReference.ReferenceData memory data; if (_underlying == WETH) {
data = IStdReference(config.oracleAddress).getReferenceData("ETH", USD);
data = IStdReference(config.oracleAddress).getReferenceData(NATIVE_TOKEN_TICKET, USD); } else { data = IStdReference(config.oracleAddress).getReferenceData(IERC20Metadata(_underlying).symbol(), USD); } ... } ... } }

Assessed type

Context

JeffCX commented 11 months ago

duplicate of #22

thereksfour commented 11 months ago

There is an assumption that WETH will be set to WBNB address on BSC, will consider QA, and leave it to sponsors

c4-judge commented 11 months ago

thereksfour marked the issue as primary issue

stalinMacias commented 11 months ago

@thereksfour more than an assumption that WETH will be set to WBNB address on BSC, it is a fact, first of all, there is a comment in the code that explictly acknowledges this fact.

And supposing that the address of the WETH contract is not updated and leave it as it is, this would even be a bigger problem, there is no a WETH contract in the BNB chain, so, that means that all the deposited native token (BNB) will be unnusable, the reason is because the WBNB contract won't match the current hardcoded address of the WETH contract, so, the request to the Band oracle will send the symbol assigned in the WBNB contract, which is "WBNB", check it here, which will cause the tx to revert because the Band oracle doesn't support the WBNB/USD pair, check here, it only supports the BNB/USD Pair

    function getPrice(address _underlying) external view returns (uint256) {
        OracleConfig memory config = oracleConfigs[_underlying];
        if (config.provider == Provider.Band) {
            IStdReference.ReferenceData memory data;
//@audit-issue => If the hardcoded WETH address is not updated to the WBNB address in the BNB chain, when the OmniPool requests the price for the WBNB contract it won't enter this condition, it will go with the code in the else block
            if (_underlying == WETH) {
                data = IStdReference(config.oracleAddress).getReferenceData("ETH", USD);
            } else {
//@audit-issue => If the Oracle requests the price of the WBNB/USD pair, the tx will revert because that pair is not supported in the Band oracle, it only supports the BNB/USD pair
                data = IStdReference(config.oracleAddress).getReferenceData(IERC20Metadata(_underlying).symbol(), USD);
            }
   ...

So, I still think this problem deserves a medium severity.

thereksfour commented 11 months ago

As discussed in #22, I don't think Band Oracle will be used on BSC to get prices for ETH or BNB as they are unsupported.

allenjlee commented 11 months ago

As mentioned in #22 I believe there was a QA report #23 that brought this up as well. I agree, the oracle implementation is not the cleanest, but I think it just puts more onus on the deployer to make sure that things are deployed correctly. I don't really think this is a medium severity, and more of a QA issue. I think the argument here is mostly that we shouldn't put so much dependency on the deployer with the contracts, which is a fair point.

Also regarding lack of support by Band, this is explicitly why we also handle support with Chainlink. It's not like all market addresses must be supported by Band. We will use either Chainlink or Band depending on what is available and reliable.

c4-sponsor commented 11 months ago

allenjlee marked the issue as disagree with severity

c4-sponsor commented 11 months ago

allenjlee (sponsor) acknowledged