0x73696d616f - Profitable liquidations and accumulation of bad debt due to earnings accumulator not being triggered before liquidating

[sherlock-admin3]

0x73696d616f

high

Profitable liquidations and accumulation of bad debt due to earnings accumulator not being triggered before liquidating

Summary

The earnings accumulator is not updated and converted to floatingAssets pre liquidation, leading to an instantaneous increase of balance of the liquidatee if it has shares which causes a profitable liquidation and the accumulation of bad debt.

Vulnerability Detail

Market::liquidate() fetches the balance and debt of a user and calculates the amount to liquidate based on them to achieve a target health, or if not possible, seize all the balance of the liquidatee, to get as much collateral as possible. Then Auditor::handleBadDebt() is called in the end if the user still had debt but no collateral.

However, the protocol does not take into account that the liquidatee will likely have market shares due to previous deposits, which will receive the pro-rata lendersAssets and debt from the penaltyRate if the maturity date of a borrow was expired.

Thus, in Auditor::checkLiquidation(), it calculates the collateral based on totalAssets(), which does not take into account an earningsAccumulator increase due to the 2 previously mentioned reasons, and base.seizeAvailable will be smaller than supposed. This means that it will end up convering the a debt and collateral balance to get the desired ratio (or the assumed maximum collateral), but due to the earningsAccumulator, the liquidatee will have more leftover collateral.

This leftover collateral may allow the liquidatee to redeem more net assets than it had before the liquidation (as the POC will show), or if the leftover collateral is still smaller than the debt, it will lead to permanent bad debt. In any case, the protocol takes a loss in favor of the liquidatee.

Add the following test to Market.t.sol:

function test_POC_ProfitableLiquidationForLiquidatee_DueToEarningsAccumulator() external {
  uint256 maturity = FixedLib.INTERVAL * 2;
  uint256 assets = 10_000 ether;

  // BOB adds liquidity for liquidation
  vm.prank(BOB);
  market.depositAtMaturity(maturity + FixedLib.INTERVAL * 1, 2*assets, 0, BOB);

  // ALICE deposits and borrows
  ERC20 asset = market.asset();
  deal(address(asset), ALICE, assets);
  vm.startPrank(ALICE);
  market.deposit(assets, ALICE);
  market.borrowAtMaturity(maturity, assets*78*78/100/100, type(uint256).max, ALICE, ALICE);
  vm.stopPrank();

  // Maturity is over and some time has passed, accruing extra debt fees
  skip(maturity + FixedLib.INTERVAL * 90 / 100);

  // ALICE net balance before liquidation
  (uint256 collateral, uint256 debt) = market.accountSnapshot(address(ALICE));
  uint256 preLiqCollateralMinusDebt = collateral - debt;

  // Liquidator liquidates
  address liquidator = makeAddr("liquidator");
  deal(address(asset), liquidator, assets);
  vm.startPrank(liquidator);
  asset.approve(address(market), type(uint256).max);
  market.liquidate(ALICE, type(uint256).max, market);
  vm.stopPrank();

  // ALICE redeems and asserts that more assets were redeemed than pre liquidation
  vm.startPrank(ALICE);
  market.repayAtMaturity(maturity, type(uint256).max, type(uint256).max, ALICE);
  uint256 redeemedAssets = market.redeem(market.balanceOf(ALICE) - 1, ALICE, ALICE);

  assertEq(preLiqCollateralMinusDebt, 802618844937982683756);
  assertEq(redeemedAssets, 1556472132091811191541);
  assertGt(redeemedAssets, preLiqCollateralMinusDebt);
}

Impact

Profitable liquidations for liquidatees, who would have no incentive to repay their debt as they could just wait for liquidations to profit. Or, if the debt is already too big, it could lead to the accumulation of bad debt as the liquidatee would have remaining collateral balance and Auditor::handleBadDebt() would never succeed.

Code Snippet

https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/Market.sol#L514
https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/Market.sol#L552
https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/Market.sol#L599
https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/Market.sol#L611
https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/Market.sol#L925
https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/Auditor.sol#L219

Tool used

Manual Review

Vscode

Foundry

Recommendation

Add the following line to the begginning of Market::liquidate():
floatingAssets += accrueAccumulatedEarnings();
This will update lastAccumulatorAccrual, so any increase in earningsAccumulator to lenders will not be reflected in totalAssets(), and the liquidatee will have all its collateral seized.

[0x73696d616f]

Escalate

This issue is of high severity as it leads to loss of funds and has no specific pre conditions.

It will never allow clearing bad debt as the liquidatee will always have shares in the last market it is in, receiving a portion of lendersAssets at the end of the liquidation, which will mean it will never have exactly 0 collateral and the debt is not cleared via Auditor::handleBadDebt(). This breaks the core mechanism of clearing bad debt and will make it grow out of bounds and the protocol will likely become insolvent.

[sherlock-admin3]

Escalate

This issue is of high severity as it leads to loss of funds and has no specific pre conditions.

It will never allow clearing bad debt as the liquidatee will always have shares in the last market it is in, receiving a portion of lendersAssets at the end of the liquidation, which will mean it will never have exactly 0 collateral and the debt is not cleared via Auditor::handleBadDebt(). This breaks the core mechanism of clearing bad debt and will make it grow out of bounds and the protocol will likely become insolvent.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[santipu03]

This issue was originally marked as a medium instead of high because anyone can liquidate a position twice, thus solving this issue. When this issue is triggered and the borrower is left with some dust collateral and more debt, the liquidator can simply liquidate again the position, seizing the last shares of collateral.

Given that the protocol will have a liquidation bot that will liquidate all unhealthy positions, this issue will probably never be triggered because the bot will liquidate all unhealthy positions regardless of the dust amount of collateral they have.

[0x73696d616f]

This issue was originally marked as a medium instead of high because anyone can liquidate a position twice, thus solving this issue. When this issue is triggered and the borrower is left with some dust collateral and more debt, the liquidator can simply liquidate again the position, seizing the last shares of collateral.
Given that the protocol will have a liquidation bot that will liquidate all unhealthy positions, this issue will probably never be triggered because the bot will liquidate all unhealthy positions regardless of the dust amount of collateral they have.

As the user will have shares left, the second liquidation will suffer from the same issue.

This issue also impacts seizing less collateral than supposed.

[santipu03]

As the user will have shares left, the second liquidation will suffer from the same issue.

Not if the second liquidation is executed in the same block.

In my opinion, this issue warrants medium severity because it has extensive limitations to be triggered. First of all, it requires a loan that has some bad debt, which is highly unlikely given that the liquidation bot will always liquidate borrowers before they can generate bad debt. Second, it requires that no one has interacted with the protocol on the same block as the liquidation. Third, a liquidator can execute the liquidation twice to easily go around this issue.

Summarizing, this issue requires certain conditions or specific states to be triggered so it qualifies for medium severity.

[0x73696d616f]

Not if the second liquidation is executed in the same block.

This is true but liquidations will not be executed twice in a row and if they are, they are also less likely to be in the same block. The likelihood is still 99% of triggering this. The reason they will not liquidate twice is because it is not profitable, only dust will be left.

In my opinion, this issue warrants medium severity because it has extensive limitations to be triggered. First of all, it requires a loan that has some bad debt, which is highly unlikely given that the liquidation bot will always liquidate borrowers before they can generate bad debt.

This does not make it less likely. The likelihood is based on Auditor::clearBadDebt() calls that fail. Which is 99% of the time.

Second, it requires that no one has interacted with the protocol on the same block as the liquidation.

This is not completely true due to:

1. Not all functions accue earnings.
2. Even then, it will be false if the liquidation is first in the block.

And even in this case, The likelihood of having blocks that do not interact with accrue earning functions is extremely high.

Third, a liquidator can execute the liquidation twice to easily go around this issue.

It's not easily at all. Firstly, the second liquidation has to be in the same block. Secondly, the second liquidation is not profitable for the liquidator.

Summarizing, this issue requires certain conditions or specific states to be triggered so it qualifies for medium severity.

This does not require relevant specific conditions as @santipu03 implies. It may be mitigated by a liquidator that is aware of the issue and makes a special smart contract to address it (always liquidate twice). But this does not mean at all that it requires a special state. Additionally, this mitigation is very costly for the liquidator as it has to liquidate twice, incurring 2x gas costs. No one is going to perform this besides the protocol, who wants to remain solvent.

This is what differentiates this issue from for example #120, which does require special market conditions and is medium and not high.

Additionally, there is still the fact that the liquidator will be seized less than supposed. This can not be mitigated as it is profitable for the liquidatee who can trigger it himself.

[0x73696d616f]

Also, the penaltyRate of a maturity that has expired goes to the earnings accumulator, so liquidations will be completely off in this case. Check the POC for details.

This creates the following scenario, where a liquidator will be unfairly liquidated if they have borrowed maturities that have expired, as it increases the earnings accumulator only after checking the health factor of the borrower in Auditor::checkLiquidation().

Example
Assume health factor of 1
Real collateral 1000 (with earnings accumulator due to expired maturity)
debt 900
actual collateral used to calculate the health factor in Auditor::checkLiquidation()
899, as the debt from the expired maturity is in the earnings accumulator and is not yet accounted for.
Health factor is < 1, user is liquidated besides having a positive health factor.

[0x73696d616f]

All in all, liquidations will be always off, either leading to a liquidator that is liquidated but has a health factor above 1, bad debt that is accumulated (unless a liquidator bot spends 2x the gas, taking a loss, and decides to create a special liquidation smart contract to liquidate twice in the same transaction), or the liquidator not seizing enough collateral assets.

The likelihood of any of these events happening is extremely high (99%) as users always have assets in the market they are seized.

[santichez]

Hey @0x73696d616f , I realized that adding floatingAssets += accrueAccumulatedEarnings(); to the beginning of the liquidate function doesn't solve the issue but actually makes it worse since the liquidation now starts reverting due to InsufficientProtocolLiquidity.
This is because lastAccumulatorAccrual is updated at the beginning, so after the liquidation's repayment, the penalties are added to the accumulator but are not accounted to be distributed, and then these are not available to be withdrawn as liquidity.
On top of this, in your test, ALICE ends up with more collateral because it's the only user depositing in the floating pool, then earns 100% of the accumulator's distribution, which means that repaid penalties are going back to the same user. Under normal circumstances, a user wouldn't control that much of the pool/shares.
However, I do agree with you that the collateral calculation is not accounting for this case. So collateral calculation before and after this specific liquidation ends with the result you stated. I would acknowledge this but doesn't require a fix IMO.

[santipu03]

Firstly, the scenario where a user is liquidated with a health factor above 1 is highly improbable because it requires a long time without accruing accumulated earnings (that are updated with every deposit and withdrawal) so that the total assets are really off, causing a noticeable difference in the user's health factor.

Secondly, the scenario where a liquidated user is left with a dust amount of collateral is also highly improbable because it requires the loan to be liquidated with some bad debt. Because the protocol will have its own liquidation bot, it's assumed that liquidations are going to happen on time, preventing bad debt. The protocol admins are in charge of setting the correct adjust factors in each market so that even if there's a sudden fall in prices, the protocol doesn't accrue bad debt.

Because the scenarios described by Watson are improbable and require external conditions and specific states, I believe the appropriate severity for this issue is medium.

[0x73696d616f]

Ok, I am going to make a quick summary of this because at this point @santipu03 is mixing the likelihood of the issue being triggered with the likelihood of each of the scenarios happening (which I do not agree with), let's break it down.

Firstly, the likelihood of this issue is extremely high. The only condition is that at least 1 block has passed (2 seconds on Optimism), which is highly likely.

1. There is always going to be an instantaneous increase of the totalAssets(). In one of the scenarios, the liquidatee even benefits from the liquidation, as shown in the POC, from 802 to 1556.
   Say some time has passed before the liquidation.
   lendersAsset and penaltyRate are converted to the earnings accumulator, BEFORE updating the accrued earnings timestamp.
   Thus, this increase due to the 2 mentioned variables will instantly update the total assets, opposed to going through the accumulator period, breaking another core invariant of the protocol.

2. The previewed collateral is always incorrect as it does not account for the earnings accumulator due to lenders assets or penalty rate. This will lead to the impossibility of clearing bad debt unless 2 liquidations are performed in the same block. Another direct impact is that the liquidator will not seize enough assets, as the collateral is increased after it is previewed.

Now, the mentioned scenarios are examples of damage caused by this issue, the impact is always here.

Firstly, the scenario where a user is liquidated with a health factor above 1 is highly improbable because it requires a long time without accruing accumulated earnings (that are updated with every deposit and withdrawal) so that the total assets are really off, causing a noticeable difference in the user's health factor.

This is not true, 1 block is enough to create the required discrepancy.

Secondly, the scenario where a liquidated user is left with a dust amount of collateral is also highly improbable because it requires the loan to be liquidated with some bad debt. Because the protocol will have its own liquidation bot, it's assumed that liquidations are going to happen on time, preventing bad debt. The protocol admins are in charge of setting the correct adjust factors in each market so that even if there's a sudden fall in prices, the protocol doesn't accrue bad debt.

This scenario always happens when there is bad debt to be accumulated from a liquidatee. This is a core property of the protocol that is broken. The argument that it is extremely unlikely for a loan to have bad debt makes no sense, this is a big risk in lending protocols. Using this argument, all issues in lending protocols that happen when a loan has bad debt would be at most medium, which is totally unacceptable.

Because the scenarios described by Watson are improbable and require external conditions and specific states, I believe the appropriate severity for this issue is medium.

They are not improbable as shown before, liquidations are always off. And the impact is guaranteed, only requires 1 block passing. Again, issue #120 is a medium because it does require borrowing maturities, this is not the case here.
Furthermore, the instantaneous increase of totalAssets() will always happen.

The docs are clear

Definite loss of funds without (extensive) limitations of external conditions.

This is true as only 1 block has to pass.

Inflicts serious non-material losses (doesn't include contract simply not working).

This is true as liquidations will harm the liquidator, the liquidatee and/or the protocol by instaneously increasing totalAssets() and incorrectly previewing the collateral of the liquidator.

[0x73696d616f]

@santichez, your first statement may be true but that's another separate issue #70.

On top of this, in your test, ALICE ends up with more collateral because it's the only user depositing in the floating pool, then earns 100% of the accumulator's distribution, which means that repaid penalties are going back to the same user. Under normal circumstances, a user wouldn't control that much of the pool/shares.

Alice would still have enough shares to cause this issue. In fact, any amount of shares trigger this issue.

However, I do agree with you that the collateral calculation is not accounting for this case. So collateral calculation before and after this specific liquidation ends with the result you stated. I would acknowledge this but doesn't require a fix IMO.

There is an instantaneous increase of totalAssets() which benefits the liquidatee and the previewed collateral is incorrect, which affects the whole liquidation flow AND instantly increases the balance of every LP, it must be fixed.

Tweaked a bit the POC to turn the fix on and off (simulated the fix by calling market.setEarningsAccumulatorSmoothFactor(), which updates the earnings accumulator). It can be seen that only with the fix is the liquidatee with huge debt correctly and fully liquidated. I also added a deposit from BOB so the market has enough liquidity (as a fix to #70). The liquidatee can not be liquidated again as a mitigation because the health factor is above 1 in the end.

As can be seen the liquidation is completely off and the liquidatee takes a big win while the protocol and lps take a huge loss.

function test_POC_ProfitableLiquidationForLiquidatee_DueToEarningsAccumulator_Diff() external {
  bool FIX_ISSUE = false;

  uint256 maturity = FixedLib.INTERVAL * 2;
  uint256 assets = 10_000 ether;

  // BOB adds liquidity for liquidation
  vm.prank(BOB);
  market.deposit(assets, BOB);

  // ALICE deposits and borrows
  ERC20 asset = market.asset();
  deal(address(asset), ALICE, assets);
  vm.startPrank(ALICE);
  market.deposit(assets, ALICE);
  market.borrowAtMaturity(maturity, assets*78*78/100/100, type(uint256).max, ALICE, ALICE);
  vm.stopPrank();

  // Maturity is over and some time has passed, accruing extra debt fees
  skip(maturity + FixedLib.INTERVAL * 90 / 100);

  // ALICE has a health factor below 1 and should be liquidated and end up with 0 assets
  (uint256 collateral, uint256 debt) = market.accountSnapshot(address(ALICE));
  assertEq(collateral, 10046671780821917806594); // 10046e18
  assertEq(debt, 9290724716705852929432); // 9290e18

  // Simulate the fix, the call below updates the earnings accumulator
  if (FIX_ISSUE) market.setEarningsAccumulatorSmoothFactor(1e18);

  // Liquidator liquidates
  address liquidator = makeAddr("liquidator");
  deal(address(asset), liquidator, assets);
  vm.startPrank(liquidator);
  asset.approve(address(market), type(uint256).max);
  market.liquidate(ALICE, type(uint256).max, market);
  vm.stopPrank();

  (collateral, debt) = market.accountSnapshot(address(ALICE));

  if (FIX_ISSUE) { // Everything is liquidated with the fix
    assertEq(collateral, 0);
    assertEq(debt, 0);
  } else { // Without the fix, the liquidatee instantly receives assets, stealing from other users
    assertEq(collateral, 774637490125015156069); // 774e18
    assertEq(debt, 157386734140473105255); // 157e18
  }
}

[0x73696d616f]

This issue was originally marked as a medium instead of high because anyone can liquidate a position twice, thus solving this issue.

Highlighting the fact that the issue was downgraded to medium due to a possible mitigation that is only applicable to 1 of the 3 described scenarios (which is not even a complete mitigation, requiring knowledge of the issue from the liquidator to fix it, which is liquidating twice in a row in the same block, so it can not even be considered) and only partially mitigates the impact. Check either POCs above to see how one of the impacts can not be mitigated by this.

[0x73696d616f]

@cvetanovv the issue and the last 3 comments are enough to understand why this is a high severity issue. I am available for any further clarification if required.

[etherSky111]

In your previous PoCs, there haven't been any updates from now to maturity + FixedLib.INTERVAL * 90 / 100 in the pool.
This is really long period.
Whenever there are changes, such as deposits or borrowing etc, the pool updates its accumulated earnings.

function beforeWithdraw(uint256 assets, uint256) internal override whenNotPaused {
  updateFloatingAssetsAverage();
  depositToTreasury(updateFloatingDebt());
  uint256 earnings = accrueAccumulatedEarnings();
  uint256 newFloatingAssets = floatingAssets + earnings - assets;
  // check if the underlying liquidity that the account wants to withdraw is borrowed
  if (floatingBackupBorrowed + floatingDebt > newFloatingAssets) revert InsufficientProtocolLiquidity();
  floatingAssets = newFloatingAssets;
}

function afterDeposit(uint256 assets, uint256) internal override whenNotPaused whenNotFrozen {
  updateFloatingAssetsAverage();
  uint256 treasuryFee = updateFloatingDebt();
  uint256 earnings = accrueAccumulatedEarnings();
  floatingAssets += earnings + assets;
  depositToTreasury(treasuryFee);
  emitMarketUpdate();
}

If there haven't been any updates for a long time, it suggests that the pool is almost inactive and has only few depositors including your liquidatee.
Is this a realistic scenario in the real market?
The likelihood of this happening is very low.

Also if the gap between the last update time and now is small, then the impact will also be minimal.

As a result, this issue is more low severity.

I want to spend more time finding issues in other contests.
However, this issue seems very clear to me, and Watson insists it's of high severity.
So, I checked it again and shared my thoughts.

Stop trying to take up the judge's time with endless comments and let's respect the lead judges' decision, as I have never seen a wrong judgment in Sherlock.

[cvetanovv]

This issue is more Medium than High. 

There are several conditions that reduce the severity:

• The position can be liquidated twice.
• For the vulnerability to be valid, no one must interact with accrue earning functions. The likelihood of this happening is very low.
• Bots will be used mainly for the liquidation, which will be able to perform the liquidation twice in the same block. This will decrease the severity of the problem from High to Medium/Low the most because the protocol will use bots to perform the liquidation.

This issue entirely matches the Medium definition:

Causes a loss of funds but requires certain external conditions or specific states.

Planning to reject the escalation and leave the issue as is.

[0x73696d616f]

@cvetanovv the ONLY necessary condition is having passed at least 1 block, nothing more. This is not 'certain external conditions or specific states.'. Pick a random protocol on some block explorer and you'll see that the last transaction was a few minutes ago.

Thus, the likelihood is extremely high.

The liquidator always steals a portion of the assets, and the exact amount depends on the earnings accumulator smooth factor and time passed.

The scenario that you are referring to that is not as likely and can be mitigated is when bad debt is not cleared due to leftover collateral. This is another impact of the issue.

[0x73696d616f]

@cvetanovv tweaked the POC once again and only 3 minutes pass now. It can be seen that the liquidator is still profiting from this. It can not be liquidated again as the health factor is above 1.

Also if the gap between the last update time and now is small, then the impact will also be minimal.

This depends entirely on the smooth factor, which is a setter. The following POC shows that 3 minutes are enough to cause a big change.

function test_POC_ProfitableLiquidationForLiquidatee_DueToEarningsAccumulator_Diff() external {
  market.setEarningsAccumulatorSmoothFactor(1e14);
  bool FIX_ISSUE = false;

  uint256 maturity = FixedLib.INTERVAL * 2;
  uint256 assets = 10_000 ether;

  // BOB adds liquidity for liquidation
  vm.prank(BOB);
  market.deposit(assets, BOB);

  // ALICE deposits and borrows
  ERC20 asset = market.asset();
  deal(address(asset), ALICE, assets);
  vm.startPrank(ALICE);
  market.deposit(assets, ALICE);
  market.borrowAtMaturity(maturity, assets*78*78/100/100, type(uint256).max, ALICE, ALICE);
  vm.stopPrank();

  // Maturity is over and some time has passed, accruing extra debt fees
  skip(maturity + FixedLib.INTERVAL * 90 / 100);

  // ALICE net balance before liquidation
  (uint256 collateral, uint256 debt) = market.accountSnapshot(address(ALICE));
  assertEq(collateral, 10046671780821917806594); // 10046e18
  assertEq(debt, 9290724716705852929432); // 9290e18

  // Simulate market interaction
  market.setEarningsAccumulatorSmoothFactor(1e14);

  // Only 3 minute passes
  skip(3 minutes);

  if (FIX_ISSUE) market.setEarningsAccumulatorSmoothFactor(1e14);

  // Liquidator liquidates
  address liquidator = makeAddr("liquidator");
  deal(address(asset), liquidator, assets);
  vm.startPrank(liquidator);
  asset.approve(address(market), type(uint256).max);
  market.liquidate(ALICE, type(uint256).max, market);
  vm.stopPrank();

  (collateral, debt) = market.accountSnapshot(address(ALICE));

  if (FIX_ISSUE) { // Everything is liquidated with the fix
    assertEq(collateral, 0);
    assertEq(debt, 0);
  } else { // Without the fix, the liquidator instantly receives assets, stealing from other users
    assertEq(collateral, 313472632221182141406); // 313e18
    assertEq(debt, 157644123455541063036); // 157e18
  }
}

[0x73696d616f]

This issue classification is
likelihood: high
impact: medium to high (if the smooth factor is big and not much time has passed, medium, if the smooth factor is small and/or some time has passed, high). Any small time will exceed small, finite amounts and the bigger the time, the bigger the losses.

[etherSky111]

Hey, the likelihood is still low.
In your above PoC, there are only 2 depositors and the liquidatee deposit 50% of total liquidity.

And in normal pool, the asset amount which liquidatee receives from earnings accumulator will be minimal.

[etherSky111]

And please, the high severity issue means that this is so critical that the sponsor team should fix this before deployment.

You didn't even convince sponsor team, lead judge, and other watsons.

[0x73696d616f]

Hey, the likelihood is still low.

Stop throwing this around, it is not low. I can change the POC to pass 15 seconds and the issue still exists.

And in normal pool, the asset amount will be minimal.
For the liquidator, he may get less amount, but it still breaks the invariant of the earnings accumulator.

@cvetanovv for context, they have an earnings accumulator to delay rewards, which works basically like this

elapsed = block.timestamp - last update
earningsAccumulator = assets * elapsed / (elapsed + earningsAccumulatorSmoothFactor * constant)

So rewards (totalAssets()) are expected to slowly grow according to this accumulator.
The problem is that the liquidation increments the variable assets in the formula, before updating last update and the previous accumulator, which will instantly increase totalAssets(), instead of going through the delay.

[0x73696d616f]

And please, the high severity issue means that this is so critical that the sponsor team should fix this before deployment.
You didn't even convince sponsor team, lead judge, and other watsons.

I have no problem with showing the sponsor how serious this is.
@santichez could you take a look at this test?.

[etherSky111]

what about this?

In your above PoC, there are only 2 depositors and the liquidatee deposit 50% of total liquidity.

[0x73696d616f]

@santipu03 I agree with the first 2 statements.

But this one

Secondly, the PoC assumes that a long time has to happen without any deposits/withdrawals in order for the issue to have any noticeable impact.

I am not sure what long time you are talking about? Some POCs show impact with only 3 minutes of time passed. Daily transactions (few hours of time between accrual update) are enough to cause noticeable impact.

[santipu03]

I've just run again this POC but changing the value of the smooth factor from 1e14 to 1e18 and changing the time that has passed without transactions from 3 minutes to 1 day. These changes represent more a realistic on-chain scenario.

After these changes are applied, the price change is only 0.6% instead of 3%. Even in this scenario where the liquidatee has 50% of the total market liquidity, which is a highly improbable scenario, the resulting price difference is highly constrained.

For these reasons, I believe this issue doesn't warrant high severity.

[0x73696d616f]

@santipu03 thank you for running the numbers again, I agree with them.

Even in this scenario where the liquidatee has 50% of the total market liquidity, which is a highly improbable scenario, the resulting price difference is highly constrained.

Smaller positions may be liquidated at a time, the impact may be smaller for each one, but overall it will add up. I am not disagreeing with your statement, just emphasizing this will add up either way.

I'd argue 0.6% is a high number as it affects total tvl, but leave this up to the judge.

[santipu03]

Sorry, I've mixed the numbers. Here's a recap of the changes made to the presented PoC:

• Change the smooth factor to a realistic value: from 1e14 to 1e18.
• Change the time without transactions to a realistic value: 12 hours.
• Change the liquidation time from maturity + INTERVAL * 90 / 100 to maturity + 2 days, meaning the liquidation is not going to be delayed too much.

Adapted POC

function test_POC_ProfitableLiquidationForLiquidatee_DueToEarningsAccumulator_Diff() external {
  market.setEarningsAccumulatorSmoothFactor(1e18);
  bool FIX_ISSUE = false;
  
  uint256 maturity = FixedLib.INTERVAL * 2;
  uint256 assets = 10_000 ether;
  
  // BOB adds liquidity for liquidation
  vm.prank(BOB);
  market.deposit(assets, BOB);
  
  // ALICE deposits and borrows
  ERC20 asset = market.asset();
  deal(address(asset), ALICE, assets);
  vm.startPrank(ALICE);
  market.deposit(assets, ALICE);
  market.borrowAtMaturity(maturity, (assets * 78 * 78) / 100 / 100, type(uint256).max, ALICE, ALICE);
  vm.stopPrank();
  
  skip(maturity + 2 days); // Maturity is over and some time has passed, accruing extra debt fees
  
  market.setEarningsAccumulatorSmoothFactor(1e18); // Simulate market interaction
  
  skip(12 hours); // 12 hours passed
  
  uint256 previousPrice = 1004667178082191780; // ~1.004e18
  
  assertEq(market.previewRedeem(1e18), previousPrice);
  
  if (FIX_ISSUE) market.setEarningsAccumulatorSmoothFactor(1e14);
  
  // Liquidator liquidates
  address liquidator = makeAddr("liquidator");
  deal(address(asset), liquidator, assets);
  vm.startPrank(liquidator);
  asset.approve(address(market), type(uint256).max);
  market.liquidate(ALICE, type(uint256).max, market);
  vm.stopPrank();
  
  if (FIX_ISSUE) assertEq(previousPrice, market.previewRedeem(1e18));
  else assertEq(market.previewRedeem(1e18), 1004719564791669940);
}

Here are the final results:

Price without the fix: 1004719564791669940
Price with the fix:    1004667178082191780

As we can see, the price increase is just 0.005% (0.000052386709478160e18), which is way less than originally claimed.

[0x73696d616f]

I have built another POC proving that the price impact is 0.3%.

Adjust factor of the market is 0.9
12 hours pass since the last interaction.
The change was the borrowed amount being smaller, so the debt has more time to accrue. The likelihood of this happening is not small but not huge either, as someone has bad debt accumulating for some time. This may be tweaked to accrue for a smaller time, increasing likelihood but decreasing the impact.

This is a realistic scenario with a decent likelihood of happening and causes substancial damage to the protocol, enough for HIGH severity

Definite loss of funds without (extensive) limitations of external conditions.

function test_POC_ProfitableLiquidationForLiquidatee_DueToEarningsAccumulator_Diff() external {
  auditor.setAdjustFactor(market, 0.9e18);
  market.setEarningsAccumulatorSmoothFactor(1e18);
  bool FIX_ISSUE = false;
  
  uint256 maturity = FixedLib.INTERVAL * 2;
  uint256 assets = 10_000 ether;
  
  // BOB adds liquidity for liquidation
  vm.prank(BOB);
  market.deposit(assets, BOB);
  
  // ALICE deposits and borrows
  ERC20 asset = market.asset();
  deal(address(asset), ALICE, assets);
  vm.startPrank(ALICE);
  market.deposit(assets, ALICE);
  market.borrowAtMaturity(maturity, (assets * 50 * 50) / 100 / 100, type(uint256).max, ALICE, ALICE);
  vm.stopPrank();
  
  // Maturity is over and some time has passed, accruing extra debt fees
  // until the account is liquidatable
  skip(maturity + 16 weeks);

  (uint256 coll, uint256 debt) = auditor.accountLiquidity(ALICE, Market(address(0)), 0);
  assertEq(coll*100/debt, 98); // Health factor is 0.98
  
  market.setEarningsAccumulatorSmoothFactor(1e18); // Simulate market interaction
  
  skip(12 hours); // 12 hours passed
  
  uint256 previousPrice = 1e18; // ~1.004e18
  
  assertEq(market.previewRedeem(1e18), previousPrice);
  
  if (FIX_ISSUE) market.setEarningsAccumulatorSmoothFactor(1e18);
  
  // Liquidator liquidates
  address liquidator = makeAddr("liquidator");
  deal(address(asset), liquidator, assets);
  vm.startPrank(liquidator);
  asset.approve(address(market), type(uint256).max);
  market.liquidate(ALICE, type(uint256).max, market);
  vm.stopPrank();
  
  if (FIX_ISSUE) assertEq(previousPrice, market.previewRedeem(1e18));
  else assertEq(market.previewRedeem(1e18), 1003198119342946548);
}

[0x73696d616f]

We can build different POCs and scenarios, proving that the likelihood is indeed very high.
Some scenarios lead to huge price impacts, some smaller impacts, but there is a big range of realistic and likely scenarios that lead to big enough price impacts, which should classify this as a HIGH.

[0x73696d616f]

There is another impact here. As the liquidatee will have outstanding collateral from the liquidation (which was not accounted for), it will be liquidated again (if the health factor is below 1). This means that instead of having the liquidated portion of the assets totally going to the earnings accumulator (as intended), a % of it will go to the liquidator. Thus, LPS will suffer further.

[santipu03]

@santichez I'd like to have your opinion again on this issue. Why do you think it doesn't require a fix?

[santichez]

@santipu03 After the very elaborate and detailed explanation, we came to the agreement that the fix is indeed necessary 👍
We appreciate it, @0x73696d616f .

[0x73696d616f]

@santichez glad I could help.

[0x73696d616f]

@santipu03 this can also be abused by depositing 12 hours prior to the liquidation and then withdrawing after, getting free arbitrage.

[santipu03]

Nice, it's true that there are some specific scenarios where the impact can become worrying.

However, as several POCs from both parts have demonstrated, for this bug to have some real impact, there must be quite some specific states that must be met. For example, some of those requirements are the following:

1. A whale borrower with an insane amount of penalties on fixed loans must be liquidated.
2. Some amount of time when no deposits/withdrawals must occur.

There are probably some more scenarios where this issue may have some impact, but those scenarios will also have some specific requirements and conditions that must be given first. As a side note, in the end, the admins can always increase the smooth factor to a higher value than usual to dampen any possible impact this issue may provoke.

At this point, I believe @cvetanovv probably may have all the information necessary to make a fair judgment about this issue.

[0x73696d616f]

A whale borrower with an insane amount of penalties on fixed loans must be liquidated.

A few smaller borrowers will also lead to the same accumulated price impact. This is not a precondition as the borrower may be split in any number of borrowers and the impact is the same.

As a side note, in the end, the admins can always increase the smooth factor to a higher value than usual to dampen any possible impact this issue may provoke.

They can also decrease it and the impact becomes even higher. Thus, this is also not a precondition as it can go either way.

The only real pre condition is time passing, in which we proved that only 12 hours (absolutely common scenario, check the protocol on optimism) leads to significant impact. At the current time, there was a transaction 33 hours ago. As long as a few hours go by, we can find problematic scenarios. This is by no means (extensive) limitations.

Just want to highlight the fact that his can be exploited by depositing a few hours prior to the liquidation. Fixed loans may have up to 12 weeks of duration, attackers may deposit just a few hours before the liquidatee is expected to reach the health factor of 1 and steal a big portion of the incorrectly instantly increased total assets. In the same POC we've been using, if the price impact is 0.3%, bob instantly earns 0.3%. As he has 10_000 DAI, this is 300 USD for free.

At this point, I believe @cvetanovv probably may have all the information necessary to make a fair judgment about this issue.

Agreed.

[etherSky111]

I agree that this issue deserves medium severity, but it doesn't qualify as high severity.
For a noticeable impact to occur, many factors must align.
In your PoC, the price change of 0.3% requires the liquidatee to deposit 50% of the total liquidity.
Additionally, the liquidatee did nothing for over 16 weeks, despite having enough liquidity and being aware of the significant penalty that applies to large debt over this period.
Is there such a trader in the real market?

I slightly modified your PoC: the liquidatee deposited 25% of the total liquidity, resulting in a 0.1% price change.

function test_POC_ProfitableLiquidationForLiquidatee_DueToEarningsAccumulator_Diff() external {
  auditor.setAdjustFactor(market, 0.9e18);
  market.setEarningsAccumulatorSmoothFactor(1e18);
  bool FIX_ISSUE = false;
  
  uint256 maturity = FixedLib.INTERVAL * 2;
  uint256 assets = 10_000 ether;
  
  // BOB adds liquidity for liquidation
  vm.prank(BOB);
  market.deposit(assets, BOB);
  
  // ALICE deposits and borrows
  uint256 assetsAlice = 3_000 ether;
  ERC20 asset = market.asset();
  deal(address(asset), ALICE, assetsAlice);
  vm.startPrank(ALICE);
  market.deposit(assetsAlice, ALICE);
  market.borrowAtMaturity(maturity, (assetsAlice * 50 * 50) / 100 / 100, type(uint256).max, ALICE, ALICE);
  vm.stopPrank();
  
  // Maturity is over and some time has passed, accruing extra debt fees
  // until the account is liquidatable
  skip(maturity + 16 weeks);

  (uint256 coll, uint256 debt) = auditor.accountLiquidity(ALICE, Market(address(0)), 0);
  // assertEq(coll*100/debt, 98); // Health factor is 0.98
  console2.log('health factor: ', coll * 100 / debt);
  
  market.setEarningsAccumulatorSmoothFactor(1e18); // Simulate market interaction
  
  skip(12 hours); // 12 hours passed
  
  uint256 previousPrice = 1e18; // ~1.004e18
  
  assertEq(market.previewRedeem(1e18), previousPrice);
  
  if (FIX_ISSUE) market.setEarningsAccumulatorSmoothFactor(1e18);
  
  // Liquidator liquidates
  address liquidator = makeAddr("liquidator");
  deal(address(asset), liquidator, assets);
  vm.startPrank(liquidator);
  asset.approve(address(market), type(uint256).max);
  market.liquidate(ALICE, type(uint256).max, market);
  vm.stopPrank();
  
  // if (FIX_ISSUE) assertEq(previousPrice, market.previewRedeem(1e18));
  // else assertEq(market.previewRedeem(1e18), 1003198119342946548);
  console2.log('price: ', market.previewRedeem(1e18));
}

Log is

health factor:  98
price:  1001476055081359945

This will be my last comment.
Everything depends on head of judge's decesion.

[0x73696d616f]

@etherSky111 you're just demonstrating the high likelihood of this issue. This trader may exist, another trader that only waits 8 weeks and results in a slightly lower impact also exists, and another one that has even a bigger impact may exist. It has been proved that the impact is significant and there are not (extensive) limitations.

And please, the high severity issue means that this is so critical that the sponsor team should fix this before deployment.
You didn't even convince sponsor team, lead judge, and other watsons.

the sponsor is going to fix it, so according to your own logic, this should be high.

[etherSky111]

only waits 8 weeks? stop kidding.
Please please stop trying to add weight to your report.

[0x73696d616f]

only waits 8 weeks? stop kidding.

The fixed loans have a maximum duration of 12 weeks, may be more depending on configs. 8 weeks is not a long time to repay. Regardless, impact can always be found and is significant in many scenarios.

Please please stop trying to add weight to your report.

You're the one making these bold claims without adding value to this discussion.

[0x73696d616f]

@cvetanovv could you make a new judgement based on the new comments?. It has been proven that all the original reasons to downgrade this issue are not applicable to the price change and arbitrage impact. They are only applicable to the impact of creation of bad debt, and even then, not much time has to pass.

[etherSky111]

Please don't try to confuse the head judge's decision with wrong arguments.

In order to get 0.1% price change, the liquidatee need to deposit at least 25% of total liquidity and should wait for 16 weeks without doing anything.
And as you can observe, the arbitrage impact is really minimal.

Please let the judge to decide.

[0x73696d616f]

@etherSky111 what are you talking about?

There are several conditions that reduce the severity:
The position can be liquidated twice.
For the vulnerability to be valid, no one must interact with accrue earning functions. The likelihood of this happening is very low.
Bots will be used mainly for the liquidation, which will be able to perform the liquidation twice in the same block. This will decrease the severity of the problem from High to Medium/Low the most because the protocol will use 

None of this is applicable to the price impact. Will not comment further, the comments above demonstrate this statement.

[cvetanovv]

I have read the discussion several times, and I stand by my original decision to reject the escalation and keep this issue Medium.

I agree that some conditions from my previous comment may not decrease the impact. But still, the vulnerability depends on the time that has passed without deposits or withdrawals, which is very important.

Also, the admin is Тrusted, and we will assume that he will increase the smooth factor to a higher value and thus further reduce the impact.

I stand by my initial decision to reject the escalation.

[0x73696d616f]

@cvetanovv I am not disputing your decision, just laying out the facts. I understand it's not easy going through so much information in short time and little incentive, that's why I am trying to help.

But still, the vulnerability depends on the time that has passed without deposits or withdrawals, which is very important.

It was proved that 12 hours are enough to cause impact and there are many hours, even days between interactions, please check the protocol on optimism. The required time to pass to have significant impact is exactly what happens in reality, as can be checked on the block explorer. This is a perfectly likely event.

Also, the admin is Тrusted, and we will assume that he will increase the smooth factor to a higher value and thus further reduce the impact.

This would happen after damage had already been caused. And if the admin decreased the factor, the damage would be higher. As the factor can go either way, either increase severity or decrease severity, it can not be used as a precondition. If I can not argue that if the factor is decreased, the impact is higher, neither can it be argued that if the factor is increased, the impact is lower. This would be a completely unfair treatment. What if the Trusted admin decides to deploy a market with a lower factor? he has no understanding of the exploit at this point.

None of these conditions are significant and real.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
exactly/protocol#727

[0x73696d616f]

@santichez what do you think of this issue?

[santichez]

@0x73696d616f I believe this should be considered medium too, sorry :(

[Evert0x]

Result:
Medium
Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• 0x73696d616f: rejected

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.