nfmelendez - GenericLogic.sol contract assumes all price feeds has the same decimals but is a wrong assumption that leads to an incorrect health factor math.

[sherlock-admin4]

nfmelendez

High

GenericLogic.sol contract assumes all price feeds has the same decimals but is a wrong assumption that leads to an incorrect health factor math.

Summary

Mixing price feeds decimals when doing the calculation of totalCollateralInBaseCurrency and totalDebtInBaseCurrency will cause an incorrect healthFactor affecting important operations of the protocol such as liquidation, borrow and withdraw. GenericLogic.sol contract assumes all price feeds have the same decimals but is a wrong assumption as is demonstrated in the Root cause section.

https://github.com/sherlock-audit/2024-06-new-scope/blob/main/zerolend-one/contracts/core/pool/logic/GenericLogic.sol#L106-L128

Root Cause

GenericLogic.sol:69 calculateUserAccountData calculates the totalCollateralInBaseCurrency and the totalDebtInBaseCurrency doing a sum of all the differents reserve assets as collateral or debt in base currency but the problem is a wrong assumption that all chainlink price feeds has the same decimals. Most USD price feeds has 8 decimals but for example AMPL / USD feed has 18 decimals. So totalCollateralInBaseCurrency and the totalDebtInBaseCurrency will be incorrect because calculateUserAccountData will sum asset prices with different decimals leading to a wrong calculation of the health factor and incorrect function of many protocol operations.

Internal pre-conditions

1. Price feeds for collateral or debt assets in a given position needs to have different decimals.

External pre-conditions

None

Attack Path

Is a wrong assumption proven by example

Impact

1. Liquidation: Mixing Price decimals lead to incorrect calculation of the healthFactor that is a result of wrong totalCollateralInBaseCurrency and the totalDebtInBaseCurrency.
2. Borrow: Wrong healthFactor also affects borrowing when doing validations to make sure that the position is not liquiditable.
3. Withdraw: Also uses healthFactor via ValidationLofic::validateHFAndLtv
4. executeUseReserveAsCollateral: Also uses healthFactor via ValidationLofic::validateHFAndLtv
5. Any other operation that uses the health factor.

PoC

none

Mitigation

There are 2 possible solution:

1. Some protocols enforce 8 decimals when assigning an oracle to an asset or reject the operation. (easy, simple, secure, not flexible)
2. Use AggregatorV3Interface::decimals to normalize to N decimals the price making sure that the precision loss is on the correct side. (flexible)

[nevillehuang]

As seen here

Q: Are there any limitations on values set by admins (or other roles) in protocols you integrate with, including restrictions on array lengths?
No

There is no limits to admin set chainlink oracles, so it is presumed that they will act accordingly when integrating tokens.

(External) Admin trust assumptions: When a function is access restricted, only values for specific function variables mentioned in the README can be taken into account when identifying an attack path.

If no values are provided, the (external) admin is trusted to use values that will not cause any issues.

[Honour-d-dev]

Escalate

this issue is valid!

The above comment is not a valid reason for it to be invalid, pools are permissionless and anyone can create a pool and choose to integrate these tokens with un-conventional price feed decimals and the impact on users can be severe especially if not detected early.

If the argument is that the oracle can be removed or the token can be paused by admin if such an issue occurs, the impact is still severe (loss of funds for users , liquidation etc) and not reversible. Such cases can be easily prevented by the mitigation provided in the report.

[sherlock-admin3]

Escalate

this issue is valid!

The above comment is not a valid reason for it to be invalid, pools are permissionless and anyone can create a pool and choose to integrate these tokens with un-conventional price feed decimals and the impact on users can be severe especially if not detected early.

If the argument is that the oracle can be removed or the token can be paused by admin if such an issue occurs, the impact is still severe (loss of funds for users , liquidation etc) and not reversible. Such cases can be easily prevented by the mitigation provided in the report.

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.

[cvetanovv]

The Lead Judge is right with his comment: #166 (comment)

In addition, pool managers are expected to act rationally(i.e. are trusted):

Essentially we expect all permissioned actors to behave rationally.

There are two set of actors. Actors who manage pools and actors who mange vaults. If an action done by one party causes the other party to suffer losses we'd want to consider that.

The only valid variant of this issue is if there was mention of the malicious attack path. Then, I would duplicate it with #234.

Planning to reject the escalation and leave the issue as is.

[Honour-d-dev]

@cvetanovv

The point here is pool creation is permissionless, so anyone can create a pool (become a pool manager) and add whichever tokens they like to their pool. The possibility of adding an oracle with wrong decimals cannot be attributed to irrational behavior(or being malicious) as there is no guarantee that the pool manager is aware of the fact that price feed decimals are not normalized (see second recommendations #166 and #442 ) in zerolend.

This can happen even if pool managers behave rationally (with good intentions) and can cause severe and irreversible damage to users before it's corrected. It's better to completely prevent the possibility of such cases as the chances of this happening is pretty high given the permissionless nature of pools.

[cvetanovv]

@Honour-d-dev

If the pool manager did everything right, then there is no issue here. By default, he is the external admin and, by default, should do everything correctly. If not, it is a user mistake.

@nevillehuang explained it in the first comment.

My decision to reject the escalation remains.

[Honour-d-dev]

@cvetanovv i appreciate the effort 🙏

Consider this. Alice creates a pool and decides to add the AML token to her pool and she also adds the chainlink AML/USD oracle to this pool.

From every perspective Alice has done everything right.
This cannot be grouped as user mistakes because user mistakes only hurt themselves, hence why they are invalid.

In this case Alice is a pool manager and this issue will affect all users of their pool not just Alice.

As I mentioned in previous comments I believe the chances of this happening is very high.
And it would be unfair to pool users to call it a user mistake.

[samuraii77]

@cvetanovv. how would a pool manager do everything right though? If he wants to create a pool for an asset that has a different amount of decimals in Chainlink, then this will always lead to an error. His only way of avoiding this is not creating a pool with such an asset which I don't believe is a fair reason to classify this as invalid.

[cvetanovv]

@Honour-d-dev @samuraii77 I understand your points.

The protocol will use standard tokens, but some standard tokens may return a different price due to the lack of decimal scaling.
This means that the pool manager will not be able to use them, although it is mentioned in the readme that they can be used.

This means no working functionality because they will not be used because the pool manager has to act rationally, and using them will cause a bug - Medium severity.

Reference: https://ethereum.stackexchange.com/questions/92508/do-all-chainlink-feeds-return-prices-with-8-decimals-of-precision, https://ethereum.stackexchange.com/questions/90552/does-chainlink-decimal-change-over-time

I am planning to accept the escalation and make this issue Medium severity.

[WangSecurity]

Result:
Medium
Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• Honour-d-dev: accepted

[coffiasd]

@cvetanovv those issues that dups with this issue are also valid or not ?

[cvetanovv]

@coffiasd, the duplicates are also valid. Labels are added at the end after the escalations are over.