Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flashloan44 - Liquidation can be DOSed due to lack of liquidity on collateral asset reserve #198

Open
sherlock-admin3 opened this issue Sep 10, 2024 · 11 comments
Labels
Escalation Resolved This issue's escalations have been approved/rejected Has Duplicates A valid issue with 1+ other issues describing the same vulnerability High A High severity issue. Reward A payout will be made for this issue

Comments

@sherlock-admin3
Copy link
Contributor

sherlock-admin3 commented Sep 10, 2024

Flashloan44

High

Liquidation can be DOSed due to lack of liquidity on collateral asset reserve

Summary

Lack of liquidity on collateral asset reserves can cause disruption to liquidation.

Root Cause

The protocol don't have option to disable borrowing or withdrawing in a particular asset reserve for a certain extent to protect the collateral deposits. It can only disable borrowing or freeze the whole reserves but not for specific portion such as collateral deposits. This can be a big problem because someone can always deduct or empty the reserves either by withdrawing their lended assets or borrowing loan. And when the liquidation comes, the collateral can't be paid to liquidator because the asset reserve is already not enough or emptied.

The pool administrator might suggest designating whole asset reserve to be used only for collateral deposit purposes and not for lending and borrowing. However this can be circumvented by malicious users by transferring their collateral to other reserves that accepts borrowing and lending which can eventually led the collateral to be borrowed. This is the nature of multi-asset lending protocol, it allows multiple asset reserves for borrowing and lending as per protocol documentation.

There could be another suggestion to resolve this by only using one asset reserve per pool that offers lending and borrowing but this will already contradict on what the protocol intends to be which is to be a multi-asset lending pool, meaning there are multiple assets offering lending in single pool.

If the protocol intends to do proper multi-asset lending pool platform, it should protect the collateral assets regarding liquidity issues.

Internal pre-conditions

  1. Pool creator should setup the pool with more than 2 asset reserves offering lending or borrowing and each of reserves accepts collateral deposits. It allows any of the asset reserves to conduct borrowing to any other asset reserves and vice versa. This is pretty much the purpose and design of the multi-asset lending protocol as per documentation.

External pre-conditions

No response

Attack Path

This can be considered as attack path or can happen also as normal scenario due to the nature or design of the multi-asset lending protocol. Take note the step 6 can be just a normal happening or deliberate attack to disrupt the liquidation.

image

Impact

This should be high risk since in a typical normal scenario, this vulnerability can happen without so much effort.
The protocol also suffers from bad debt as the loan can't be liquidated.

PoC

  1. Modify this test file /zerolend-one/test/forge/core/pool/PoolLiquidationTests.t.sol
    and insert the following:
    a. in line 16, put address carl = address(3); // add carl as borrower
    b. modify this function _generateLiquidationCondition() internal {
    _mintAndApprove(alice, tokenA, mintAmountA, address(pool)); // alice 1000 tokenA
    _mintAndApprove(bob, tokenB, mintAmountB, address(pool)); // bob 2000 tokenB
    _mintAndApprove(carl, tokenB, mintAmountB, address(pool)); // carl 2000 tokenB >>> add this line

    vm.startPrank(alice);
    pool.supplySimple(address(tokenA), alice, supplyAmountA, 0); // 550 tokenA alice supply
    vm.stopPrank();

    vm.startPrank(bob);
    pool.supplySimple(address(tokenB), bob, supplyAmountB, 0); // 750 tokenB bob supply
    vm.stopPrank();

    vm.startPrank(carl);
    pool.supplySimple(address(tokenB), carl, supplyAmountB, 0); // 750 tokenB carl supply >>> add this portion
    vm.stopPrank();

    vm.startPrank(alice);
    pool.borrowSimple(address(tokenB), alice, borrowAmountB, 0); // 100 tokenB alice borrow
    vm.stopPrank();

    assertEq(tokenB.balanceOf(alice), borrowAmountB);

    oracleA.updateAnswer(5e3);
    }
    c. Insert this test
    function testLiquidationSimple2() external {
    _generateLiquidationCondition();
    (, uint256 totalDebtBase,,,,) = pool.getUserAccountData(alice, 0);

    vm.startPrank(carl);
    pool.borrowSimple(address(tokenA), carl, borrowAmountB, 0); // 100 tokenA carl borrow to deduct the reserves in which the collateral is deposited
    vm.stopPrank();

    vm.startPrank(bob);
    vm.expectRevert();
    pool.liquidateSimple(address(tokenA), address(tokenB), pos, 10 ether); // Bob tries to liquidate Alice but will revert

    vm.stopPrank();

    (, uint256 totalDebtBaseNew,,,,) = pool.getUserAccountData(alice, 0);

    // Ensure that no liquidation happened and Alice's debt remains the same
    assertEq(totalDebtBase, totalDebtBaseNew, "Debt should remain the same after failed liquidation");

}
2. Run the test forge test -vvvv --match-contract PoolLiquidationTest --match-test testLiquidationSimple2

Mitigation

Each asset reserve should be modified to not allow borrowing or withdrawing for certain collateral deposits. For example, if a particular asset reserve has deposits for collateral, these deposits should not be allowed to be borrowed or withdrew. The rest of the balance of asset reserves will do the lending. At the current design, the pool admin can only make the whole reserve as not enabled for borrowing but not for specific account or amount.

@sherlock-admin3
Copy link
Contributor Author

1 comment(s) were left on this issue during the judging contest.

Honour commented:

see #147

@nevillehuang nevillehuang added Medium A Medium severity issue. and removed High A High severity issue. labels Oct 1, 2024
@sherlock-admin3 sherlock-admin3 changed the title Happy Midnight Griffin - Liquidation can be DOSed due to lack of liquidity on collateral asset reserve Flashloan44 - Liquidation can be DOSed due to lack of liquidity on collateral asset reserve Oct 3, 2024
@sherlock-admin3 sherlock-admin3 added the Reward A payout will be made for this issue label Oct 3, 2024
@0xspearmint1
Copy link

This issue should be high severity, it satisfies Sherlock's criteria for high issues

Definite loss of funds without (extensive) limitations of external conditions. The loss of the affected party must exceed 1%.

The attacker can easily delay the liquidation till bad debt accumulates which will be a >1% loss for the lender

@Haxatron
Copy link

Haxatron commented Oct 5, 2024

Escalate

Final time to use this

@sherlock-admin3
Copy link
Contributor Author

Escalate

Final time to use this

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.

@sherlock-admin4 sherlock-admin4 added the Escalated This issue contains a pending escalation label Oct 5, 2024
@cvetanovv
Copy link

This issue does not qualify as high severity.

The vulnerability arises because the protocol allows collateral to be borrowed, which can lead to temporary liquidation failures due to insufficient liquidity in the pool.

However, for a user to withdraw all the funds from a pool and cause this scenario, he would need a large amount of capital. Even if they succeed in doing so, the DoS on liquidations is only temporary because the borrower must eventually return the borrowed funds, and they will also incur interest costs.

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

@0xjuaan
Copy link

0xjuaan commented Oct 14, 2024

@cvetanovv

and they will also incur interest costs

The interest cost will never need to be paid, because the borrow will not be liquidateable.

he DoS on liquidations is only temporary because the borrower must eventually return the borrowed funds

The DoS on liquidations is not temporary because the borrow will never need to be repaid (because there is no risk of liquidation from accrued interest, because all the collateral is borrowed)
Even if it was temporary, a DoS of liquidations can be weaponised to lead to bad debt, which is >1% profit for the attacker (at the expense of depositors) since their borrowed funds will be worth more than their collateral provided.

Based on the above, it is clearly a high severity issue. It has arised due to forking AAVE but not allowing representative aTokens to be seized, as mentioned in #318:

This is a known issue that aave have mitigated by allowing liquidators to seize ATokens instead of underlying tokens, when there is not enough liquidity in the pools.
To achieve the modularity expected zerolend have tried to simplify the design by removing this core functionality, this however exposes the protocol to the risk of liquidation being blocked if there is not enough liquidity in the pools.

@Honour-d-dev
Copy link

i agree with @cvetanovv
there will always be costs for the attacker, as all assets ltv must be less than 1 so the attacker will have to deposit more in value than they borrow. Combined with their growing interest that, they'll have to either repay the loan + interest to retrieve their initial capital or don't repay and still suffer losses as borrowing is overcollateralized.
This is a temporary DOS at best.

@0xjuaan
Copy link

0xjuaan commented Oct 15, 2024

As I explained, it's a DOS of liquidation which directly leads to bad debt when collateral value keeps dropping, where the attacker's borrowed funds is worth more than their collateral, so they steal funds via this DOS

They don't need to repay and collect their collateral as the debt is worth more (due to bad debt)

@cvetanovv
Copy link

I will accept the escalation. In theory, a malicious user with very large capital could DoS the liquidation without any constraints, and does not have to return the collateral.

Planning to accept the escalation and make this issue High severity.

@WangSecurity WangSecurity added High A High severity issue. and removed Medium A Medium severity issue. labels Oct 21, 2024
@WangSecurity
Copy link

Result:
High
Has duplicates

@sherlock-admin2 sherlock-admin2 removed the Escalated This issue contains a pending escalation label Oct 21, 2024
@sherlock-admin3 sherlock-admin3 added the Escalation Resolved This issue's escalations have been approved/rejected label Oct 21, 2024
@sherlock-admin4
Copy link
Contributor

Escalations have been resolved successfully!

Escalation status:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Escalation Resolved This issue's escalations have been approved/rejected Has Duplicates A valid issue with 1+ other issues describing the same vulnerability High A High severity issue. Reward A payout will be made for this issue
Projects
None yet
Development

No branches or pull requests

10 participants