The Standard

Although it appears to be out of scope, I do want to mention the PriceCalculator contract is not handling Oracle data correctly. Because the SmartVault highly depends on these results, I still think it's important to mention this issue.

I've combined both issues in the PriceCalculator contract. Please mitigate these issues

1. Oracle price data

All methods don't validate the Oracle return value suffficiently

avgPrice
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/utils/PriceCalculator.sol#L23
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/utils/PriceCalculator.sol#L29
tokenToEurAvg
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/utils/PriceCalculator.sol#L47
tokenToEur
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/utils/PriceCalculator.sol#L54
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/utils/PriceCalculator.sol#L56
eurToToken
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/utils/PriceCalculator.sol#L62-L63

Vulnerability details

There is no validation to check if the answer (or price) received was actually a stale one. Reasons for a price feed to stop updating are listed here. Using a stale price in the application can result in wrong calculations.

https://docs.chain.link/data-feeds/api-reference

Impact

According to Chainlink's documentation, This function does not error if no answer has been reached but returns 0, causing an incorrect price fed. The external Chainlink oracle, which provides index price information to the system, introduces risk inherent to any dependency on third-party data sources. For example, the oracle could fall behind or otherwise fail to be maintained, resulting in outdated data being fed to the index price calculations of the liquidity.

The SmartVault uses the PriceCalculator contract 5 times, making it highly dependent on a correct implementation of the contract.

It directly calculated the collateral of the vault, if the price is not correctly validated this can result in an undercollateralized vault and results in liquidation.

Tools Used

Manual Review

Recommended mitigation steps

Everytime the PriceCalculator fetch data from the Chainlink Oracle, it is recommended to validate if the answer/price is correctly, and to check for stale prices. This can be achieved by doing the following:

2. Scaling token decimals

The getTokenScaleDiff assumes all ERC20 tokens have the decimals() getter, but that's not true.
For example the IOTA token does not have this method implemented. This results in a reverted transaction if this token is going to be supported. But just as IOTA there are more tokens not implementing the decimals method. So assuming the method is always present can resulting in DOS fetching the token price.

Another issue that can arise is what if a token has more than 1e18 decimals? This results in a negative value in the getTokenScaleDiff, also resulting in reverting the transaction.

Vulnerability details

Following the Chainlink documentation they give the following example

If you require a denomination other than what is provided, you can use two data feeds to derive the pair that you need. For example, if you needed a BTC / EUR price, you could take the BTC / USD feed and the EUR / USD feed and derive BTC / EUR using division.

$ \frac{BTC/USD}{EUR/USD} = BTC/EUR $

This is exactly what happens in tokenToEur.

Chainlink uses 8 decimals unless it's an ETH pair.

Impact

Not scaling to the right amount of decimals can result in wrong accounting in collateral or token prices. Because Vaults can get liquidated it is important these prices and decimals are correctly used.

Tools Used

Manual Review

Recommended mitigation steps

It is recommended to use the Aggregator decimals method and scale correctly.

Add the following two methods inside the PriceCalculation smart contracts