Omniscia Rari Capital Audit
Keep3rPriceOracle Manual Review Findings
Keep3rPriceOracle Manual Review Findings
KPO-01M: Inconsistent Normalization
| Type | Severity | Location |
|---|---|---|
| Logical Fault | Medium | Keep3rPriceOracle.sol:L52 |
Description:
The price normalization applied here is inconsistent with that applied by ChainlinkPriceOracle as well as the normalization that should occur in general.
Recommendation:
We advise this section to be rigorously evaluated as the current normalization system of the contract does not return the price of a single unit. For example, if the token has no decimals and a price of 1 ETH, the result of _price will be 1e18 as Keep3r yields the actual ETH amount (or token utilized in the pair) and the final result will be 1e18 multiplied by 1e18 yielding an 1e36 price in Ether which does not make sense.
Alleviation:
The Rari team has stated the intended normalization to indeed result in a final integer scaled by 1e(36 - underlyingDecimals) as that is what Compound expects. Thus, this exhibit can be considered void.
KPO-02M: Insecure Data Freshness
| Type | Severity | Location |
|---|---|---|
| Logical Fault | Medium | Keep3rPriceOracle.sol:L64 |
Description:
The current function of the Keep3r interface (a.k.a. Uniquote) yields the latest price of an asset using only 2 data points which are easily manipulate-able.
Recommendation:
We advise that more data points are utilized by invoking the consult function instead with a granularity of a desired security level, such as 48 for 24-hour data freshness. Although data freshness won't be real-time, it can be considered secure.
Alleviation:
Based on this exhibit, the Rari team stated that they internally evaluated the options that current and quote provide and decided to instead move forward with Alpha Homora's V2 implementation whereby two TWAPs are stored and converted to wei. However, we believe the normalization conducted on L160 and L161 to be incorrect whereby the baseUnit should be replaced by 1e18 hard-codedly as the underlying token's decimals are assimilated in the getUnderlyingPrice function already and this segment would misbehave for tokens with less than 18 decimals.