search

hats-finance / VMEX-0x050183b53cf62bcd6c2a932632f8156953fd146f

LP token lending protocol
MIT License
2 stars 1 forks source link

VMEX would overvalue collateral if if the token's USD price feed's `decimals != 8` #13

Open hats-bug-reporter[bot] opened 1 year ago

hats-bug-reporter[bot] commented 1 year ago

Github username: @abhishekvispute Submission hash (on-chain): 0xb0e84c6d8e6ff1df0dc79d11d6db45ea73ae669287a9d0a9706333c85878389e Severity: medium severity

Description:

Description :

VMEX makes the assumption that all USD feeds would be of 8 decimals, and have not handled price feed decimals anywhere explicitly, considering it would always be compared in the same denomination of base currency. However this is not always the case, sometimes chainlink price feeds for USD have decimals ≠8 For example, AMPL/USD on mainnet has 18 decimals https://etherscan.io/address/0xe20CA8D7546932360e37E9D72c1a47334af57706#code

Hence it is not guaranteed from the chainlink side that the constraint of decimals being 8 is always satisfied.

In some places where this is not considered, https://github.com/hats-finance/VMEX-0x050183b53cf62bcd6c2a932632f8156953fd146f/blob/fb396a3fa412e643de7d8a1fd8a0268ab3a2f993/packages/contracts/contracts/protocol/oracles/VelodromeOracle.sol#L49

https://github.com/hats-finance/VMEX-0x050183b53cf62bcd6c2a932632f8156953fd146f/blob/fb396a3fa412e643de7d8a1fd8a0268ab3a2f993/packages/contracts/contracts/protocol/libraries/logic/GenericLogic.sol#L181

Attack Scenario:

Deposit Token (eg: AMPL), Borrow more than allowed since the collateral value is calculated incorrectly

Likelihood

Considering that this requires the VMEX team to allow such a token, this is not a high likelihood, however, it is not a rare occurrence as well, since AAVE themselves support AMPL on mainnet.

Recommendation:

Either normalize prices in getAssetPrice() explicitly to base asset decimals

or

Verify inside setAssetSources() that decimals of price feed are equal to base asset decimals only

0xcuriousapple commented 1 year ago

One example from the wild https://solodit.xyz/issues/3350 https://github.com/sentimentxyz/oracle/blob/815233add2d23a7e2a2c5136504537b234a65c47/src/chainlink/ChainlinkOracle.sol#L93

ksyao2002 commented 1 year ago

Thanks for the review. This is a valid concern; however, on OP are there any USD oracles that don't have 8 decimals? If so, I will agree with your rating as a medium sev. However, if all OP USD oracles have 8 decimals, I would say that this is a low sev since no funds will be at risk. Regardless, it is good to add the guard you mentioned that checks that the decimals are what we expect. On mainnet we use ETH as the denomination so the issue doesn't apply.

Please let me know your thoughts.

0xcuriousapple commented 1 year ago

makes sense, low is fair