OETH Oracle #1815

naddison36 · 2023-09-12T12:38:45Z

Contract Changes

Added floorPrice to the Vault to get the value (USD or ETH) of the collateral assets received from redeeming 1 Origin Token (OUSD or OETH) from the Vault.
Added Chainlink style OETHOracle which is updated by OETHOracleUpdater that aggregates on-chain OETH/ETH prices.
Added price_oracle to Curve interface which gets the EMA Oracle price

Tests

Unit tests

yarn test ./test/oracle/oracle-oeth.js

Fork tests

yarn test:fork ./test/oracle/oracle.fork-test.js

Review

If you made a contract change, make sure to complete the checklist below before merging it in master.

Refer to our documentation for more details about contract security best practices.

Contract change checklist:

Code reviewed by 2 reviewers.
Copy & paste code review security checklist below this checklist.
Unit tests pass
Slither tests pass with no warning
Echidna tests pass if PR includes changes to OUSD contract (not automated, run manually on local)

github-actions · 2023-09-12T12:39:24Z

	Warnings
⚠️	👀 This PR needs at least 2 reviewers

Generated by 🚫 dangerJS against 2b83ec6

notion-workspace · 2023-09-13T11:17:17Z

Contract development of OETH Oracle

Added more OETH Oracle fork tests

notion-workspace · 2023-11-08T08:33:57Z

Prepare OETH Oracle for audit

…elated Added governable behaviour tests to OETH Oracle unit tests

DanielVF · 2023-11-08T16:46:39Z

Second pass review looks good.

DanielVF · 2023-11-09T19:02:59Z

Review part 1: Requirements

We want to provide an oracle for OETH for use on lending platforms, both classic lending platforms and CDP style stablecoins, to allow them to determine the value of OETH as collateral.

Oracles for lending platforms and stablecoin CDP collateral have some core considerations:

There is no safe direction for the oracle to fail to.

If an oracle reports a price that is too low, then the borrower' collateral will report as insuffienct, and will be liquidated. This make angrey borrowers. If the reported price is extra low, then these liquidations could operate agaist the protocol as well, making the protocol itself insolvant. If prices are too high, then attackers will deposit OETH, and borrow out more than its value in collateral, making the protocol insolvant.

Oracles only have to be near the right answer.

Because lending platforms / CDP's usually have collateral ratios on ETH of 110%+, 5-8% off in either direction will not result in protocol level insolvancy. This is a pretty big amount of allowed error, and is a good thing. However, even small errors in the direction of low will result in people who were on the edge of liquidation getting liquidated. They would be mad, but at least it's not everybody.

Transient errors are very bad.

A single extreme bad value can cause a lot of reckage. Inherint to large lending platforms are a lot of liquidation bots in a constant race with each other. If bad values happen, the consequences can be as fast as the same block that the oracle update happened in.

Proposed architecture

We will have a chainlink compatibile oracle contract, feed by an on chain updater contract using on chain data, callable by anyone. A chainlink keeper will backstop the calls.

This seems like a good architecure. Great compatibility, cheap reads. No hard requirement that oracle be updated by the origin team, and a good backstop as long as someone keeps feeding the keeper money.

Proposed pricing strategy

Our current plan for getting new prices to use in updates is:

Use the curve price oracle if it is above the redeem fee.
If below the redeem fee, take the higher of the curve price and the redeem price.
Cap the result at one ETH.

How does this work in practice:

Protocol health	Curve price	Behavior
Healthy	Too high	🟢 Correctly capped at 1ETH
Healthy	Too low	🟢 Will do approx 0.995ETH, which should be good.
Small Depegged Backing	Too high	🟠 Will use curve price. Okay for small depegs.
Small Depegged Backing	Too low	🟢 Will use vault floor. Accurate price.
Missing vault collateral	Too high	🔴 Will be too high if curve is manipulated up.
Missing vault collateral	Too low	🔴 Will assume that the vault has assets to redeem at the target price.
Vault reverts floorPrice()	Pegged	🟠 Oracle will use curve price. If we would be reverting, things prob bad though
Vault reverts floorPrice()	Below threshold	🔴 Oracle will fail to update.
Vault reverts redeems	Above threshold	🟠 Oracle will use curve price. If we would be reverting redeems, things prob bad though
Vault reverts redeems	Below threshold	🔴 We could be reverting becuase the protcool has lost funds or the price oracles are weird.

Assuming our vault, strategies, and backing coins are healthy, this pricing strategy should work great for the intended use case. The cap prevents curve pool manipulation up, and any manipulation down is quickly limited by the vault's floor price. The 0.5% margin for error here is well within what lending platforms would tolerate with no problems.

However, the pricing strategy could be broken if something were wrong.

If something went wrong, manipulating the curve price up would would bypass all vault level and collateral price checks. Also in case of either collateral level problems or OETH vault problems, the pool size is likely to shrink, making manipulation less costly. How easily and at what price could curve prices be manipulated back to peg?
If a backing collateral fell below to 0.70 ETH, then our vault oracles would revert. This both blocks redeems (good), but also reverts our floorPrice() method, causing our oracle to be unable to update at all, and effectively freezing at the last value for the duration of the problem.
floorPrice() always assumes the vault can redeem at the current quoted mix of coins. If the vault or an underlying backing strategy lost big funds, then floorPrice() would continue to report at current asset prices, while redeems would be unavailable (since we revert redeems if more than 3%? undercapitalized). For example, in the case that all balancer pools were hacked, and we were down 35% assets, this oracle would still report the price as near peg, even with the curve pool trading at low, accurate levels.

DanielVF · 2023-11-09T20:50:16Z

DanielVF · 2023-11-09T20:54:19Z

contracts/contracts/oracle/OETHOracleUpdater.sol

+        marketPrice = _getMarketPrice();
+
+        // If market price is above the vault price with the withdraw fee
+        if (marketPrice > MAX_VAULT_PRICE) {


>= would be more efficient here, assuming MAX_VAULT_PRICE is truly the max the vault can report. If the vault actually did report MAX_VAULT_PRICE and the marketPrice was also MAX_VAULT_PRICE, then we would get the same answer checking the vault as not checking the vault.

good point. I'll update

DanielVF · 2023-11-09T20:57:42Z

contracts/contracts/oracle/OETHOracleUpdater.sol

+
+import { IOracleReceiver } from "./IOracleReceiver.sol";
+import { IVault } from "../interfaces/IVault.sol";
+import { AggregatorV3Interface } from "../interfaces/chainlink/AggregatorV3Interface.sol";


I don't think AggregatorV3Interface is used?

AggregatorV3Interface is not used. I'll remove it.

Apparently Slither can detect unused imports. I'll see why it's not detecting ours.

Yeah, I was wondering how this got through the automated tools. I should have chased this down more.

contracts/contracts/oracle/OETHOracleUpdater.sol

DanielVF · 2023-11-09T21:31:46Z

DanielVF · 2023-11-13T20:27:19Z

@naddison36, got any thoughts on how we should handle bad vault situations?

Only thing I've thought of is that we could factor in the difference between the vault total supply and the OUSD supply, if it's less than one. Would add a ton of gas to the oracle update, but in theory we only need to actually call the update if there's been a big price move, and we only the pay the cost if we are off peg.

sparrowDom · 2023-11-13T23:32:38Z

There is some good discussion here. Regarding the bad Vault State:

in case the backing collateral's falls below 0.70 ETH/USDT it would make sense to me to still allow our floorPrice function to update and not revert (the behaviour for redeems stays the same). This would at least require change in the OracleRouter, to return the price even in cases where there is more than 30% deviation.
Thinking what to do in the other case where underlying strategies are being hacked and not able to report the correct value in checkBalance. If we can't really rely on the Vault to report the correct value and also not on the Curve pool (easy to manipulate) what is really a trusted source in that case? Can we assume Curve pool being manipulated if latest price and e.g. price X minutes ago diverge too much? We could also take an average of the last X minute price of the Curve pool and consider that as the "closest to truth" with some resistance being manipulated by tilting the pool.

sparrowDom · 2023-11-15T22:59:00Z

contracts/contracts/oracle/BaseOracle.sol

+    ****************************************/
+
+    /// @notice The number of decimals in the Oracle price.
+    function decimals()


This could also be expressed as:

uint8 public constant override decimals = 18;

sparrowDom · 2023-11-15T23:01:02Z

contracts/test/oracle/oracle.fork-test.js

+      expect(await oethOracleUpdater.vault()).to.eq(oethVault.address);
+
+      expect(await oethOracle.decimals()).to.eq(18);
+      expect(await oethOracle.version()).to.eq(1);


would be cool to have a test that maps deployed implementation addresses to versions. And when we attempt to update the Oracle and forget to also bump the version that test would fail.

sparrowDom

@naddison36 do you mind pinging when you implement the changes we have discussed today and I'll finish my full review?

sparrowDom · 2023-11-15T23:05:01Z

contracts/contracts/oracle/BaseOracle.sol

+    /// Can not be run twice in the same block.
+    /// @param _price is the Oracle price with 18 decimals
+    function addPrice(uint128 _price) external override {
+        if (msg.sender != oracleUpdater) revert OnlyOracleUpdater();


👍 on using custom errors

sparrowDom · 2023-11-22T14:28:41Z

contracts/contracts/vault/VaultCore.sol

+            // price is to 18 decimals, so we do not need to scale them up to 18 decimals
+            price +=
+                redeemAssets[i] *
+                IOracle(priceProvider).price(allAssets[i]);


Leaving a note here so it is not forgotten: we query oracle price per asset in this line above and also in _toUnitPrice function withing the _calculateRedeemOutputs

sparrowDom

Eyeballed the PR again and left some extra comments

sparrowDom · 2023-11-28T21:45:43Z

contracts/contracts/oracle/BaseOracle.sol

+    /// @notice Contract or account that can call addRoundData() to update the Oracle prices
+    address public oracleUpdater;
+
+    /// @notice Last round ID where isBadData is false and price is within maximum deviation


I can't find any reference to isBadData does this comment need updating?

sparrowDom · 2023-11-28T21:49:41Z

contracts/test/oracle/oracle.fork-test.js

+        addresses.mainnet.Timelock
+      );
+      expect(await oethOracleUpdater.MAX_VAULT_PRICE()).to.eq(
+        parseUnits("0.995")


to add extra security we should add a fork test that subtracts the VaultCore's redeemFeeBps from 1 and see if it matches "0.995". This way the fork test will fail in case we update the redeemFeeBps and forget to re-deploy the OETH oracle updater with its MAX_VAULT_PRICE constant

sparrowDom · 2023-11-28T21:57:52Z

contracts/contracts/oracle/OETHOracleUpdater.sol

+            answer = marketPrice;
+            // Avoid getting the vault price as this is gas intensive.
+            // its not going to be higher than 0.995 with a 0.5% withdraw fee
+            vaultPrice = MAX_VAULT_PRICE;


mental note: I think we've discussed this to change to:

vaultPrice = vault.floorPrice(); answer = Math.min(vaultPrice + vault.redeemFeeBps().scaleBy(18, 4), marketPrice);

sparrowDom · 2023-11-28T21:59:29Z

contracts/contracts/oracle/OETHOracleUpdater.sol

+
+            if (marketPrice > vaultPrice) {
+                // Return the market price with the Vault price as the floor price
+                answer = marketPrice;


mental note: and this one similarly:

answer = Math.min(vaultPrice + vault.redeemFeeBps().scaleBy(18, 4), marketPrice);

probably the if condition can be reworked

sparrowDom · 2023-11-28T22:03:50Z

contracts/contracts/oracle/OETHOracleUpdater.sol

+                answer = marketPrice;
+            } else {
+                // Return the vault price
+                answer = vaultPrice;


And I think we've said that if market price is lower than the vaultPrice, then we might go with the market price.

Thinking more about this, I am not so sure this is a good idea, since the market price can be manipulated downwards even though there is a smaller financial incentive to do it. The way it is in the codebase right now seems safer to me.

probably something we should discuss more about.

DanielVF · 2024-05-08T17:43:25Z

After some analysis, we decided to not use this oracle approach. We did end up discovering a bug in Curve's oracle implementation though.

naddison36 added 3 commits September 12, 2023 20:04

Added AggregatorV3Interface diagram

2ca524f

Updated OETH Oracle contracts diagram

876fd06

Added price Oracle to vault

c7dc5b4

rafaelugolini temporarily deployed to preview-oeth-nicka-oeth-73ahqp September 12, 2023 12:39 Inactive

rafaelugolini temporarily deployed to preview-ousd-nicka-oeth-naw3a5 September 12, 2023 12:39 Inactive

naddison36 changed the title ~~OETH Oracle~~ WIP: OETH Oracle Sep 12, 2023

Added reference implementation of OETH Oracle

4c6361c

rafaelugolini temporarily deployed to preview-ousd-nicka-oeth-naw3a5 September 13, 2023 07:07 Inactive

rafaelugolini temporarily deployed to preview-oeth-nicka-oeth-73ahqp September 13, 2023 07:07 Inactive

Refactor OETH Oracle

08824ef

rafaelugolini temporarily deployed to preview-ousd-nicka-oeth-naw3a5 September 13, 2023 11:31 Inactive

rafaelugolini temporarily deployed to preview-oeth-nicka-oeth-73ahqp September 13, 2023 11:31 Inactive

naddison36 added 2 commits September 13, 2023 22:10

Fixed warnings of price override

3be9b7a

Fix unit tests

0d55c81

rafaelugolini temporarily deployed to preview-ousd-nicka-oeth-naw3a5 September 13, 2023 12:29 Inactive

rafaelugolini temporarily deployed to preview-oeth-nicka-oeth-73ahqp September 13, 2023 12:29 Inactive

Fix fork tests

90e8e8c

rafaelugolini temporarily deployed to preview-ousd-nicka-oeth-naw3a5 September 13, 2023 12:52 Inactive

rafaelugolini temporarily deployed to preview-oeth-nicka-oeth-73ahqp September 13, 2023 12:52 Inactive

naddison36 added 2 commits September 14, 2023 16:37

Implements two floorPrice functions to compare

db6fa51

Remove test floorPrice2

dbd9525

rafaelugolini temporarily deployed to preview-oeth-nicka-oeth-73ahqp September 14, 2023 06:59 Inactive

rafaelugolini temporarily deployed to preview-ousd-nicka-oeth-naw3a5 September 14, 2023 06:59 Inactive

Added price cap on OETH Oracle

714e393

rafaelugolini temporarily deployed to preview-oeth-nicka-oeth-73ahqp September 14, 2023 13:23 Inactive

rafaelugolini temporarily deployed to preview-ousd-nicka-oeth-naw3a5 September 14, 2023 13:23 Inactive

Merge remote-tracking branch 'origin/master' into nicka/oeth-oracle

06a6691

rafaelugolini temporarily deployed to preview-ousd-nicka-oeth-naw3a5 September 14, 2023 22:30 Inactive

naddison36 requested a review from shahthepro as a code owner November 7, 2023 23:59

naddison36 added 7 commits November 8, 2023 11:54

fixed OETH Oracle fork tests

7e21054

Merge remote-tracking branch 'origin/master' into nicka/oeth-oracle

b7e118a

fix unit tests after merge

f52cd40

regenerated OETH Oracle contract diagrams

d8e34a1

Updated Oracle docs

a393ffd

Fix Slither error

2bd9e0a

Fixed OETHOracle deploy script

2789e13

Added more OETH Oracle fork tests

naddison36 added 2 commits November 8, 2023 23:05

Added OETH Oracle unit tests

f31dcca

Removed transferToken from governable behaviour as that is strategy r…

85bf20e

…elated Added governable behaviour tests to OETH Oracle unit tests

DanielVF reviewed Nov 9, 2023

View reviewed changes

contracts/contracts/oracle/OETHOracleUpdater.sol Outdated Show resolved Hide resolved

naddison36 added 2 commits November 10, 2023 08:23

Internal review updates to the OETH Oracle

b6af118

Merge remote-tracking branch 'origin/master' into nicka/oeth-oracle

467a0d2

sparrowDom reviewed Nov 15, 2023

View reviewed changes

sparrowDom reviewed Nov 22, 2023

View reviewed changes

Merge remote-tracking branch 'origin/master' into nicka/oeth-oracle

2b83ec6

sparrowDom reviewed Nov 28, 2023

View reviewed changes

DanielVF closed this May 8, 2024

OETH Oracle #1815

OETH Oracle #1815

Uh oh!

Conversation

naddison36 commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contract Changes

Tests

Review

Uh oh!

github-actions bot commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notion-workspace bot commented Sep 13, 2023

Uh oh!

notion-workspace bot commented Nov 8, 2023

Uh oh!

DanielVF commented Nov 8, 2023

Uh oh!

DanielVF commented Nov 9, 2023

Review part 1: Requirements

Proposed architecture

Proposed pricing strategy

Uh oh!

DanielVF commented Nov 9, 2023

Review Part 2: Easy Checks

Authentication

Ethereum

Cryptographic code

Gas problems

Black magic

Overflow

Proxy

Events

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!

Uh oh!

DanielVF commented Nov 9, 2023

Review Part 3: Medium Checks

Rounding

Dependencies

External calls

Deploy

Uh oh!

DanielVF commented Nov 13, 2023

Uh oh!

sparrowDom commented Nov 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sparrowDom 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!

sparrowDom 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

naddison36 commented Sep 12, 2023 •

edited

Loading

github-actions bot commented Sep 12, 2023 •

edited

Loading