Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

000000 - Not including Chainlink decimals upon calculations will lead to accounting issues #38

Closed
sherlock-admin3 opened this issue Sep 10, 2024 · 0 comments
Labels
Non-Reward This issue will not receive a payout

Comments

@sherlock-admin3
Copy link
Contributor

sherlock-admin3 commented Sep 10, 2024

000000

High

Not including Chainlink decimals upon calculations will lead to accounting issues

Summary

Not including Chainlink decimals upon calculations will lead to accounting issues

Vulnerability Detail

Upon calculating prices in the base currency, we use GenericLogic::_getUserDebtInBaseCurrency() or GenericLogic::_getPositionBalanceInBaseCurrency():

function _getUserDebtInBaseCurrency(
    DataTypes.PositionBalance storage balance,
    DataTypes.ReserveData storage reserve,
    uint256 assetPrice,
    uint256 assetUnit
  ) private view returns (uint256) {
    // fetching variable debt
    uint256 userTotalDebt = balance.debtShares;
    if (userTotalDebt != 0) userTotalDebt = userTotalDebt.rayMul(reserve.getNormalizedDebt());
    userTotalDebt = assetPrice * userTotalDebt;

    unchecked {
      return userTotalDebt / assetUnit;
    }
  }

The issue is that the assetPrice is provided as it is after being fetched from Chainlink. Chainlink feeds return prices in different decimals (most commonly 8 and 18) and there is no clear pattern as to how many decimals will a feed return:

There are also price feeds such as AMPL/USD that report using 18 decimals which breaks the general rule that USD price feeds report in 8 decimals. (https://medium.com/cyfrin/chainlink-oracle-defi-attacks-93b6cb6541bf)

Thus, for some assets, we will have a much larger or smaller value depending on the amount of decimals a Chainlink feed returns which will be used for important calculations such as the health factor of a user. A user might be unfairly liquidated or unfairly kept with a healthy position due to that. Note that the assetUnit input in the function are the decimals of the asset, not the decimals which Chainlink returns.

Impact

Not including Chainlink decimals upon calculations will lead to accounting issues

Code Snippet

https://github.com/sherlock-audit/2024-06-new-scope/blob/c8300e73f4d751796daad3dadbae4d11072b3d79/zerolend-one/contracts/core/pool/logic/GenericLogic.sol#L184-L219

Tool used

Manual Review

Recommendation

Fetch the Chainlink decimals as well and include them in the calculation so that you always return the same amount of decimals

Duplicate of #166

@github-actions github-actions bot added Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label High A High severity issue. labels Sep 20, 2024
@nevillehuang nevillehuang removed the High A High severity issue. label Oct 2, 2024
@sherlock-admin3 sherlock-admin3 changed the title Acrobatic Rainbow Grasshopper - Not including Chainlink decimals upon calculations will lead to accounting issues 000000 - Not including Chainlink decimals upon calculations will lead to accounting issues Oct 3, 2024
@sherlock-admin3 sherlock-admin3 added Non-Reward This issue will not receive a payout and removed Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Non-Reward This issue will not receive a payout
Projects
None yet
Development

No branches or pull requests

2 participants