search

hats-finance / Origami-0x998f1b716a5022be026ca6b919c0ddf45ca31abd

GNU Affero General Public License v3.0
2 stars 0 forks source link

`OrigamiWstEthToEthOracle` utilizes Chainlink's stETH/ETH feed which has 24 hour long Heartbeat #17

Open hats-bug-reporter[bot] opened 7 months ago

hats-bug-reporter[bot] commented 7 months ago

Github username: @Tri-pathi Twitter username: 0xTripathi Submission hash (on-chain): 0x4885db2009e4165168d8385f0d5d93202d16a2c943bb35a62b93a9d499f1999a Severity: high

Description: Description

OrigamiWstEthToEthOracle utilizes Chainlink's stETH/ETH feed which has 24 hour long HeartBeat and 0.5% deviation, which might lead to wrong price calculation and loss of funds for protocol and user

Attack Scenario\ Describe how the vulnerability can be exploited.

Attachments

  1. Proof of Concept (PoC) File

     * @notice The (rebasing) Lido staked ETH contract (stETH)
     */
    IStETH public immutable stEth;

    /**
     * @notice The stETH/ETH oracle
     */
    IOrigamiOracle public immutable stEthToEthOracle;

    constructor (
        string memory _description,
        address _wstEthAddress,
        uint8 _wstEthDecimals,
        address _ethAddress,
        uint8 _ethDecimals,
        address _stEth,
        address _stEthToEthOracle
    ) 
        OrigamiOracleBase(
            _description, 
            _wstEthAddress, 
            _wstEthDecimals, 
            _ethAddress, 
            _ethDecimals
        )
    {
        stEth = IStETH(_stEth);
        stEthToEthOracle = IOrigamiOracle(_stEthToEthOracle);
    }

https://github.com/hats-finance/Origami-0x998f1b716a5022be026ca6b919c0ddf45ca31abd/blob/main/apps/protocol/contracts/common/oracle/OrigamiWstEthToEthOracle.sol#L26

Here as we can see stETH/ETH pricefeed has been used

According to the chainlink docs

https://data.chain.link/feeds/ethereum/mainnet/steth-eth

stETH/ETH pricefeed has 0.5% deviation threshold and 24 hour long heartbeat

For Info, Heartbeat and Deviation are variables on which the update of the prices for price feeds depends. That mean the current price feed price will be updated for the token only if one of them is met.

i.e The price feed will be update only when the market price for the stETH fluctuates by 0.5% or time equal to 86400s(1 day) has passed since last price update. This could be a very problematic thing

eg Current price of stETH is 9992888991 and suppose after 1 or 2 hour price of stETH drops by 0.49%, chainlink will be giving same price whole day and by that time protocol will be using same outdated price

  1. Revised Code File (Optional)

It is recommended to use price feeds with less heartbeat and deviation threshold . Although the deviation threshold can be handled. Deviation is already handled by reverting out of range prices

Here using stETH/USD+ ETH/USD pricefeed will be optimum which has heartbeat of 1 hour and deviation 1%

PS: there had been many instances for such sudden drops in stETH.

frontier159 commented 7 months ago

Thanks for raising. We don't believe this is a valid issue - using the [stETH/USD] / [ETH/USD] could result in a worse deviation than just using [stETH/ETH]

So while the max heartbeat of using the [stETH/USD] / [ETH/USD] would provide a potentially faster heartbeat, the multiplicative effects of the deviation threshold is in fact far worse and can lead to a much larger deviation from peg.

Tri-pathi commented 7 months ago

@frontier159

Based on the historic data, heartbeat had been more reliable/accurate source for stETH the multiplicative effects of the deviation threshold is in fact far worse and can lead to a much larger deviation from peg- HOW??

You can always close the range for deviation but you can't manage the heartbeat.

Adding some reference for you since it is Valid submission and accepted in all auditing platform.

https://github.com/sherlock-audit/2023-03-olympus-judging/issues/2

https://github.com/code-423n4/2023-05-asymmetry-mitigation-findings/issues/60

https://github.com/code-423n4/2023-11-kelp-findings/issues/865

https://github.com/code-423n4/2023-11-kelp-findings/issues/191

These are some reference for you since it is Valid submission and accepted in all auditing platform.

1) 24 hr long heartbeat and get affected by daily price movement upto 0.5%

2) 1 hr long heartbeat [you are worried about deviation threshold which is not a problem since 1% is very common fluctuation in stETH price] and get update price every hour

Neither protocol and nor users will be impacted by mechanism 2 while in other case protocol and user both could be largely affected by Mechanism 1

You can always revert if deviation is more than 0.5% if you ae worried about this using [stETH/ETH + stETH/USD]

Even at the time of my this comment

stETH from stETH/ETH ~ 3214.6 USD stETH from stETH/USD ~ 3222.93 USD

and max price for today was 3259

So just tell me how this is not valid issue ?

frontier159 commented 7 months ago

If the net 'deviation threshold before a new price' are equivalent between the two methods, then yes, using the method with a smaller heartbeat would be favorable. However using the stETH/USD / ETH/USD creates a larger potential price deviation compared to stETH/ETH before the price is updated.

To illustrate the multiplicative impact of the deviation when you use a cross-rate, I've created a sample spreadsheet which reports the minimum or maximum price for the deviation threshold of the respective feed. The feed prices were real observed values at the time I checked. https://docs.google.com/spreadsheets/d/1QwElevhtgCkOQQc1pRxw-9wJvKyxFse88BJ2iM2Puhs/edit#gid=0

If the stETH/USD is at prevPrice-1% (cell D4), and ETH/USD is at prevPrice+0.5% (cell E5), then when taking the cross rate, the actual implied deviation is quite a lot higher than the max stETH/ETH rate (cell D2). In this scenario the stETH/ETH oracle would have updated far sooner (even though it's a daily heartbeat) than the implied stETH/USD / ETH/USD cross rate.

The underlying issue is not a staleness problem, it's the maximum deviation threshold we're willing to tolerate before the price is updated. We favor a smaller 'deviation threshold' before the price is forced updated, over a more frequent updates but a larger 'deviation threshold'.

The larger deviation cause other issues for the vault too:

frontier159 commented 7 months ago

One thing to note in one of the audits you referenced that I picked up:

https://github.com/sherlock-audit/2023-03-olympus-judging/issues/2

stETH/ETH used to have a 24 hour heartbeat and a 2% deviation threshold. If this were still true, we would indeed have been a worse outcome and we would have used stETH/USD / ETH/USD

However it's since been reduced to 0.5%, so it is now preferable.

Tri-pathi commented 7 months ago

@frontier159 Again I think you are missing the point

1) This issue is about stETH/ETH pricefeed and the impact it can give to the protocol and users. Which seems valid concern from your given comments just you need better solution to mitigate this

2) stETH/USD + ETH/USD was just a recommended solution and i think it's optional to give , recommended solution shouldn't be perfect to validate the issue. It's just best practice of SR who want to help you to mitigate the issue.

As i can see you are accepting the issue if you have better ways

If the net 'deviation threshold before a new price' are equivalent between the two methods, then yes, using the method with a smaller heartbeat would be favorable

Let's have more methods which will give better mechanims but again if you just want to invalidate the issue and ok with such attacks nothing one can do.

1) (stETH/USD+ETH/USD) + Uniswap TWAP / (Another off-chain oracle: For example, Tellor.)

cross chain oracle give fresh price every hour and in case of 1% deviation while TWAP/Tellor will work as fallback oracle to

support in case you are worried i.e [1% deviation which is your concern can be handled easily with TWAP] A lot of High TVL defi protocol do this

2) stETH/ETH + (stETH/USD + ETH/USD) 

 Integrate both oracles and fetch the latest updated price from the two of them
 eg. if 15 min before (stETH/ETH) updated price and 20 mins before (stETH/USD + ETH/USD) then use the price from 
 stETH/ETH.
one will solve longer heartbeat problem and other will solve deviation

There are many such possible fix and it's not what I get rewarded for in the contest.

https://github.com/sherlock-audit/2023-03-olympus-judging/issues/2

Even if price deviation is 0.5% it will be accepted in all the contest platform and sponsor will positively mitigate this issue since it is affecting protocol and users.

A 0.5% deviation for a day could badly affect the protocol(As <0.5% is allowable in a day) eg. Current stETH is ~ 3200 so 0.5% will be 16USD every one stETH trade will imbalance the funds by +- 16USD

PS: Seeing the exposure of LST, soon deviation of stETH/USD feed will be 0.5%

frontier159 commented 7 months ago

@Tri-pathi After discussing internally, we are confident that this is not an issue for our lovToken vaults.

The lovDSR and lovStETH have a dynamic fee mechanism, where the minimum exit fee is >= 0.5%, ie at least the Oracle deviation threshold (0.5% for stETH/ETH). This minimum fee is very intentional. It is to protect existing users of the vault from having value extracted from things like the Oracle having a lag compared to an off-chain price (aggregated CEX/DEX data).

The maximum heartbeat time is irrelevant (because the potential arbitrage could happen at 5 minutes or 5 hours). The key factor here is the deviation threshold that we're willing to accept and to make sure there is economic prevention for that threshold.

All oracles have some lag or risk of deviation from a 'true' price irrespective of adding more complex fallback mechanisms. The nature of the Oracle imprecision cannot be fixed simply by taking more measurements. We're confident the ability to set the appropriate fee structure that we have implemented does mitigate this risk.