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.
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
:
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:
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:
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 inGenericLogic::calculateUserAccountData()
- L172-L177: