29186 - [SC - Insight] ValidationLogicvalidateBorrow - L-L Incorrect i...

ValidationLogic::validateBorrow() - L227-L231: Incorrect implementation of the intended (AAVE) protocol logic prevents all users with a health factor (HF) equal to HEALTH_FACTOR_LIQUIDATION_THRESHOLD from borrowing.

Submitted on Mar 10th 2024 at 00:25:30 UTC by @OxSCSamurai for Boost | ZeroLend

Report ID: #29186

Report type: Smart Contract

Report severity: Insight

Target: https://immunefi.com/

Impacts:

• Griefing (e.g. no profit motive for an attacker, but damage to the users or the protocol)

Description

Brief/Intro

ValidationLogic::validateBorrow() - L227-L231: Incorrect implementation of the intended (AAVE) protocol logic prevents all users with a health factor (HF) equal to HEALTH_FACTOR_LIQUIDATION_THRESHOLD from borrowing.

Vulnerability Details

Description:

Users won't be able to borrow or borrow more whenever their health factor (HF) value is EQUAL to the HEALTH_FACTOR_LIQUIDATION_THRESHOLD. This is not a correct representation of intended protocol logic. Intended protocol logic makes it possible for users to borrow/borrow more whenever their HF >= HEALTH_FACTOR_LIQUIDATION_THRESHOLD.

To add even more weight to my argument, see below where it's made clear that there are two different "minimum" HF liquidation thresholds, the absolute minimum allowed one, 0.95e18, where immediate liquidation probably occurs for any HF value below it, and the other more healthy "minimum" HF liq threshold with a value of 1e18:

  // Minimum health factor allowed under any circumstance
  // A value of 0.95e18 results in 0.95
  uint256 public constant MINIMUM_HEALTH_FACTOR_LIQUIDATION_THRESHOLD = 0.95e18;

  /**
   * @dev Minimum health factor to consider a user position healthy
   * A value of 1e18 results in 1
   */
  uint256 public constant HEALTH_FACTOR_LIQUIDATION_THRESHOLD = 1e18;

There shouldn't exist any valid logical reason why HEALTH_FACTOR_LIQUIDATION_THRESHOLD = 1e18 should be excluded as a valid minimum value as is incorrectly done in the below code snippet:

    require(
      vars.healthFactor > HEALTH_FACTOR_LIQUIDATION_THRESHOLD,
      Errors.HEALTH_FACTOR_LOWER_THAN_LIQUIDATION_THRESHOLD
    );

Here it's clear that the require check will revert whenever vars.healthFactor <= HEALTH_FACTOR_LIQUIDATION_THRESHOLD is true. The < is correct. It's the = in <= that's wrong, because the HEALTH_FACTOR_LIQUIDATION_THRESHOLD is a VALID minimum (healthy) HF value, and to exclude it like the current implementation does, is in fact completely wrong.

Not only that, but check the error description: Errors.HEALTH_FACTOR_LOWER_THAN_LIQUIDATION_THRESHOLD. It's clear what it says, but the code implementation doesn't reflect this.

Impact Details

POTENTIAL IMPACT IN SCOPE:

• Griefing -- no attacker or attack necessary, instead it's the bug itself causing griefing to the user/borrower, unfairly preventing them from borrowing when they should be able to borrow.

In case this contract is not in scope, it affects the results of at least the following contracts & functions in scope, therefore primacy of impact applies: WrappedTokenGatewayV3::borrowETH() and Pool::flashLoan() for both zksync and manta.

IMPACTS:

• Users won't be able to borrow or borrow more whenever their health factor (HF) value is EQUAL to the HEALTH_FACTOR_LIQUIDATION_THRESHOLD.

• Some users who might check out your smart contracts will notice that your intended protocol logic should allow for borrowing in the case where vars.healthFactor == HEALTH_FACTOR_LIQUIDATION_THRESHOLD, but that the expected behaviour is different from the actual behaviour, where it doesn't allow this.

• Specifically, the following functions will revert when called under above conditions, for both zksync and manta: -- WrappedTokenGatewayV3::borrowETH() -- Pool::flashLoan()

References

https://github.com/zerolend/core-contracts/blob/2448f46b6b472ba0f83a615f68aa8614866a8321/contracts/protocol/libraries/logic/ValidationLogic.sol#L227-L231 https://github.com/zerolend/core-contracts/blob/2448f46b6b472ba0f83a615f68aa8614866a8321/contracts/protocol/libraries/logic/GenericLogic.sol#L172-L176

Proof of Concept

PoC:

For some reason I couldnt get the tests to work in your local github repos, so I decided to run the same tests in AAVE's github repo, which should suffice.

And just for your records, here is the test error I get when I try run npm run test or yarn test in your repo:

Generating typings for: 134 artifacts in dir: types for target: ethers-v5
Successfully generated 296 typings!
Compiled 129 Solidity files successfully (evm target: london).
[BASH] Testnet environment ready
WARNING: You are currently using Node.js v21.6.2, which is not supported by Hardhat. This can lead to unexpected behavior. See https://hardhat.org/nodejs-versions


(node:29266) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)


  1) "before all" hook in "{root}"
·------------------------|---------------------------|----------------|----------------------------·
|  Solc version: 0.8.12  ·  Optimizer enabled: true  ·  Runs: 100000  ·  Block limit: 6718946 gas  │
·························|···························|················|·····························
|  Methods                                                                                         │
··············|··········|·············|·············|················|·············|···············
|  Contract   ·  Method  ·  Min        ·  Max        ·  Avg           ·  # calls    ·  usd (avg)   │
·-------------|----------|-------------|-------------|----------------|-------------|--------------·

  0 passing (3s)
  1 failing

  1) "before all" hook in "{root}":
     ProviderError: Method not found
      at HttpProvider.request (/mnt/DATA/DEV/WEB3_SEC/BUG_HUNTING/BUG_BOUNTIES/IMMUNEFI/CURRENT/BUG_BOUNTY_BOOSTS/zerolendxyz/zerolendxyz_REPO/core-contracts/node_modules/hardhat/src/internal/core/providers/http.ts:90:21)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)
      at DeploymentsManager.saveSnapshot (/mnt/DATA/DEV/WEB3_SEC/BUG_HUNTING/BUG_BOUNTIES/IMMUNEFI/CURRENT/BUG_BOUNTY_BOOSTS/zerolendxyz/zerolendxyz_REPO/core-contracts/node_modules/hardhat-deploy/src/DeploymentsManager.ts:1418:22)
      at Object.fixture (/mnt/DATA/DEV/WEB3_SEC/BUG_HUNTING/BUG_BOUNTIES/IMMUNEFI/CURRENT/BUG_BOUNTY_BOOSTS/zerolendxyz/zerolendxyz_REPO/core-contracts/node_modules/hardhat-deploy/src/DeploymentsManager.ts:323:9)
      at Context.<anonymous> (/mnt/DATA/DEV/WEB3_SEC/BUG_HUNTING/BUG_BOUNTIES/IMMUNEFI/CURRENT/BUG_BOUNTY_BOOSTS/zerolendxyz/zerolendxyz_REPO/core-contracts/test-suites/__setup.spec.ts:5:3)



error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

THE AAVE TESTS:

Command used for all tests: npm run test

TESTS:

• Test 1: With bug not fixed

• Test 2: With bug fixed

• Test 3: Control test.

Test 1:

• With bug not fixed and explicitly passing in vars.healthFactor = 1e18 via a slight modification in GenericLogic::calculateUserAccountData() - L172-L177:

    vars.healthFactor = 1e18; /// @audit added for PoC/testing purposes
    // vars.healthFactor = (vars.totalDebtInBaseCurrency == 0)
    //   ? type(uint256).max
    //   : (vars.totalCollateralInBaseCurrency.percentMul(vars.avgLiquidationThreshold)).wadDiv(
    //     vars.totalDebtInBaseCurrency
    //   ); /// @audit NOTE: this is required for some of the protocol tests to execute successfully...

Test 1 results:

  AToken: Mint and Burn Event Accounting
    ✔ User 1 supplies DAI
    ✔ User 1 supplies DAI on behalf of user 2

    1) User 2 supplies ETH,and borrows DAI
    .
    .
    .
    .
    .
    .
  63 passing (23s)
  1 failing

  1) AToken: Mint and Burn Event Accounting
       User 2 supplies ETH,and borrows DAI:
     Error: VM Exception while processing transaction: reverted with reason string '35'
    at BorrowLogic.validateBorrow (contracts/protocol/libraries/logic/ValidationLogic.sol:145)
    at BorrowLogic.executeBorrow (contracts/protocol/libraries/logic/BorrowLogic.sol:85)
    at Pool.liquidationCall (contracts/protocol/pool/Pool.sol:366)
    at Pool.borrow (contracts/protocol/pool/Pool.sol:219)
    at InitializableImmutableAdminUpgradeabilityProxy._delegate (contracts/dependencies/openzeppelin/upgradeability/Proxy.sol:42)
    at InitializableImmutableAdminUpgradeabilityProxy._fallback (contracts/dependencies/openzeppelin/upgradeability/Proxy.sol:71)
    at InitializableImmutableAdminUpgradeabilityProxy.<fallback> (contracts/dependencies/openzeppelin/upgradeability/Proxy.sol:18)
    at EdrProviderWrapper.request (node_modules/hardhat/src/internal/hardhat-network/provider/provider.ts:415:41)
    at EthersProviderWrapper.send (node_modules/@nomiclabs/hardhat-ethers/src/internal/ethers-provider-wrapper.ts:13:20)

In above test result with one failed test the reason string 35 is:

string public constant HEALTH_FACTOR_LOWER_THAN_LIQUIDATION_THRESHOLD = '35'; // 'Health factor is lesser than the liquidation threshold'

I explicitly passed in a HF exactly equal to the constant HEALTH_FACTOR_LIQUIDATION_THRESHOLD, and the buggy require check saw this as an invalid HF.

The next test was run with the bug fixed and still explicitly passing in vars.healthFactor = 1e18 as the borrower's HF.

Test 2: with bug fixed

    require(
      //vars.healthFactor > HEALTH_FACTOR_LIQUIDATION_THRESHOLD,
      vars.healthFactor >= HEALTH_FACTOR_LIQUIDATION_THRESHOLD, /// @audit added for PoC/testing purposes
      Errors.HEALTH_FACTOR_LOWER_THAN_LIQUIDATION_THRESHOLD
    );

Test 2 results (see my two @audit tags below):

  AToken: Mint and Burn Event Accounting
    ✔ User 1 supplies DAI
    ✔ User 1 supplies DAI on behalf of user 2
    ✔ User 2 supplies ETH,and borrows DAI 						/// @audit failed in test 1, passes here
    ✔ User 2 borrows more DAI - confirm mint event includes accrued interest
    ✔ User 1 - supplies more DAI - confirm mint event includes accrued interest
    ✔ User 1 supplies more DAI again - confirm mint event includes accrued interest
    ✔ User 2 repays all remaining DAI
    ✔ User 1 withdraws all deposited funds and interest
    ✔ User 2 borrows, pass time and repay DAI less than accrued debt
    ✔ User 1 withdraws amount less than accrued interest
    .
    .
    .
    .
    .
    .
  AToken: Transfer
    ✔ User 0 deposits 1000 DAI, transfers 1000 to user 0
    ✔ User 0 deposits 1000 DAI, disable as collateral, transfers 1000 to user 1
    ✔ User 0 deposits 1000 DAI, transfers 5 to user 1 twice, then transfer 0 to user 1
    ✔ User 0 deposits 1000 DAI, transfers to user 1
    ✔ User 0 deposits 1 WETH and user 1 tries to borrow the WETH with the received DAI as collateral

    1) User 1 tries to transfer all the DAI used as collateral back to user 0 (revert expected) /// @audit this is OK, it's because of the code snippet I commented out so that I could pass in `vars.healthFactor = 1e18` explicitly. Uncomment this code snippet and this test will pass.
    .
    .
    .
    .
    .
    .
  104 passing (27s)
  1 failing

  1) AToken: Transfer
       User 1 tries to transfer all the DAI used as collateral back to user 0 (revert expected):
     AssertionError: Expected transaction to be reverted with reason '35', but it didn't revert
      at Context.<anonymous> (test-suites/atoken-transfer.spec.ts:228:5)

Here the bugfixed worked, the user was able to borrow successfully >>> ✔ User 2 supplies ETH,and borrows DAI. Ignore the failed test, it's expected because I commented out the code snippet necessary for this failed test to pass...

Test 3: Control test.

• using test command: npm run test

• Here I removed all my changes that I made to the codebase for testing purposes, but I left my bugfix in there, and ran all the tests again, to see if my bugfix causes any issues with any of your tests. The bugfix again:

    require(
      //vars.healthFactor > HEALTH_FACTOR_LIQUIDATION_THRESHOLD,
      vars.healthFactor >= HEALTH_FACTOR_LIQUIDATION_THRESHOLD, /// @audit added for PoC/testing purposes
      Errors.HEALTH_FACTOR_LOWER_THAN_LIQUIDATION_THRESHOLD
    );

Test 3 test result:

.
.
.
  WadRayMath
    ✔ Plain getters
    ✔ wadMul()
    ✔ wadDiv()
    ✔ rayMul()
    ✔ rayDiv()
    ✔ rayToWad()
    ✔ wadToRay()

·--------------------------------------------------------------------------------------------|---------------------------|----------------|-----------------------------·
|                                    Solc version: 0.8.10                                    ·  Optimizer enabled: true  ·  Runs: 100000  ·  Block limit: 12450000 gas  │
········
.
.
.
.
.
.
·······
|  ZeroReserveInterestRateStrategy                                                           ·          -  ·          -  ·        232430  ·        1.9 %  ·          -  │
·--------------------------------------------------------------------------------------------|-------------|-------------|----------------|---------------|-------------·

  813 passing (3m)

NO issues, all AAVE tests ran successfully with my bugfix implemented, as expected.

THE BUGFIX:

ValidationLogic::validateBorrow() - L227-L231:

        require(
-           vars.healthFactor > HEALTH_FACTOR_LIQUIDATION_THRESHOLD,
+           vars.healthFactor >= HEALTH_FACTOR_LIQUIDATION_THRESHOLD,
            Errors.HEALTH_FACTOR_LOWER_THAN_LIQUIDATION_THRESHOLD
        );