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

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

@@ -70,11 +83,11 @@ abstract contract MainnetPriceFeedBase is IPriceFeed, Ownable {
 return (scaledPrice, oracleIsDown);
 }

- 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

+ // Only called if the primary LST oracle has failed, branch has shut down, 
+ // and we've switched to using: ETH-USD * canonical_rate.
+ 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


- 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

+ // Only called if the primary LST oracle has failed, branch has shut down, 
+ // and we've switched to using: ETH-USD * canonical_rate.
+ 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?
 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

@@ -158,20 +156,14 @@ contract TestDeployer is MetadataDeployment {
 address ETHOracle;
 address STETHOracle;
 address RETHOracle;
- address ETHXOracle;
- address OSETHOracle;


bingen

Looks good to me!

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

contracts/src/PriceFeeds/CompositePriceFeed.sol

+ // Only called if the primary LST oracle has failed, branch has shut down, 
+ // and we've switched to using: ETH-USD * canonical_rate.
+ 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

 uint256 lstUsdMarketPrice = ethUsdPrice * rEthEthPrice / 1e18;

- // 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

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 added 6 commits September 6, 2024 14:13

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

81a1a2a

Add tests for oracle shutdown logic

eedb8c6

Remove console.log

12796dc

Fix test

512326d

Add tests

55ccbff

Add oracle logic fallback price tests

bbf0e6f

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

RickGriff added 3 commits September 17, 2024 10:52

Add additional price fallback logic

41c1ba2

Add test for logic when using ETHUSDxCanonical

f83569c

Fix tests

3ad1127

bingen assigned RickGriff Sep 19, 2024

bingen approved these changes Sep 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ETH-USD price fallback #393

ETH-USD price fallback #393

RickGriff commented Aug 29, 2024

bingen left a comment

bingen Sep 5, 2024

bingen Sep 12, 2024

bingen Sep 5, 2024

bingen Sep 5, 2024

RickGriff Sep 9, 2024

bingen Sep 12, 2024

bingen Sep 5, 2024

RickGriff Sep 9, 2024

bingen Sep 12, 2024

bingen Sep 5, 2024

bingen Sep 12, 2024

bingen Sep 5, 2024

bingen Sep 12, 2024

bingen Sep 19, 2024

bingen Sep 5, 2024

bingen Sep 12, 2024

bingen Sep 5, 2024

RickGriff Sep 9, 2024

bingen Sep 12, 2024

bingen Sep 5, 2024

RickGriff Sep 9, 2024

bingen Sep 12, 2024

bingen Sep 5, 2024

bingen left a comment

bingen Sep 19, 2024

bingen Sep 19, 2024

ETH-USD price fallback #393

Are you sure you want to change the base?

ETH-USD price fallback #393

Conversation

RickGriff commented Aug 29, 2024

bingen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bingen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment