Beta Finance Invitational - dirk_y's results

Lines of code

https://github.com/code-423n4/2023-11-betafinance/blob/main/Omni_Protocol/src/OmniPool.sol#L122-L151 https://github.com/code-423n4/2023-11-betafinance/blob/main/Omni_Protocol/src/OmniPool.sol#L266-L276

Vulnerability details

Impact

In Omni, a user can enter multiple markets at the same time. In order to borrow from a market the user first needs to enter sed market. And in the current implementation a user has to repay all borrow balances from all markets before being able to exit a market.

For non-isolated markets this is overly restrictive to users since just because a user has entered a market it doesn't mean that they are exposed to that market. For example, a user might enter 5 markets of which 4 have expiry times 1 year in the future and 1 market has an expiry time of today. In order to avoid the potential of being liquidated, the user has to exit the market that is about to expire, and to do so the user first has to repay all their borrows, even though the other 4 markets are not close to expiry. There is no reason why the user shouldn't just be able to repay the borrows and remove all collateral for the market that is expiring before exiting that market.

The impact of the current behaviour is that users are disincentivised to enter markets that may be expiring soon since when a market a user has entered expires they are open to liquidation; particularly if the user already has borrows outstanding and is actively using the borrowed capital. Furthermore, there is a period during which all the borrows are repaid and thus are not earning interest; the deposited capital of lenders is idle and is not earning yield when it could be.

Proof of Concept

When a user wants to exit a market they call exitMarket in OmniPool:

    function exitMarket(uint96 _subId, address _market) external {
        bytes32 accountId = msg.sender.toAccount(_subId);
        AccountInfo memory account = accountInfos[accountId];
        require(account.modeId == 0, "OmniPool::exitMarkets: In a mode, need to call exitMode.");
        address[] memory markets_ = getAccountPoolMarkets(accountId, account);
        Evaluation memory eval = _evaluateAccountInternal(accountId, markets_, account);
        require(eval.numBorrow == 0, "OmniPool::exitMarkets: Non-zero borrow count.");
        if (_market == account.isolatedCollateralMarket) {
            accountInfos[accountId].isolatedCollateralMarket = address(0);
        } else {
            require(markets_.length > 0, "OmniPool::exitMarkets: No markets to exit");
            require(_contains(markets_, _market), "OmniPool::exitMarkets: Market not entered");
            uint256 newMarketsLength = markets_.length - 1;
            if (newMarketsLength > 0) {
                address[] memory newMarkets = new address[](markets_.length - 1);
                uint256 newIndex = 0;
                for (uint256 i = 0; i < markets_.length; i++) {
                    if (markets_[i] != _market) {
                        newMarkets[newIndex] = markets_[i];
                        ++newIndex;
                    }
                }
                delete accountMarkets[accountId]; // Gas refund?
                accountMarkets[accountId] = newMarkets;
            } else {
                delete accountMarkets[accountId];
            }
        }
        emit ExitedMarket(accountId, _market);
    }

This makes an underlying call to _evaluateAccountInternal and then checks that the returned numBorrows == 0. The relevant part of _evaluateAccountInternal is shown below:

            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
            }

Here, if the borrow amount of a market the user has entered is greater than 0 (we're looping through all the user entered markets) then the numDeposit variable is incremented. Basically, we're counting how many markets the user has borrowed from.

However, like I mentioned above, this is overly restrictive and actually results in lower capital efficiency for lenders since borrowers have to repay their borrow from all markets even if those markets are not expiring.

Below is a diff to the existing test suite that can be executed with forge test --match-test test_ExitMarketsWithNoBorrows that demonstrates how a user can't exit from a market despite having zero borrows and no deposits for that market:

diff --git a/Omni_Protocol/src/tests/TestOmniPool.t.sol b/Omni_Protocol/src/tests/TestOmniPool.t.sol
index bf2d396..3493e7e 100644
--- a/Omni_Protocol/src/tests/TestOmniPool.t.sol
+++ b/Omni_Protocol/src/tests/TestOmniPool.t.sol
@@ -198,6 +198,15 @@ contract TestOmniPool is Test {
         assertEq(info2.isolatedCollateralMarket, address(0), "isolatedCollateralMarket is incorrect");
     }
 
+    function test_ExitMarketsWithNoBorrows() public {
+        test_EnterMarkets();
+
+        oToken.deposit(0, 0, 1000e18);
+        pool.borrow(0, address(oToken), 10e18);
+        vm.expectRevert("OmniPool::exitMarkets: Non-zero borrow count.");
+        pool.exitMarket(0, address(oToken2));
+    }
+
     function test_ClearMarkets() public {
         test_EnterMarkets();
         test_EnterIsolatedMarket();

Tools Used

Manual review + Foundry

Recommended Mitigation Steps

I would suggest updating the exitMarket logic to allow a user to exit a non-isolated market if the user doesn't have any borrows or deposits in that market. This allows borrowers to keep borrowing from different markets (using collateral from other markets) as other markets they've entered expire. Below is a suggested diff and a new test case:

diff --git a/Omni_Protocol/src/OmniPool.sol b/Omni_Protocol/src/OmniPool.sol
index f006770..9b80e32 100644
--- a/Omni_Protocol/src/OmniPool.sol
+++ b/Omni_Protocol/src/OmniPool.sol
@@ -124,13 +124,18 @@ contract OmniPool is IOmniPool, AccessControl, ReentrancyGuardUpgradeable, Pausa
         AccountInfo memory account = accountInfos[accountId];
         require(account.modeId == 0, "OmniPool::exitMarkets: In a mode, need to call exitMode.");
         address[] memory markets_ = getAccountPoolMarkets(accountId, account);
-        Evaluation memory eval = _evaluateAccountInternal(accountId, markets_, account);
-        require(eval.numBorrow == 0, "OmniPool::exitMarkets: Non-zero borrow count.");
         if (_market == account.isolatedCollateralMarket) {
+            Evaluation memory eval = _evaluateAccountInternal(accountId, markets_, account);
+            require(eval.numBorrow == 0, "OmniPool::exitMarkets: Non-zero borrow count.");
             accountInfos[accountId].isolatedCollateralMarket = address(0);
         } else {
             require(markets_.length > 0, "OmniPool::exitMarkets: No markets to exit");
             require(_contains(markets_, _market), "OmniPool::exitMarkets: Market not entered");
+            uint8 borrowTier = getAccountBorrowTier(account);
+            uint256 borrowAmount = IOmniToken(_market).getAccountBorrowInUnderlying(accountId, borrowTier);
+            uint256 depositAmount = IOmniTokenBase(_market).getAccountDepositInUnderlying(accountId);
+            require(borrowAmount == 0 && depositAmount == 0, "OmniPool::exitMarkets: Non-zero borrow or deposit.");
+
             uint256 newMarketsLength = markets_.length - 1;
             if (newMarketsLength > 0) {
                 address[] memory newMarkets = new address[](markets_.length - 1);
diff --git a/Omni_Protocol/src/tests/TestOmniPool.t.sol b/Omni_Protocol/src/tests/TestOmniPool.t.sol
index bf2d396..c9f5b64 100644
--- a/Omni_Protocol/src/tests/TestOmniPool.t.sol
+++ b/Omni_Protocol/src/tests/TestOmniPool.t.sol
@@ -198,6 +198,18 @@ contract TestOmniPool is Test {
         assertEq(info2.isolatedCollateralMarket, address(0), "isolatedCollateralMarket is incorrect");
     }
 
+    function test_ExitMarketsWithNoBorrowsOrDeposits() public {
+        test_EnterMarkets();
+
+        oToken.deposit(0, 0, 1000e18);
+        pool.borrow(0, address(oToken), 10e18);
+        oToken2.deposit(0, 0, 1e18);
+        vm.expectRevert("OmniPool::exitMarkets: Non-zero borrow or deposit.");
+        pool.exitMarket(0, address(oToken2));
+        oToken2.withdraw(0, 0, 0);
+        pool.exitMarket(0, address(oToken2));
+    }
+
     function test_ClearMarkets() public {
         test_EnterMarkets();
         test_EnterIsolatedMarket();
@@ -878,7 +890,7 @@ contract TestOmniPool is Test {
 
         oToken.deposit(0, 0, 1000e18);
         pool.borrow(0, address(oToken), 10e18);
-        vm.expectRevert("OmniPool::exitMarkets: Non-zero borrow count.");
+        vm.expectRevert("OmniPool::exitMarkets: Non-zero borrow or deposit.");
         pool.exitMarket(0, address(oToken));
     }
 
@@ -893,7 +905,7 @@ contract TestOmniPool is Test {
         pool.enterMarkets(0, markets);
         oToken.deposit(0, 0, 1000e18);
         pool.borrow(0, address(oToken), 10e18);
-        vm.expectRevert("OmniPool::exitMarkets: Non-zero borrow count.");
+        vm.expectRevert("OmniPool::exitMarkets: Non-zero borrow or deposit.");
         pool.exitMarket(0, address(oToken));
     }
 

Assessed type

Invalid Validation