_evaluateAccountInternal() may Index out of bounds

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-betafinance/blob/0f1bb077afe8e8e03093c8f26dc0b7a2983c3e47/Omni_Protocol/src/OmniPool.sol#L266-L267

Vulnerability details

Vulnerability details

_evaluateAccountInternal() returns the user's current deposits, current borrowings, and other important factors. It is mainly used in several places, such as borrow(), withdraw(), liquidate(), and other business methods.

Inside the method, a very important function is to calculate the current user's total borrowing, mainly through the borrowTier, loop through all the markets, and through the market getAccountBorrowInUnderlying() to accumulate the total borrowing. The implementation is as follows：

    function _evaluateAccountInternal(bytes32 _accountId, address[] memory _poolMarkets, AccountInfo memory _account)
        internal
        returns (Evaluation memory eval)
    {
...
@>          uint8 borrowTier = getAccountBorrowTier(_account);
@>          uint256 borrowAmount = IOmniToken(market).getAccountBorrowInUnderlying(_accountId, borrowTier);
            if (borrowAmount != 0) {
                ++eval.numBorrow;
                uint256 borrowValue = (borrowAmount * price) / PRICE_SCALE; // Rounds down
                eval.borrowTrueValue += borrowValue;
                uint256 borrowFactor = marketCount == 1
                    ? SELF_COLLATERALIZATION_FACTOR
                    : _account.modeId == 0 ? uint256(marketConfiguration_.borrowFactor) : uint256(mode.borrowFactor);
                eval.borrowAdjValue += (borrowValue * FACTOR_PRECISION_SCALE) / borrowFactor; // Rounds down
            }
        }

    function getAccountBorrowInUnderlying(bytes32 _account, uint8 _borrowTier) external view returns (uint256) {
@>      OmniTokenTranche storage tranche = tranches[_borrowTier];
        uint256 share = trancheAccountBorrowShares[_borrowTier][_account];
        if (share == 0) {
            return 0;
        } else {
            return Math.ceilDiv(share * tranche.totalBorrowAmount, tranche.totalBorrowShare);
        }
    }

The current implementation of getAccountBorrowInUnderlying() does not determine whether _borrowTier is greater than or equal to tranches.length. This may result in an array Index out of bounds when the getAccountBorrowInUnderlying() method executes tranches[_borrowTier];.

Example.

1. Alice has isolatedCollateral = market_A and marketConfigurations[market_A].riskTranche = 2
2. pool has market_K market_K.tranches.length =2

so

1. this way Alice 's _borrowTier will 2 : getAccountBorrowInUnderlying(Alice) == 2
2. Alice entry into the market market_K, Alice call enterMarkets(market_K)
3. _evaluateAccountInternal(Alice) will revert with Index out of bounds

Another reason is that enterMarkets() does not check that the trancheCount of the market to be entered is greater than the user's current borrowTier.

Impact

When a user is liquidated, it is possible to maliciously enter a market smaller than _borrowTier to prevent liquidation Or the user accidentally enters such a market Since _evaluateAccountInternal() is always revert, resulting in the user not being able to borrow/withdraw/liquidate, the funds may be locked in the contract

Proof of Concept

add to TestOmniPool.t.sol

The above code demonstrates that the scenario described above, the _evaluateAccountInternal() will Index out of bounds

    function test_EvaluateAccountRevert() public {
        //1. have 2 Tier
        uint256[] memory borrowCaps = new uint256[](2);
        borrowCaps[0] = 1e9 * (10 ** uToken.decimals());
        borrowCaps[1] = 1e3 * (10 ** uToken.decimals());
        OmniToken oToken5 = new OmniToken();
        oToken5.initialize(address(pool), address(uToken), address(irm), borrowCaps);
        IOmniPool.MarketConfiguration memory mConfig5 =
            IOmniPool.MarketConfiguration(0.4e9, 0, uint32(block.timestamp + 1000 days), 2, false);
        pool.setMarketConfiguration(address(oToken5), mConfig5);
        //2. pool have IsolatedMarket, riskTranche = 2
        IOmniPool(pool).enterIsolatedMarket(0, address(oToken3));         
        bytes32 account = address(this).toAccount(0);
        IOmniPool.AccountInfo memory info = _getAccountInfo(account);        
        console.log("borrowTier:",pool.getAccountBorrowTier(info));               
        //3. is's ok if without oToken5
        address[] memory markets = new address[](1);
        markets[0] = address(oToken);
        pool.enterMarkets(0, markets);
        pool.isAccountHealthy(account);
        //4.enter  oToken5 will revert Index out of bounds 
        markets[0] = address(oToken5);
        pool.enterMarkets(0, markets);
        pool.isAccountHealthy(account);
    }

$ forge test -vvv --match-test test_EvaluateAccountRevert
Running 1 test for src/tests/TestOmniPool.t.sol:TestOmniPool

[FAIL. Reason: Index out of bounds] test_EvaluateAccountRevert() (gas: 3122844)
Logs:
  borrowTier: 2

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 7.01ms
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in src/tests/TestOmniPool.t.sol:TestOmniPool
[FAIL. Reason: Index out of bounds] test_EvaluateAccountRevert() (gas: 3122844)

Recommended Mitigation

suggest

1. enterMarkets()Adding a judgment cannot be smaller than the current BorrowTier
2. getAccountBorrowInUnderlying() if _borrowTier <=tranches.length return 0

Assessed type

Decimal

[JeffCX]

I think this has to do with admin misconfiguration

The current implementation of getAccountBorrowInUnderlying() does not determine whether _borrowTier is greater than or equal to tranches.length.

the admin should increase and create more tranches and then set the mode

or it requires the admin to set a tranche index in mode that is out of bounds in the first place

[c4-judge]

thereksfour marked the issue as primary issue

[JeffCX]

When a user is liquidated, it is possible to maliciously enter a market smaller than _borrowTier to prevent liquidation

this looks valid

suppose the borrower tier is 10

user can enter a market after borrow with the max tranche id 5

and transaction revert in index out of bounds error when accessing index 10 if the market max tranche id 5

[c4-judge]

thereksfour marked the issue as satisfactory

[stalinMacias]

I'd like to add my thoughts about this, because for this issue to be possible it means that the configurations were set incorrectly, that means, the admins did a misstake while setting up the market configs.

suppose the borrower tier is 10 user can enter a market after borrow with the max tranche id 5

If the borrowTier assigned to an IsolatedCollateral is greather than the max tranch ID of the OmniTokens, isn't this a market configuration error? The admin did the misstake of setting a higher borrowTier to an IsollatedCollateral than the highest existing tier of the OmniTokens.

As a matter of fact, if this is the case, no borrows would be possible,

OmniPool contract

function borrow(uint96 _subId, address _market, uint256 _amount) external whenNotPaused {
bytes32 accountId = msg.sender.toAccount(_subId);
AccountInfo memory account = accountInfos[accountId];
address[] memory poolMarkets = getAccountPoolMarkets(accountId, account);
require(_contains(poolMarkets, _market), "OmniPool::borrow: Not in pool markets.");

//@audit-info => Will get the borrowTier assigned to the user, suppose that is using an IsollatedCollateral, and riskTier is set to 10 uint8 borrowTier = getAccountBorrowTier(account);

//@audit-info => Will try to borrow from the specified omnitoken market from the Tranch 10, and suppose the maxTier in this OmniToken is 5. The tx will revert because the trancheBorrowCaps assigned to tranch10 is not configured, thus is 0, check below block of code.... IOmniToken(_market).borrow(accountId, borrowTier, _amount);

accounted as debt when computing new borrows in a new tier! Evaluation memory eval = _evaluateAccountInternal(accountId, poolMarkets, account);

    require(eval.depositAdjValue >= eval.borrowAdjValue && !eval.isExpired, "OmniPool::borrow: Not healthy after borrow.");
}

> OmniToken contract

function borrow(bytes32 _account, uint8 _trancheId, uint256 _amount)
    external
    nonReentrant
    returns (uint256 share)
{
    ...

//@audit-info => Will validate if the requested amount to borrow surpasses the trancheBorrowCap, but tranch10 is not configured in this market, it doesn't even exists, so, the tranchBorrowCap will be 0, thus, tx will be reverted! require(totalBorrowAmount_ + _amount <= trancheBorrowCaps[_trancheId], "OmniToken::borrow: Borrow cap reached.");

    ...
}

So, if the riskTier of an isollatedCollateral is greather than any of the highest existing and configured tranches of the OmniToken markets, it was because the admins did a misstake when assigning the riskTier of the isollated collateral.

Based on this, I think this is a low-info severity given the fact that the admin needs to make a misstake while setting up the riskTiers.

[JeffCX]

admin can set tranche to increase the tranche count if user wants to escape liquidation. so this ultimately falls into admin configuration error

[thereksfour]

suppose the borrower tier is 10 user can enter a market after borrow with the max tranche id 5

What warden means is that the user borrows in the market with max tranche id 10 and then enters the market with max tranche id 5.

And @allenjlee, is this misconfiguration possible?

If possible I would consider this M as this can be recovered by admin. If not possible, this would be considered QA due to admin error.

[JeffCX]

Want to share some new thoughts

Personally I thought it is possible,

If possible I would consider this M as this can be recovered by admin.

suppose the borrower tier is 10 user can enter a market after borrow with the max tranche id 5

could be when admin create a new market and does not set all tranche count yet

but admin can quickly fix the configuration by increase the tranche count to borrow tier and liquidate user that use this problem to avoid liquidation, once they are liquidated, they are liquidated

also in my QA report

https://github.com/code-423n4/2023-11-betafinance-findings/blob/main/data/ladboy233-Q.md

the first issue is

when admin set the mode, should validate if duplicate market exists

if there are duplicate market in the mode, the accounting is broken, user's deposit and borrow is count twice, leads to either overborrow (if deposit value > debt) or fast liquidation (if debt > deposit value)

and when admin make this mistake, admin cannot recover and modify the mode market setting,

should we can consider that issue a medium? no because that so heavily relies on admin issue

and if we do not consider something that is non-recoverable admin mistake as medium

don't think we should consider something that is recoverable admin mistake as medium

[thereksfour]

True, the "possible" above means that the admin may not be aware of the issue, which leads to such configurations existing in markets. And I think the issue is very insidious, so maybe the admin didn't notice it. And, for the duplicate markets issue, please check TRST-H-3 enterMarkets() allows users to enter duplicate markets and CR-2 setModeConfiguration() does not check for duplicate markets, I think it's a QA issue that requires admin error.

[stalinMacias]

the admin may not be aware of the issue, which leads to such configurations existing in markets.

@thereksfour
How are we supposed to know if admins are aware of such things? Isn't assumed that actions performed and data send by trusted parties (especially admins) supposed to be valid data, (by valid I just don't mean not fake data, but data/configurations that have been sanitized, verified and validated to not cause a bad impact)

I think that adding new markets to the protocol is one of the key and most important actions that the admins will do, and, yes, the OmniToken could be initialized with less tranches than one of the risk tranches of any of the collateral markets, but, the OmniToken can be entered by the users until the admin register and setups the new OmniToken in the OmniPool contract, so, at this point they are supposed to have configured the market to be compatible with the rest of markets, which that includes the fact to have validated the total tranches for this market, the borrowCaps and all other essential configurations of the market.

I would say that it could be a medium if the admin error would not be possible to be fixed, like, causing a permanent DOS for the users, but this problem is not the case, supposing that the admin did the mistake of setting up an incorrect number of tranches, the mistake can be solved right away, just a matter of adjusting the configurations to what it should be (which it should've been done since the beginning)

[thereksfour]

Admins may not realize that this is a misconfiguration, for example they may want to have a market with only 1 tranche, so we need to wait for the sponsor's thoughts.

[allenjlee]

suppose the borrower tier is 10 user can enter a market after borrow with the max tranche id 5

What warden means is that the user borrows in the market with max tranche id 10 and then enters the market with max tranche id 5.

And @allenjlee, is this misconfiguration possible?

If possible I would consider this M as this can be recovered by admin. If not possible, this would be considered QA due to admin error.

This shouldn't happen normally, would be admin error. There shouldn't be a mismatch between the max trancheCount and the riskTranche assigned that influences the borrowTier, i.e. the trancheCount for all borrowable markets should be equal, and the max riskTranche should be less than the trancheCount.

If the markets are misconfigured, then it must be prior to any borrows taking place. Therefore, it will also not be possible to borrow, as the user is not able to pass in the specific tranche they are borrowing from. The tranche they borrow from is determined by the borrowTier, and it will be invalid if trancheCount <= borrowTier.

This will however cause freezing of funds if markets are misconfigured, but is recoverable by the admin setting the riskTranche to the appropriate tier. I added the check as the warden suggested to return 0 if _borrowTier >= trancheCount.

I will defer to judge on the severity of this

[c4-sponsor]

allenjlee (sponsor) disputed

[c4-sponsor]

allenjlee (sponsor) acknowledged

[c4-sponsor]

allenjlee (sponsor) confirmed

[thereksfour]

As per sponsor's comment, it will be downgraded to QA due to adminr error required

[c4-judge]

thereksfour changed the severity to QA (Quality Assurance)