hash - Incorrect decimal adjustment in `ChainlinkUsdOracle`

[sherlock-admin3]

hash

Medium

Incorrect decimal adjustment in ChainlinkUsdOracle

Summary

Incorrect decimal adjustment in ChainlinkUsdOracle

Vulnerability Detail

When adjusting for the decimals, the bracks are ommitted causing incorrect division

    function getValueInEth(address asset, uint256 amt) external view returns (uint256) {

        ....

        if (decimals <= 18) return (amt * 10 ** (18 - decimals)).mulDiv(uint256(assetUsdPrice), uint256(ethUsdPrice));
=>      else return (amt / (10 ** decimals - 18)).mulDiv(uint256(assetUsdPrice), uint256(ethUsdPrice));
    }

Eg: Decimals was 19, then instead of dividing by 10(19-18), the division will be performed by ~1019 itself. Casuing massive loss in the value

Impact

Incorrect valuation of assets breaking every calculation dependent on it, for eg: debt valuation,collateral valuation etc.

Code Snippet

https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/0b472f4bffdb2c7432a5d21f1636139cc01561a5/protocol-v2/src/oracle/ChainlinkUsdOracle.sol#L86

Tool used

Manual Review

Recommendation

Change to 10 ** (decimals - 18)

[iamnmt]

Escalate

Out of scope.

Per contest README

Tokens are whitelisted, only tokens with valid oracles can be used to create Base Pools. Protocol governance will ensure that oracles are only set for standard ERC-20 tokens (plus USDC/USDT)

A token that has decimals higher than 18 is a weird token. Since only standard ERC-20 tokens are in scope of this contest, the buggy line of code will never run.

[sherlock-admin3]

Escalate

Out of scope.

Per contest README

Tokens are whitelisted, only tokens with valid oracles can be used to create Base Pools. Protocol governance will ensure that oracles are only set for standard ERC-20 tokens (plus USDC/USDT)

A token that has decimals higher than 18 is a weird token. Since only standard ERC-20 tokens are in scope of this contest, the buggy line of code will never run.

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.

[cawfree]

Escalate

This is a good issue and well done to everyone that found it.

[sherlock-admin3]

Escalate

This is a good issue and well done to everyone that found it.

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.

[0xRstStn]

Escalate

This issue is valid. A token having more than 18 decimals does not make it not following the ERC20 standard. It is not defined anywhere that an ERC20 token must have a maximum of 18 decimals. See https://eips.ethereum.org/EIPS/eip-20. Otherwise, why is the possibility of having more than 18 decimals even coded in the protocol?

Escalate

Out of scope.

Per contest README

Tokens are whitelisted, only tokens with valid oracles can be used to create Base Pools. Protocol governance will ensure that oracles are only set for standard ERC-20 tokens (plus USDC/USDT)

A token that has decimals higher than 18 is a weird token. Since only standard ERC-20 tokens are in scope of this contest, the buggy line of code will never run.

[sherlock-admin3]

Escalate

This issue is valid. A token having more than 18 decimals does not make it not following the ERC20 standard. It is not defined anywhere that an ERC20 token must have a maximum of 18 decimals. See https://eips.ethereum.org/EIPS/eip-20. Otherwise, why is the possibility of having more than 18 decimals even coded in the protocol?

Escalate

Out of scope.

Per contest README

Tokens are whitelisted, only tokens with valid oracles can be used to create Base Pools. Protocol governance will ensure that oracles are only set for standard ERC-20 tokens (plus USDC/USDT)

A token that has decimals higher than 18 is a weird token. Since only standard ERC-20 tokens are in scope of this contest, the buggy line of code will never run.

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.

[4gontuk]

Escalate

This issue is valid. A token having more than 18 decimals does not make it not following the ERC20 standard. It is not defined anywhere that an ERC20 token must have a maximum of 18 decimals. See https://eips.ethereum.org/EIPS/eip-20. Otherwise, why is the possibility of having more than 18 decimals even coded in the protocol?

Escalate Out of scope. Per contest README

Tokens are whitelisted, only tokens with valid oracles can be used to create Base Pools. Protocol governance will ensure that oracles are only set for standard ERC-20 tokens (plus USDC/USDT)

A token that has decimals higher than 18 is a weird token. Since only standard ERC-20 tokens are in scope of this contest, the buggy line of code will never run.

High decimal (>18) tokens are in weird token category. And it`s simply OOS for this contest. https://github.com/d-xo/weird-erc20?tab=readme-ov-file#high-decimals

[Honour-d-dev]

I think when we talk about standard tokens we're not simply referring to token that follow the eip. Some non-standard token adhere to the eip but are still weird , for example USDC follows the eip, no? but the sponsor still had to mention it explicitly because having 6 decimals make it non-standard and you have to treat it differently when integrating it into your protocol because of that.

So i believe non-18 decimal tokens actually classify as non-standard as they require special handling

[0xRstStn]

All good points. I still think it is valid but will leave it to those who submitted it to defend it 😉

[cawfree]

Spoke with the sponsor before submitting this issue, unsure of whether they considered tokens over 18 decimals were weird tokens. They confirmed there was a definite probability that the protocol would whitelist tokens over 18 decimals, which would be catastrophic.

[Honour-d-dev]

Spoke with the sponsor before submitting this issue, unsure of whether they considered tokens over 18 decimals were weird tokens. They confirmed there was a definite probability that the protocol would whitelist tokens over 18 decimals, which would be catastrophic.

Then they should not have said " only standard erc20 + usdc" 🤷🏼‍♂️

[cvetanovv]

Currently, tokens over 18 decimals are considered non-standard, and the Readme states that only standard tokens will be used. Because of that, this issue is invalid.

Maybe in future contests, this rule for weird tokens will change a bit, and the sponsor will be asked to specify explicit weird traits of tokens they will use (decimals etc.)

Planning to accept @iamnmt escalation and invalidate the issue.

[WangSecurity]

Result: Invalid Has duplicates

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• iamnmt: accepted
• cawfree: rejected
• 0xRstStn: rejected

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/sentimentxyz/protocol-v2/pull/318