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]

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

If the liquidatee has less % of the total liquidity, obviously he will get less assets after the liquidation. The key issue remains, which is the fact that totalAssets() will be instantly updated, without going through the accumulator. Again, this is a core invariant of the protocol that is broken.

[0x73696d616f]

From the docs:

So, to avoid opening possible MEV profits for bots or external actors to sandwich these operations by depositing to the pool with a significant amount of assets to acquire a more considerable proportion and thus earning profits for then instantly withdrawing, we've come up with an earnings accumulator. This accumulator will hold earnings that come from extraordinary sources and will gradually and smoothly distribute these earnings to the pool using a distribution factor. Then incentivizing users to keep lending their liquidity while disincentivizing atomic bots that might look to profit from Exactly unfairly.

This is broken. will gradually and smoothly distribute these earnings to the pool using a distribution factor. False.

[0x73696d616f]

@cvetanovv ran the POC once again to show how the price before and after the liquidation changes significantly, from 1e18 to 1.03e18, a 3% price increase. This property of the protocol is broken, see the docs above.

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);

  uint256 previousPrice = 1004667178082191780; // 1e18

  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), 1036014441304309994); //1.03e18.
}

[0x73696d616f]

Also

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.

I am contributing to this issue with facts and POCs, helping keeping the judgment fair on Sherlock. This is what @cvetanovv intends, that the judgement is correct, not that it is rushed.

[etherSky111]

I slightly changed your PoC. In your PoC, the liquidation happens after maturity + FixedLib.INTERVAL * 90 / 100.

skip(maturity + FixedLib.INTERVAL * 90 / 100);

I changed this as below:

skip(maturity + FixedLib.INTERVAL * 10 / 100);  // here

I mean the liquidation can happen earlier and anyone can easily liquidate this user once it becomes liquidatable.

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

  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 * 10 / 100);  // here

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

  // 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);
    console2.log('collater after      : ', collateral);
    console2.log('debt after          : ', debt);
  } else { // Without the fix, the liquidator instantly receives assets, stealing from other users
    // assertEq(collateral, 313472632221182141406); // 313e18
    // assertEq(debt, 157644123455541063036); // 157e18
    console2.log('collater after      : ', collateral);
    console2.log('debt after          : ', debt);
  }
}

The result is as below: after fixing:

collateral before   :  10046671780821917806594
debt before         :  6523274801095170870548
collater after      :  6572313121269814777661  // here
debt after          :  3365024318090145165663

now

collateral before   :  10046671780821917806594
debt before         :  6523274801095170870548
collater after      :  6592123038195909881811  // here
debt after          :  3365024318090145165663

Even though this liquidatee has 50% of total liquidiaty (almost impossble), there is no big change. You forgot that the account will be liquidated immediately once it becomes liquidatable.

And in this case, the increase of earnings accumulator will be small.

[0x73696d616f]

ok @etherSky111, so you agree with me, the impact goes from medium to high. At least you seem to understand now that the likelihood is high.

[etherSky111]

Please stop trying to show wonderful writing skills.

Please try to be fair.

[0x73696d616f]

Appreciate the compliment. I am trying to be fair. The likelihood is high and the impact is medium to high. You know this is true sir.

[etherSky111]

In order to the impact becomes medium, the likelihood is low. The liquidator deposit almost liquidity and the liquidation should not happen for enough period.

[0x73696d616f]

If you run the price in your modified POC, you can see that it goes from 1004667178082191780 to 1006648169774801291, which is a 0.2% increase. This exceeds small and finite amounts. So the impact is still medium, while the likelihood is high (you made the scenario, not me).

[etherSky111]

Don't forget that your liquidatee deposited 50% of total liquidity.

Is likelihood still high?

[etherSky111]

I showed my thoughts enough and let the head of judge to decide.

Anyway, agree that this is good finding.

[santipu03]

@0x73696d616f From the PoC presented, setting the earnings accumulator smooth factor to 1e14 is solving the issue?

[0x73696d616f]

Don't forget that your liquidatee deposited 50% of total liquidity.

If the total liquidity of the liquidatee is smaller, the price impact will also be smaller. But there are more liquidations, and all of them impact the price, when they should not.

@etherSky111 thank you for your comments, was a nice discussion sir.

[0x73696d616f]

From the PoC presented, setting the earnings accumulator smooth factor to 1e14 is solving the issue?

yes because it triggers the earnings accumulator accrual. This was just a workaround to show that the issue exists.

[0x73696d616f]

If you run the price in your modified POC, you can see that it goes from 1004667178082191780 to 1006648169774801291, which is a 0.2% increase. This exceeds small and finite amounts. So the impact is still medium, while the likelihood is high (you made the scenario, not me).

Btw 0.2% has been accepted as exceeding small and finite amounts in the past.

[santipu03]

@0x73696d616f What happens if we set the smooth factor at 1e18 at the start of the test? Because that is the deployment values as per the Market tests.

[0x73696d616f]

@santipu03 the price impact will be smaller.

[santipu03]

What I get from the discussion above is that the impact of this issue depends on mainly two factors:

1. The value of the earnings accumulator smooth factor. The higher the factor, the lower the impact.
2. The time passed since the last deposit/withdrawal on that market. The longer the time, the higher the impact.

Regarding the first point, the trusted admins are in charge of setting the correct smooth factor so it's assumed that it will be set to a reasonable value, e.g. 1e18. Take into account that the current value of the smooth factor in the PoC presented is 1e14 instead of 1e18, and that is 10,000 lower.

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've looked at the live markets of Exactly and we can see that even the lowest-used markets (OP and WBTC) still have daily transactions involving deposits and withdrawals.

As we can see, the conditions that must be met for this issue to have any noticeable impact are highly unlikely. For this reason, I still believe this issue warrants a medium severity.

[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

```solidity 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.