ETH-USD price fallback #393

RickGriff · 2024-08-29T23:49:49Z

Addresses the issue of a stale price during shutdown after oracle failure. For WSTETH and RETH branches, we now do our best to use a live price based on ETH-USD x canonical_rate if the primary price calculation has failed.
https://github.com/liquity/bold?tab=readme-ov-file#4---oracle-failure-and-urgent-redemptions-with-the-frozen-last-good-price

Fixes #474

bingen

Looks good in general. I left some questions inline.

bingen · 2024-09-05T10:45:42Z

contracts/src/PriceFeeds/WETHPriceFeed.sol

+     function fetchPrice() public returns (uint256, bool) {
+        // If branch is live and the primary oracle setup has been working, try to use it 
+        if (priceSource == PriceSource.primary) {
+            return _fetchPrice();}


I guess we can pass forge fmt here (I was confused until I saw the ending } was at the end of the line).

This is not done yet, right? No big deal, we can merge and do forge fmt later.

bingen · 2024-09-05T10:55:03Z

contracts/src/PriceFeeds/WETHPriceFeed.sol

+            return _fetchPrice();}
+
+        // Otherwise if branch is shut down and already using the lastGoodPrice, continue with it
+        if (priceSource == PriceSource.lastGoodPrice) {return (lastGoodPrice, false);}


As far is I can see, this one will never have ETHUSDxCanonical. So I guess we can get rid of the if clause (and adapt the comment above)

We can even have an assert for testing.

Good point.

This is not done yet, right?

bingen · 2024-09-05T11:00:37Z

contracts/src/PriceFeeds/MainnetPriceFeedBase.sol

-    function _disableFeedAndShutDown(address _failedOracleAddr) internal returns (uint256) {
+    function _shutDownAndSwitchToLastGoodPrice(address _failedOracleAddr) internal returns (uint256) {
        // Shut down the branch
        borrowerOperations.shutdownFromOracleFailure(_failedOracleAddr);


It’s not related to this PR, but I wonder if we could emit the ShutDownFromOracleFailure here, so we don’t need to pass _failedOracleAddr as param to BorrowerOperations.

Sure, makes sense

This is not done yet, right?

bingen · 2024-09-05T11:05:41Z

contracts/src/PriceFeeds/CompositePriceFeed.sol

+            }
+
+        // Otherwise if branch is shut down and already using the lastGoodPrice, continue with it
+        if (priceSource == PriceSource.lastGoodPrice) {return (lastGoodPrice, false);}


Maybe we can get rid of the if, as there’s no other possibility, and note it in a comment.
It doesn’t matter too much, but right now it feels like the execution flow could not enter in any of the if clauses and return (0, false) (which is not the case, fortunately).

This is not done yet, right?

bingen · 2024-09-05T11:09:29Z

contracts/src/PriceFeeds/CompositePriceFeed.sol

+    function _fetchPriceETHUSDxCanonical(uint256 _ethUsdPrice) internal returns (uint256) {
+        assert(priceSource == PriceSource.ETHUSDxCanonical);
        // Get the ETH_per_LST canonical rate directly from the LST contract
        // TODO: Should we also shutdown if the call to the canonical rate reverts, or returns 0?


I think so, we should fallback to lastGoodPrice.

This is not done yet, right?

I guess we can remove the TODO?

bingen · 2024-09-05T11:12:11Z

contracts/src/PriceFeeds/CompositePriceFeed.sol

+        lastGoodPrice = lstUsdCanonicalPrice;

-        return (lstUsdPrice, false);
+        return (lstUsdCanonicalPrice);


I guess we don’t parenthesis anymore.

This is not done yet, right?

bingen · 2024-09-05T11:23:25Z

contracts/src/PriceFeeds/CompositePriceFeed.sol

+        assert(priceSource == PriceSource.ETHUSDxCanonical);
        // Get the ETH_per_LST canonical rate directly from the LST contract
        // TODO: Should we also shutdown if the call to the canonical rate reverts, or returns 0?
        uint256 lstEthRate = _getCanonicalRate();


In the case of wstETH this is not LST -> ETH, but LST -> stETH. I guess we are assuming 1 stETH = 1 ETH, but historically there have been deviations.
https://www.coingecko.com/en/coins/lido-staked-ether/eth
Maybe we should at least note it as a comment or in the README.

Yes, it's the best we can do when the STETH-USD source has failed. Sure, I'll make a note in the Readme.

This is not done yet, right?

bingen · 2024-09-05T11:24:49Z

contracts/src/PriceFeeds/CompositePriceFeed.sol

-
-        // Take the minimum of (market, canonical) in order to mitigate against upward market price manipulation
-        uint256 lstUsdPrice = LiquityMath._min(lstUsdMarketPrice, lstUsdCanonicalPrice);
+        uint256 lstUsdCanonicalPrice = _ethUsdPrice * lstEthRate / 1e18;


I wonder if it would still make sense to use the min against lastGoodPrice.
If the branch was shutdown, it may be due to a bug in the LST, so the canonical price may be wrong.
This way we would at least make sure we protect against the worst case of urgent redemptions not being profitable.

Right, yes this makes sense. It could still be that market_price < min(lastGoodPrice, canonical_price) (depends on price movement) - but it increases the chances that we can fully clear the bad debt. Good idea!

This is not done yet, right?

bingen · 2024-09-05T11:31:45Z

contracts/src/test/TestContracts/Deployment.t.sol

        address STETHOracle;
        address RETHOracle;
-        address ETHXOracle;
-        address OSETHOracle;


bingen

Looks good to me!

bingen · 2024-09-19T15:00:43Z

contracts/src/PriceFeeds/CompositePriceFeed.sol

+    function _fetchPriceETHUSDxCanonical(uint256 _ethUsdPrice) internal returns (uint256) {
+        assert(priceSource == PriceSource.ETHUSDxCanonical);
        // Get the ETH_per_LST canonical rate directly from the LST contract
        // TODO: Should we also shutdown if the call to the canonical rate reverts, or returns 0?


I guess we can remove the TODO?

bingen · 2024-09-19T15:16:18Z

contracts/src/PriceFeeds/RETHPriceFeed.sol


-        // Calculate the canonical LST-USD price: USD_per_LST = USD_per_ETH * ETH_per_LST
+        // Calculate the canonical LST-USD price: USD_per_RETH = USD_per_ETH * ETH_per_RETH
        // TODO: Should we also shutdown if the call to the canonical rate reverts, or returns 0?


We are doing this above, right? So we can remove the TODO

bingen · 2024-10-23T15:19:30Z

contracts/src/PriceFeeds/RETHPriceFeed.sol

+            rEthUsdPrice = LiquityMath._max(rEthUsdMarketPrice, rEthUsdCanonicalPrice);
+        } else {
+            // Take the minimum of (market, canonical) in order to mitigate against upward market price manipulation.
+            // Assumes a deviation between market <> canonical of >2% represents a legitemate market price difference.


There’s a typo here, right? (legitimate)

bingen · 2024-10-23T15:21:24Z

contracts/src/PriceFeeds/RETHPriceFeed.sol

+        uint256 max = _rEthUsdCanonicalPrice * (DECIMAL_PRECISION + _2_PERCENT) / 1e18;
+        uint256 min = _rEthUsdCanonicalPrice * (DECIMAL_PRECISION - _2_PERCENT) / 1e18;
+
+        return _rEthUsdMarketPrice >= min && _rEthUsdCanonicalPrice <= max;


Shouldn’t both be _rEthUsdMarketPrice here?

bingen · 2024-10-23T15:22:37Z

contracts/src/PriceFeeds/WETHPriceFeed.sol

+        return fetchPrice();
+    }
+
+    function _fetchPricePrimary(bool _isRedemption) internal override returns (uint256, bool) {


_isRedemption isn’t used here? Doesn’t it throw a warning?

RickGriff changed the title ~~Eth price fallback 2~~ Eth price fallback Aug 29, 2024

RickGriff changed the title ~~Eth price fallback~~ ETH-USD price fallback Aug 29, 2024

RickGriff requested a review from bingen September 3, 2024 09:10

RickGriff marked this pull request as ready for review September 3, 2024 09:10

RickGriff marked this pull request as draft September 3, 2024 09:10

bingen marked this pull request as ready for review September 4, 2024 17:15

bingen approved these changes Sep 5, 2024

View reviewed changes

RickGriff force-pushed the eth_price_fallback_2 branch from 9cdf066 to bbf0e6f Compare September 6, 2024 13:19

bingen assigned RickGriff Sep 19, 2024

bingen approved these changes Sep 19, 2024

View reviewed changes

bingen reviewed Oct 23, 2024

View reviewed changes

RickGriff added 18 commits November 5, 2024 23:18

Add ETH-USD fallback logic and remove OSETH and ETHX contracts

54648b8

Add tests for oracle shutdown logic

63b4db5

Remove console.log

53a3382

Fix test

52756ab

Add tests

b1fb3d1

Add oracle logic fallback price tests

5840a07

Add additional price fallback logic

7058884

Add test for logic when using ETHUSDxCanonical

d19116d

Fix tests

5727c26

Remove TODO comments

5816899

Alter price calc logic for normal redemptions

312bd6a

Prevent two shutdown events when oracle fails in TCR shutdown call

a437ec8

Fix bug in 2pct comparison

d45f664

Fix oracle redemption test

87d848a

Add threshold condition to WSTETH redemption pricing

79cab00

Add gas checks for external calls

6848657

Make composite feed abstract

eb41c9c

Update sol versions and cleanup

bdb6492

RickGriff force-pushed the eth_price_fallback_2 branch from 6a05645 to bdb6492 Compare November 5, 2024 23:50

Remove warnings from tests and mocks

1bb9c46

RickGriff merged commit bf2a5c1 into main Nov 6, 2024

danielattilasimon deleted the eth_price_fallback_2 branch January 19, 2025 04:19

ETH-USD price fallback #393

ETH-USD price fallback #393

Uh oh!

Conversation

RickGriff commented Aug 29, 2024 • edited by bingen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bingen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bingen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RickGriff commented Aug 29, 2024 •

edited by bingen

Loading