search

sherlock-audit / 2023-04-unitasprotocol-judging

4 stars 3 forks source link

chainNue - Naive Price oracle design with a simple `putPrice` doesn't have any mechanism to protect from any abnormalities #153

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

chainNue

medium

Naive Price oracle design with a simple putPrice doesn't have any mechanism to protect from any abnormalities

Summary

Naive Price oracle design with a simple putPrice which doesn't have a threshold mechanism to protect from any abnormalities

Vulnerability Detail

putPrice will impact the protocol directly, updating it by Feeder role with EOA can break the whole system in case EOA get compromised. There are several past hack happen because of EOA being compromised, and if an EOA have a high impact to protocol it can have a disastorous effect if it's exploited.

Current XOracle implementation does not have any protection / failsafe mechanism in case Feeder feed a wrong price, due to any issues. The current price oracle will just accept the price as it is, more over the Feeder is an EOA.

This price oracle is a highly critical aspect in every DeFi protocol. Failed to design a robust price oracle will result in inaccurate or manipulated price data, leading to incorrect valuations, faulty automated actions, and potential financial losses for users. It can compromise the security and integrity of the entire protocol, creating vulnerabilities for various attacks such as flash loan exploits, front-running, or manipulation of decentralized exchanges. Therefore, it is crucial to carefully consider the design and implementation of a robust price oracle to ensure the accuracy and reliability of price data within the DeFi ecosystem.

Impact

Open to oracle failure and compromised protocol

Code Snippet

File: XOracle.sol
26:     function putPrice(address asset, uint64 timestamp, uint256 price) public onlyRole(FEEDER_ROLE) {
27:         uint64 prev_timestamp = prices[asset].timestamp;
28:         uint256 prev_price = prices[asset].price;
29:         require(prev_timestamp < timestamp, "Outdated timestamp");
30:         prices[asset] = IOracle.Price(asset, timestamp, prev_timestamp, price, prev_price);
31:         emit newPrice(asset, timestamp, price);
32:     }
33:
34:     function updatePrices(IOracle.NewPrice[] calldata _array) external onlyRole(FEEDER_ROLE) {
35:         uint256 arrLength = _array.length;
36:         for(uint256 i=0; i<arrLength; ){
37:             address asset = _array[i].asset;
38:             uint64 timestamp = _array[i].timestamp;
39:             uint256 price = _array[i].price;
40:             putPrice(asset, timestamp, price);
41:             unchecked {
42:                 i++;
43:             }
44:         }
45:     }

https://github.com/sherlock-audit/2023-04-unitasprotocol/blob/main/Unitas-Protocol/src/XOracle.sol#L26-L45

Tool used

Manual Review

Recommendation

Need to have a protection to minimize potential effect, for example implement range threshold update between timestamp and price to protect a sudden change of token price. Implementing data validation, using multiple oracles, and incorporating off-chain data sources can also enhance the security and reliability of the price oracle system.

sherlock-admin commented 1 year ago

Escalate for 10 USDC

Please judges to review and read my submission again, (my submission title may not clearly describe well)

My issue seems like it is a broader term of how oracle contract doesn't simply have protection from xoracle failure (resulting abnormalities), and it also mention about lack of implementation of range threshold of timestamp and price, which kind of following this existing valid issue:

If we rethink again, the open valid issue above is about when the oracle is updated which resulting stale/freshness price issue, and most of the duplicates of that issue is mentioning about getting fresh data. The updating is a work of "write"-ing data, (which the accepted issue and my issue is about), while the duplicates mostly about "read" or fetching data.

The snippet code for the open accepted issue (#150) is pointing to updatePrices() function (same with mine), meanwhile all of the duplicates pointing to the getLatestPrice() "read" / "get" function.

Moreover the recommendation of open issue (#150)

Add a clear threshold when the prices should be updated (ex. each 2 minutes), potentially not using the gas fees as an indicator to when updating the oracle.

and mine

Need to have a protection to minimize potential effect, for example implement range threshold update between timestamp and price..

which both mentioning the threshold update.

In short, my submission is mentioning the lack of protection mechanism, like threshold of timestamp and price when writing the price, which is align to the accepted issue. My submission doesn't specific to common Chainlink's freshness read issue, but a common write oracle security pattern.

In addition, I might think this issue can be separated, the "write" and "read" oracle issue. Most of the time, we only "consume" / "read" price from oracle (chainlink), thus the stale and freshness does exist in previous contests. But in Unitas case, there is also a "write" as if it's an oracle. This "write" price should be a separated issue than the common "read" price.

I'm not sure why my submission is excluded, while the duplicates were accepted. If the confusion is because I mentioned about EOA, it's just additional open potential condition looking at existing past hack, having an EOA with a direct powerful capability is quite dangerous. (or maybe it's because I didn't put the stale and freshness keyword which is a pretty common Chainlink issue)

You've deleted an escalation for this issue.
ctf-sec commented 1 year ago

the report does not mention explicitly about the lack of freshness check can let stale price to be used like https://github.com/sherlock-audit/2023-04-unitasprotocol-judging/issues/150

putPrice will impact the protocol directly, updating it by Feeder role with EOA can break the whole system in case EOA get compromised. There are several past hack happen because of EOA being compromised, and if an EOA have a high impact to protocol it can have a disastorous effect if it's exploited.

centraliziation Out of scope