MVKsentry - Improper handling of price normalization to `e18` in `RedstoneOracle.sol#getValueInEth`

[sherlock-admin4]

MVKsentry

High

Improper handling of price normalization to e18 in RedstoneOracle.sol#getValueInEth

Summary

The RedstoneOracle#getValueInEth returns price of ASSSETe18 mutiplied by (ASSET/USD) / (ETH/USD)which are gotten from Redstone Oracles and not scaled to 18 decimals as the ASSET.

The problem is that Redstone Oracles are 8 decimals by default which can be seen here Redstone price feeds decimals

Root Cause

The issue itself lies in the normalization to e18 of the returned by the method price.

https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/main/protocol-v2/src/oracle/RedstoneOracle.sol#L67-L71

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

Financial losses due to inaccuracy in the math.

PoC

As per openzeppelin math mulDiv function parameters x = amt: Scaled to 18 decimals. y = assetUsdPrice: 8 decimals. denominator = ethUsdPrice: 8 decimals.

The calculation performed by the getValueInEth function is:

\text{Result} = \frac{\text{amt} \times \text{assetUsdPrice}}{\text{ethUsdPrice}}

Substituting the values:

\text{Result} = \frac{(1 \times 10^{18}) \times (1 \times 10^{8})}{2 \times 10^{8}}

Simplifying the expression:

\text{Result} = \frac{1 \times 10^{26}}{2 \times 10^{8}} = \frac{1 \times 10^{26}}{2 \times 10^{8}} = 0.5 \times 10^{18} = 5 \times 10^{17}

Thus, the function will return:

`\boxed{5 \times 10^{17}}

Mitigation

Consider normalizing the prices of assetUsdPrice, ethUsdPrice to 1e18 OR the asset price to 1e8 and after that the calculation outcome value to 1e18

[samuraii77]

The example given works completely as intended. The user has provided 1 asset of some asset which was then normalized to 18 decimals, that asset is worth 1$ while ETH is worth 2$. Thus, the user has half an ETH which is what the function returns - 5e17.

[zarkk01]

Escalate

For above comment.

[sherlock-admin3]

Escalate

For above comment.

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.

[Anirruth]

The duplicate is a different issue.

[Anirruth]

When checking this issue, do check the duplicate : 604 as its a different issue.

[MarioBozhikov]

@samuraii77, in the case of an asset worth $1 and ETH worth $2, your math checks out, especially for small amounts. However, when dealing with larger amounts of assets or assets with low prices, the function can return inflated or deflated values due to the mismatch between the 18-decimal asset and 8-decimal price feeds. This discrepancy causes the result to be inaccurate, especially when scaling to larger or smaller values.

[MarioBozhikov]

@Anirruth Both issues are caused by the mismatch between the 8-decimal price feeds from Redstone Oracles and the 18-decimal arithmetic used in the contract.

[Anirruth]

In this issue its given that assetusdPrice and ethusdPrice are with 8 decimal precision, but my submission shows there are some standard erc20 tokens which returns with 1e18 decimal precision in different chains, which the code doesn't take into consideration. So in some instance the ethusdPrice will be in 8 decimal precision while the assetusdPrice will be in 18 decimal precision which will increase the amount by 10 times.

[ruvaag]

this should be invalid imo.

it is not an issue if both assetUsdPrice and ethUsdPrice are 8 decimals since they cancel out without loss of precision. there might be something to be said about non-standard redstone feeds that don't have 8 decimals, but that is not part of the discussion here either.

[Anirruth]

the dup of this issue 604 states a different issue, not related to this. The dup states the precision difference on different chains.

[cvetanovv]

I agree with the sponsor's comment that we have no problem here. #604 is also invalid.

If #103 and #604 try to write a real PoC(which is mandatory for this type of issue), they will see that we have no issue.

Planning to accept the escalation and invalidate the issue.

[Anirruth]

I agree with the sponsor's comment that we have no problem here. #604 is also invalid.

If #103 and #604 try to write a real PoC(which is mandatory for this type of issue), they will see that we have no issue.

Planning to accept the escalation and invalidate the issue.

Could you explain why 604 is invalid, since it shows that erc20 standard following tokens might have different precision (such as 1e18 as linked in the report) for chainlink price feeds in different chains while ethusd price is always in 1e8 precision, which causes the calculation to be 10 times higher since it divides assetUsdPrice(1e18) / ethUsdPrice(1e8) and doesn't take the precision difference into consideration . Whereas 103 is not related to 604 at all and doesn't mention anything about chainlink precision difference in different chain at all.

[Anirruth]

The etherscan address linked is from this - https://data.chain.link/feeds/arbitrum/mainnet/pepe-usd . If you click on the contract address and check the decimals in the read contract section on etherscan it shows 18. The contest details states that :

On what chains are the smart contracts going to be deployed?

Any EVM-compatbile network

But it doesn't take into account the precision difference in different chains. The token's chainlink oracle price in arbitrum is in 1e18 precision while in other chains it's in 1e8. The contract doesn't check the precision difference and always divides with the ethusd price which is in 1e8 precision. This is shown in issue 604. Hence why issue 604 should be valid.