code-423n4 / 2022-09-y2k-finance-findings

3 stars 1 forks source link

Problems with PegOracle #482

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/oracles/PegOracle.sol#L39-L83

Vulnerability details

Impact

There are at least a few problems with the PegOracle. I am grouping them into one submission because some of them are not that significant but the last one I believe deserves a higher severity.

1) Function latestRoundData queries getOracle2_Price but getOracle1_Price is not invoked and the price is fetched directly instead:

    (
        uint80 roundID1,
        int256 price1,
        uint256 startedAt1,
        uint256 timeStamp1,
        uint80 answeredInRound1
    ) = priceFeed1.latestRoundData();

leaving it unvalidated against the stale data and passing this burden up to the caller.

2) Decimals are stored in a signed int type even though it can never be negative:

  int256 decimals10 = int256(10**(18 - priceFeed1.decimals()));

Also, this will revert if the price feed has >18 decimals, it should verify that in the constructor.

3) The main problem with why this has a severity of high is that this contract assumes the default value of decimals is 8, which is not the case: "all pairs have 8 decimals unless it's an ETH pair" (source: https://ethereum.stackexchange.com/a/92513/17387 ). This function will always return 0 when the price feed has 18 decimals. See the proof of concept for more details.

Proof of Concept

I wrote a simple POC that you can try: https://gist.github.com/pauliax/770034a5b744dd2fbba6fa01d9e20eb6

When priceFeed1Decimals is set to 18, then no matter what values are price1 and price2, it will always return 0. Even though this contract fetches the decimals, the code still assumes a default value of 8 which can cause misbehaving.

Recommended Mitigation Steps

Consider fixing the aforementioned issues of the oracle.

HickupHH3 commented 1 year ago

uggghhhh really should have split into 2 issues....

(1) is a dup of #61 (3) is a dup of #195.

Am grouping as a dup of #195. Really unforunate; should have submitted as separate issues.