danzero - Small amount of borrow can drain pool

[sherlock-admin4]

danzero

High

Small amount of borrow can drain pool

Summary

A user can supply some amount of a token and borrow a small amount of a token from the pool and withdraw the initial amount they supplied without repaying the borrowed amount which could cause insolvency for the pool.

Root Cause

https://github.com/sherlock-audit/2024-06-new-scope/blob/main/zerolend-one/contracts/core/pool/logic/ValidationLogic.sol#L124
The validateBorrow function In ValidationLogic.sol is used to validate borrow request from the user, currently it only requires that the borrowed amount is not 0, this allows user to borrow a minuscule amount of token.

https://github.com/sherlock-audit/2024-06-new-scope/blob/main/zerolend-one/contracts/core/pool/logic/ValidationLogic.sol#L239
The validateHealthFactor function in ValidationLogic.sol validate the requested amount of withdrawal from the user corresponding to the health factor. The healthFactor variable returned from the GenericLogic.calculateUserAccountData function is compared with the HEALTH_FACTOR_LIQUIDATION_THRESHOLD to be bigger or equal or else it reverts with an error.

https://github.com/sherlock-audit/2024-06-new-scope/blob/main/zerolend-one/contracts/core/pool/logic/GenericLogic.sol#L69
Inside the calculateUserAccountData function the vars.totalDebtInBaseCurrency variable determines the healthFactor variable. The vars.totalDebtInBaseCurrency variable is increased by the _getUserDebtInBaseCurrency function.

https://github.com/sherlock-audit/2024-06-new-scope/blob/main/zerolend-one/contracts/core/pool/logic/GenericLogic.sol#L184
Inside this function the usersTotalDebt variable is divided by the assetUnit variable which is fetched from the reserve configuration of the asset. This is a problem if the assetUnit is bigger than the usersTotalDebt as it will amount to 0 in the end which would set the healthFactor variable to type(uint256).max which would allow the user to withdraw the initial amount that is supplied while still keeping the borrowed amount.

Internal pre-conditions

1. Reserve Configuration of the asset needs to be borrowable :

• frozen: false
• borrowable: true

External pre-conditions

1. At least 1 user needs to supply the token in the pool for an attacker to exploit this vulnerability.

Attack Path

1. User 1 supply 1e18 wei of token A
2. Attacker supply 1e13 wei of token A at position index 0
3. Attacker borrow 1e9*9 wei of token A at position index 0
4. Attacker withdraw initial supplied amount of token A at position index 0
5. Attacker repeat step 2 - 4 for possibly thousands of times at incrementing position index (1,2,3....)
6. User 1 withdraw initial supplied amount of token A (Revert error)

Impact

• Pool insolvency
• Loss of funds for users
• Reputational damage for the protocol

PoC

// SPDX-License-Identifier: BUSL-1.1
pragma solidity 0.8.19;

import {MintableERC20} from '../../../../contracts/mocks/MintableERC20.sol';
import {PoolEventsLib, PoolSetup} from '../../core/pool/PoolSetup.sol';
import {console2} from '../../../../lib/forge-std/src/console2.sol';
import {stdError} from '../../../../lib/forge-std/src/StdError.sol';
import {DataTypes} from '../../../../contracts/core/pool/configuration/DataTypes.sol';
import {ReserveConfiguration} from '../../../../contracts/core/pool/configuration/ReserveConfiguration.sol';

contract mytest is PoolSetup {
  function setUp() public {
    _setUpPool();

    console2.log('This:', address(this));
    console2.log('msg.sender:', address(msg.sender));
    console2.log('owner:', address(owner));
    console2.log('whale:', address(whale));
    console2.log('ant:', address(ant));
    console2.log('governance:', address(governance));
  }

  function testSmallBorroWithdraw() external {
    
    uint256 index = 0;
    pos = keccak256(abi.encodePacked(address(owner), 'index', uint256(index)));
    uint256 mintAmount = 5 ether;

    tokenA.mint(owner, mintAmount);
    tokenA.mint(whale, mintAmount);
    tokenA.mint(ant, mintAmount);

    vm.startPrank(owner);
    tokenA.approve(address(pool), mintAmount * 1e9);

    vm.startPrank(ant);
    tokenA.approve(address(pool), mintAmount * 1e9);

    vm.startPrank(whale);
    tokenA.approve(address(pool), mintAmount * 1e9);

    console2.log('Owner Balance: ', tokenA.balanceOf(owner));
    console2.log('Ant Balance: ', tokenA.balanceOf(ant));
    console2.log('Whale Balance: ', tokenA.balanceOf(whale));
    console2.log('Pool Balance: ', tokenA.balanceOf(address(pool)));

    vm.startPrank(whale);
    pool.supplySimple(address(tokenA), whale, mintAmount, index);
    // pool.borrowSimple(address(tokenA), whale, mintAmount/2, index);
    // pool.withdrawSimple(address(tokenA), whale, mintAmount, index);

    vm.startPrank(owner);

    //Why only 4000 ? Avoid out of gas error, there could be a way to bypass this in foundry but due to time constraint, this is the best POC for now...
    for(uint i = 0; i < 4000; i++){
      pool.supplySimple(address(tokenA), owner, mintAmount, index+i);
      pool.borrowSimple(address(tokenA), owner, 1e9*9, index+i);
      pool.withdrawSimple(address(tokenA), owner, mintAmount, index+i);

      // console2.log(i);

    }

    // pool.borrowSimple(address(tokenA), owner, 1e9, index);

    // vm.startPrank(ant);
    // pool.supplySimple(address(tokenA), ant, mintAmount, index);
    // pool.borrowSimple(address(tokenA), ant, 1e9*9, index);
    // // pool.repaySimple(address(tokenA), 1e9*9, index);
    // pool.withdrawSimple(address(tokenA), ant, mintAmount, index);

    console2.log('Owner Debt: ', pool.getDebt(address(tokenA), owner, 0));
    // console2.log('Ant Debt: ', pool.getDebt(address(tokenA), ant, 0));

    console2.log('Owner Balance: ', tokenA.balanceOf(owner));
    // console2.log('Ant Balance: ', tokenA.balanceOf(ant));
    console2.log('Whale Balance: ', tokenA.balanceOf(whale));
    console2.log('Pool Balance: ', tokenA.balanceOf(address(pool)));

    vm.startPrank(whale);
    pool.withdrawSimple(address(tokenA), owner, mintAmount, index); //[FAIL. Reason: panic: arithmetic underflow or overflow (0x11)]

  }
}

Mitigation

Fix the require statement in validateBorrow function In ValidationLogic.sol such that it the minimum borrowed amount depends on the decimals of the asset borrowed. An attacker can get away borrowing 1e9 of an asset with 1e18 decimal but fails when it tries to borrow 1e10, hence the require statement could be something like "minimum borrow amount needs to be 8 decimal difference to the asset decimal". Could be something like this:

uint256 minAmount = 10 ** params.cache.reserveConfiguration.getDecimals() / 1e8;
require(params.amount >= minAmount, PoolErrorsLib.INVALID_AMOUNT);

[sherlock-admin3]

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

Honour commented:

Possibly invalid along with dupes #137 #148 #390. Code is a fork of AAVE and aave doesn't implement a min. borrow. Cant't drain pool as it would require 10^9 (txs/ loops in a single tx) to borrow 1e18 token worth of debt. it's difficult to say how much can actually be borrowed using this.

[git-denial]

I think this should be a unique issue on its own

Possible duplicates:
#11
#137
#148
#390

While these issues had some similarity with this report, there's no mention how user can "get away" by withdrawing the initial supplied amount.

I believe it became a whole other issue when user can get away by withdrawing their initial supplied amount.When the pool implementation is upgraded which includes the health check fix by the admin, they could possibly be liquidated if the user didn't withdraw.

[git-denial]

Also why is this not a high vulnerability ? This is a direct theft of user funds and protocol which could spread like a wildfire if many people discovered this vulnerability knowing the simplicity to execute the attack which eventually could trigger a bank-run.

[0xjuaan]

Escalate

This is a good catch, but the impact does not meet the criteria for medium severity.

Here is the code where the precision loss occurs:

uint256 userTotalDebt = balance.debtShares;
if (userTotalDebt != 0) userTotalDebt = userTotalDebt.rayMul(reserve.getNormalizedDebt());
userTotalDebt = assetPrice * userTotalDebt;

unchecked {
  return userTotalDebt / assetUnit;
}

For precision loss, we require userTotalDebt < assetUnit in the last line.

This means we require assetPrice*userTotalDebt < assetUnit

This means that userTotalDebt < assetUnit where assetPrice is the price returned by getAssetPrice() which is from the chainlink feed.

Most chainlink feeds return values in 8 decimals.

Take for example assetPrice = 1e8 (DAI)

In this case, assetUnit = 1e18

Hence in order for precision loss, we require userTotalDebt < 1e10

So each iteration, we can borrow less than 1e10 assets

This means it takes 100 million iterations of supply->borrow->withdraw in order to profit $1. (Since assetPrice is 1e8)

With 4000 iterations per transaction, 25000 transactions are required.

While the above transactions will use extreme amounts of gas we'll optimistically assume they take the average gas cost of an L2 which is $0.02

The cost would be $0.02 * 25k = $500, to earn $1.

Side note:
Note that if an asset like ETH is used instead, while the assetPrice increases, this proportionally decreases the maximum userTotalDebt which can be borrowed, so the attack cost remains unchanged regardless of the asset used.

Also, this won't work at all for lower decimal assets like USDT/USDC since userTotalDebt*assetPrice (debt * 1e8) will always be greater than assetUnit (1e6)

[sherlock-admin3]

Escalate

This is a good catch, but the impact does not meet the criteria for medium severity.

Here is the code where the precision loss occurs:

uint256 userTotalDebt = balance.debtShares;
if (userTotalDebt != 0) userTotalDebt = userTotalDebt.rayMul(reserve.getNormalizedDebt());
userTotalDebt = assetPrice * userTotalDebt;

unchecked {
  return userTotalDebt / assetUnit;
}

For precision loss, we require userTotalDebt < assetUnit in the last line.

This means we require assetPrice*userTotalDebt < assetUnit

This means that userTotalDebt < assetUnit where assetPrice is the price returned by getAssetPrice() which is from the chainlink feed.

Most chainlink feeds return values in 8 decimals.

Take for example assetPrice = 1e8 (DAI)

In this case, assetUnit = 1e18

Hence in order for precision loss, we require userTotalDebt < 1e10

So each iteration, we can borrow less than 1e10 assets

This means it takes 100 million iterations of supply->borrow->withdraw in order to profit $1. (Since assetPrice is 1e8)

With 4000 iterations per transaction, 25000 transactions are required.

While the above transactions will use extreme amounts of gas we'll optimistically assume they take the average gas cost of an L2 which is $0.02

The cost would be $0.02 * 25k = $500, to earn $1.

Side note:
Note that if an asset like ETH is used instead, while the assetPrice increases, this proportionally decreases the maximum userTotalDebt which can be borrowed, so the attack cost remains unchanged regardless of the asset used.

Also, this won't work at all for lower decimal assets like USDT/USDC since userTotalDebt*assetPrice (debt * 1e8) will always be greater than assetUnit (1e6)

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.

[git-denial]

Escalate

This is a good catch, but the impact does not meet the criteria for medium severity.

For precision loss, we require userTotalDebt < assetUnit in the last line.

This means we require assetPrice*userTotalDebt < assetUnit

This means that userTotalDebt < assetUnit where assetPrice is the price returned by getAssetPrice() which is from the chainlink feed.

Most chainlink feeds return values in 8 decimals.

Take for example assetPrice = 1e8 (DAI)

In this case, assetUnit = 1e18
.....................................................

Thanks for the escalation and the breakdown, I agree that this should not be a medium severity. This should be a high severity, however for the record, this report also meets the medium vulnerability:

From https://docs.sherlock.xyz/audits/real-time-judging/judging#v.-how-to-identify-a-medium-issue:

Note: If a single attack can cause a 0.01% loss but can be replayed indefinitely, it will be considered a 100% loss and can be medium or high, depending on the constraints.

This attack can be replayed indefinitely on almost every token out there given 18 decimal format token is the default standard for ERC20 token. Some ways this attack can be stopped is if the admin froze the asset (which could be temporary) or all lenders decided to withdraw the asset.

And for why this deserves a high severity, from https://docs.sherlock.xyz/audits/real-time-judging/judging#iv.-how-to-identify-a-high-issue :

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

Take an example, a lender that lends $100 in a $100_000 pool, it is guaranteed that this attack will eventually cause a $100 loss given it can be replayed indefinitely, maybe not in 1 day. But as more users discover this vulnerability, the loss could scale up pretty quick.

[cvetanovv]

I completely agree with the escalation: #291 (comment)

The attack exceeds the gain many times over. This is no more than Low severity.

Planning to accept the escalation and invalidate the issue.

[Almur100]

Issue #137 , issue #148 , issue #390 should be duplicated together and this is a known issue, #137,#148,#390 is different from this issue.

[git-denial]

I completely agree with the escalation: #291 (comment)

The attack exceeds the gain many times over. This is no more than Low severity.

Planning to accept the escalation and invalidate the issue.

The gain does not matter, when evaulating the severity of a vulnerability, Sherlock rules don't consider the gain of the attacker.

[git-denial]

Issue #137 , issue #148 , issue #390 should be duplicated together and this is a known issue, #137,#148,#390 is different from this issue.

Yes, additionally please dedupe #11 as well

[cvetanovv]

Issue #137 , issue #148 , issue #390 should be duplicated together and this is a known issue, #137,#148,#390 is different from this issue.

@Almur100 I don't understand what you mean by that comment. You want all duplicates of this issue to be reviewed separately from this issue?

@git-denial

The gain does not matter, when evaulating the severity of a vulnerability,

You mean someone would pay $500 to get $1, and it's an economically viable attack to be a valid Medium?

[Almur100]

No , I am saying #137,#148,#390 are explaining the same problem which is different from the issue #291. Similar issue is accepted in sentiment v2, see the sentiment v2 issue , the link is sherlock-audit/2024-08-sentiment-v2-judging#572

min borrow amount should be set in lending and borrowing protocol, but in zerolend there is no check for borrowing minimum amounts, Attackers can borrow multiple small amounts of positions which may not be profitable during liquidation. This is the problem which is explained in issue #137, issue #148, issue #390.

[git-denial]

@cvetanovv

You mean someone would pay $500 to get $1, and it's an economically viable attack to be a valid Medium?

1. If the attack only could cause $1 loss then no, but that is not the case in this attack, the potential loss for this attack is infinite since this attack can be replayed indefinitely.

2. I think this should be high severity, I have given my reasoning above but to expand on it, due to the simplicity to execute this attack, this attack will not be done by a single person. Some people or influencer could share this attack as "tips and hacks on Zerolend" or something along that line with a link to the script which could easily execute the attack 1000 times. If we follow the calculation that 25k transaction could steal $1, and this attack is discovered by 10k people and they only run the script once, then the total transaction that is done for this attack is 10,000 * 1,000 = 10M transaction which means the total loss of funds is $400. A lender lending $100 in a pool would experience a 100% loss in this case. Of course, $400 is not the limit of the total loss of funds caused by this attack since this attack could be replayed indefinitely.

[Honour-d-dev]

@cvetanovv

You mean someone would pay $500 to get $1, and it's an economically viable attack to be a valid Medium?

1. If the attack only could cause $1 loss then no, but that is not the case in this attack, the potential loss for this attack is infinite since this attack can be replayed indefinitely.
2. I think this should be high severity, I have given my reasoning above but to expand on it, due to the simplicity to execute this attack, this attack will not be done by a single person. Some people or influencer could share this attack as "tips and hacks on Zerolend" or something along that line with a link to the script which could easily execute the attack 1000 times. If we follow the calculation that 25k transaction could steal $1, and this attack is discovered by 10k people and they only run the script once, then the total transaction that is done for this attack is 10,000 * 1,000 = 10M transaction which means the total loss of funds is $400. A lender lending $100 in a pool would experience a 100% loss in this case. Of course, $400 is not the limit of the total loss of funds caused by this attack since this attack could be replayed indefinitely.

Following your example, executing the script will cost each person $20 ,to borrow $0.04. No follower would do this.

Currently AAVE doesn't implement a min. borrow as well, see here , the influencer doesn't have to wait for zerolend, if this was realistic

[git-denial]

Following your example, executing the script will cost each person $20 ,to borrow $0.04. No follower would do this.

The person sharing this attack will not share how much profit will be made, just how it is possible to steal someone's token. People that listen to this type of tips are usually not so savvy, they just execute the script and think later before they realized the results. Some would even try to execute the script more than once.

Currently AAVE doesn't implement a min. borrow as well, see here , the influencer doesn't have to wait for zerolend, if this was realistic

AAVE has different health check logic, you can try it yourself it will not work.

[Honour-d-dev]

The person sharing this attack will not share how much profit will be made, just how it is possible to steal someone's token. People that listen to this type of tips are usually not so savvy, they just execute the script and think later before they realized the results. Some would even try to execute the script more than once.

I don't think this logic can be used to validate any attack especially on sherlock. Savvy or not, i believe such people just want to make a profit rather than "steal someone's tokens" and loose money in return.

Also the attack cannot be replayed indefinitely as claimed due to the heavy cost on the attacker

[git-denial]

I don't think this logic can be used to validate any attack especially on sherlock. Savvy or not, i believe such people just want to make a profit rather than "steal someone's tokens" and loose money in return.

We are talking about users here, not admin, so we can't expect them to be rational.

Also the attack cannot be replayed indefinitely as claimed due to the heavy cost on the attacker

Using that logic, even a 1 penny transaction cost could stop an indefinite replay attack if the attacker only had $1 in their wallet.

It can be replayed indefinitely, because anyone can exploit this bug at anytime.

[cvetanovv]

The implementation is the same as in AAVE, and there is no issue.

In the Sentiment contest, a similar issue is valid because it is stated in the readme that the minimum borrow/debt amount can be 0 or a very low value. Here, the situation is different; it can still be low value, but the user has no initiative to take small borrowings.

Furthermore, the readme states that the protocol will use "liquidation bots that run off-chain to execute liquidations". All the duplicates indicate that the protocol will get bad debt. But that won't happen because the bots will take care of this.

My decision to accept the escalation and invalidate the issue remains.

[git-denial]

The implementation is the same as in AAVE, and there is no issue.

In the Sentiment contest, a similar issue is valid because it is stated in the readme that the minimum borrow/debt amount can be 0 or a very low value. Here, the situation is different; it can still be low value, but the user has no initiative to take small borrowings.

Furthermore, the readme states that the protocol will use "liquidation bots that run off-chain to execute liquidations". All the duplicates indicate that the protocol will get bad debt. But that won't happen because the bots will take care of this.

My decision to accept the escalation and invalidate the issue remains.

I don't think you have fully understood the issue, there are 2 different root causes mentioned in this issue : The non-zero borrow amount requirement and the calculateUserAccountData function.

As long as the attacker borrow amount is under 1e10 for a 18 token decimal, their debt will be deemed as 0 from the calculateUserAccountData function. As a result, the user's health factor is still at type(uint256).max. So, liquidation bot won't help here.

[git-denial]

Furthermore, the readme states that the protocol will use "liquidation bots that run off-chain to execute liquidations". All the duplicates indicate that the protocol will get bad debt. But that won't happen because the bots will take care of this.

The duplicates of this issue only stops at borrowing and hence the impact is limited to bad debts. Here, the token has been withdrawn, the owner of the token is the user, can't be liquidated, the fund has been lost, it will not return to the pool nor the protocol, it is not a case of bad debt anymore, it is theft of funds which could lead to insolvency.

[cvetanovv]

I don't think you have fully understood the issue, there are 2 different root causes mentioned in this issue : The non-zero borrow amount requirement and the calculateUserAccountData function.

As long as the attacker borrow amount is under 1e10 for a 18 token decimal, their debt will be deemed as 0 from the calculateUserAccountData function. As a result, the user's health factor is still at type(uint256).max. So, liquidation bot won't help here.

@git-denial Nobody would borrow 1e10 for an 18-decimal token. I don't think you understand how 1e10 is smaller than 1e18. This is 0.00000001 of the token's value. The attack is economically unsustainable.

[git-denial]

@git-denial Nobody would borrow 1e10 for an 18-decimal token. I don't think you understand how 1e10 is smaller than 1e18.
This is 0.00000001 of the token's value. The attack is economically unsustainable.

@cvetanovv
Oh, somebody would, when profit is no longer a concern, attackers will use everything at their disposal.

You don't understand how 1e10 can become 1000e18.

I have provided a scenario where this attack can be done by many people to make it more 'economically sustainable'.

Downplaying a vulnerability like this is why hackers prefer to hack protocols themselves.

[git-denial]

@cvetanovv
I suggest you read again on how to evaluate a finding according to Sherlock rules here: https://docs.sherlock.xyz/audits/real-time-judging/judging#ii.-criteria-for-issue-severity

There's 0 mention of attacker's profit, economic sustainability, etc. And if you think it implies likelihood, there's also 0 mention of it.

[git-denial]

@cvetanovv
You can look at your own judgement regarding attacker's profit here: #141

Although the grief attack will also cause the malicious user to take losses, it is possible, and I think Medium severity > is appropriate for this issue.

And regarding likelihood here: #437

This issue shows an edge case situation where totalSupply is zero, and the rewards during this period will be lost.
Planning to accept the escalation and make this issue Medium.

[cvetanovv]

When the attack costs 500 times the gain, it is not an adequate grief attack, and no one will do it.

Juan has shown a very good example:

The example that the attacker can share the attack cannot be taken into consideration because it is not written in the issue itself. But even if we take it into account, the cost of the attack remains the same.

Someone to earn $500 would have to pay $250,000 in gas taxes. This issue is no more than Low/Information severity.

My decision is to accept the escalation and invalidate the issue.

[WangSecurity]

Result:
Invalid
Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0xjuaan: accepted