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

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.