OETH - Full Changes

[DanielVF]

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.

TODO:

• PriceUnitMint & PriceUnitRedeem need to be updated in the dapp

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)

[sparrowDom]

LGTM I am surprised by how little changes to the Vault are required for it to support a token with an exchange rate. Elegantly solved! Left a couple of comments inline

[DanielVF]

OETH Zapper Code Review

Requirements

We want to allow users with raw ETH to being able to send it to an address and get OETH. We also want to allow users with sFXETH to mint OETH. By having this as a separate contract, we keep the vault simple, and avoid handling ETH in the vault.

Deployment Considerations

Monitoring would be nice, but not necessary. Good to see how much this is used.

Will need to check constructor addresses when deployed.

Internal State

No stored state. Intended to only ephemerally hold balances.

Attack

If this contract failed, the worst case would a transaction's worth of user's funds (not protocol funds).

All publicly callable methods also use a minimum amount which will revert if the user does not get back that much OETH.

Could funds could become stuck in this contract? Every publicly callable method ends by sending all OETH on the contract to the caller. If a someone accidentally sends mintable funds or OETH to this contract, they will be folded into the next deposit and returned to the next depositor. Any other ERC20 would become stuck, which is acceptable.

If OETH had a rounding error, and returned less funds than expected, it could break the deposit. depositSFRXETH has a user controlled minimum. This could be adjusted in the UI. Depositing ETH insists on that zapper receiving exactly 1:1. Given that the current version of the contract is non-rebasing, this should always be true.

Logic

There are no if statements, and all external paths end with funds going to the user.

Tests

🔅 This code has no unit test or fork tests. Don't need unit tests since this hardcodes live contract addresses for simplicity. Fork tests would be good.

Flavor

Could this code be simpler? Not that I see.

🔅✔︎ The repeated slither-disable-next-line unused-return are a bit ugly. I've removed them and replace them with entries in the triage file.

🔅✔︎ No document strings on methods. Added.

Could this code be less vulnerable to other code behaving weirdly? In the end the requires ensure that they user gets what they expect. If something breaks one day in a called token or contract, we can just deploy a new zapper, and there is no state that needs to carry over.

Overflow

No math!

Proxy

No proxy!

Black magic

• Does not contain selfdestruct
• Does not use delegatecall outside of proxying
• Address.isContract should be treated as if could return anything at any time, because that's reality.

Dependencies

Only interfaces are used.

Deploy

• Check that any deployer permissions are removed after deploy. _No deployer permissions.

Authentication

• Never use tx.origin
• Check that every external/public function should actually be external
• Check that every external/public function has the correct authentication

Cryptographic code

No crypto.

Gas problems

No Loops.

External calls

• Contract addresses passed in are validated
• Unsafe external calls are handled
• Reentrancy guards on all state changing functions. No state
• Could fail from stack depth problems (low level calls must require success)
• No slippage attacks (we need to validate expected tokens received)
• Oracles, one of:
  • No oracles
  • Oracles can't be bent
  • If oracle can be bent, it won't hurt us.
• Don't call balanceOf for external contracts to determine what they will do, when they instead use internal accounting?

Ethereum

This contract sends and receives ethereum.

The only receiving is in receive(), which calls deposit. The vault's reentrancy guards should be protect the vault. We do not allow fallback calls with eth to arbitrary addresses.

We send eth, but only to the trusted WETH contract, and we have minimum amount checks afterwords that prevent any funny business.

[DanielVF]

Code Review: Frax ETH Strategy

Requirements

This stakes FRXETH tokens into the 4626 sFRXETH contract. This also works as a generic 4626 strategy to wrap any token into a 4626.

As with any Strategy, the three key concerns are:

• Money goes in
• Money goes out
• Balances are correct, and cannot be manipulated below what we can withdraw.

Deployment Considerations

Should be in our monitoring / event logging.

Internal State

This contract only holds configuration.

shareToken and assetToken are somewhat duplicates of the InitializableAbstractStrategy's built in pToken storage. However, having real names adds a lot of clarity to the code, as well as gas savings. The only thing I could see that could go wrong here would be that the contract could be initialized with multiple assets set, the last would win, and then you would have to upgrade the proxy in order to go back. I don't think this is worth attempting to prevent. If you want to misconfigure a strat when setting it up, there's a lot of ways to do this. Maybe a comments note.

Attack

This strategy could be holding a large portion of our backing funds. It's important that it works.

The most common danger with these strategies is a balance checking problem, with an attacker being able to force an incorrect high balance, rebasing to catch the gains, and then withdrawing.

🔆 Check balance uses convertToAssets to get our maximum withdraw. This gets the correct answer with stfxeth, but it would feel better to use previewRedeem in case redeeming in some other 4626 contract was implemented differently.

4626 contracts can sometimes be vulnerable to rounding attacks on initial deposits. stfxeth has protection against loosing all your money (might only lose half?). However this would be very gas expensive to check check for. Perhaps a comment on only going into pools that have existing funds. stfxeth also uses stored asset amounts, which should be donation proof.

The canonical 4626, and stfxeth, correctly rounds previewRedeem amounts down, as does redeeming. This means that our balance check should match our worst case.

Deposit and withdraw look correct.

Logic

Looks good

Tests

• Each publicly callable method has a test

Flavor

Code is elegant.

Overflow

No math

Proxy

• Make sure proxy implementation contracts don't initialise variable state on variable declaration and do it rather in initialize function.

Black magic

• Does not contain selfdestruct
• Does not use delegatecall outside of proxying
• Address.isContract should be treated as if could return anything at any time, because that's reality.

Dependencies

No change to standard strategy dependancies.

Deploy

• Check that any deployer permissions are removed after deploy

Authentication

• Never use tx.origin
• Check that every external/public function should actually be external
• Check that every external/public function has the correct authentication

Cryptographic code

no crypto

Gas problems

no loops, other than one in our control in initialization.

External calls

• Contract addresses passed in are validated
• Reentrancy guards on all state changing functions
  • Still doesn't protect against external contracts changing the state of the world if they are called.
• Could fail from stack depth problems (low level calls must require success)
• No slippage attacks (we need to validate expected tokens received)
• Oracles, one of:
  • No oracles
  • Oracles can't be bent
  • If oracle can be bent, it won't hurt us.
• Don't call balanceOf for external contracts to determine what they will do, when they instead use internal accounting?

I notice that we have a reentrancy checks on the state changing methods, but not on check checkBalance. It's probably not worth adding a read only reentrancy check on this method, since the vault should have it's own reentrancy checks that should prevent a rebase from happening in the middle of a deposit or a withdraw. Oh hey! The vault core does have these checks, but the vault admin does not!

📛 Vault Admin needs to have reentrancy checks on the strategist moving funds. If an attacker somehow gains execution in the middle of a funds move, this will prevent the attacker from being able to make a rebase. ( Issue created on this #1366 )

Ethereum

• Contract does not send or receive Ethereum.
• Contract has no payable methods.

[DanielVF]

Code Review: Oracle Router

Requirements

The Oracle Router provides depeg resistance to the vault.

If a coin coin loses peg, then the vault will block new mints. If a tiny depeg happens, then less OETH will be minted. If a small increase happens, then the vault will redeem less of that kind of coin. (This happens in stablecoins during deepens, the non-depegging coin gets more expensive)

However, our oracles are currently dog biscuits compared with what we are used to in stablecoin land:

🟪 steth and reth have 2% update margins - they could get very depegged without OETH knowing anything about it. In practice, we get one update every 24 hours. Besides being late to catch depegs, we would be late to catch repegs, and might continue to use an old price for 24 hours.

🟪 We have no oracle on sfrxeth. This means that if it depegs, the only thing we can do is pause the whole vault.

🔆 We don't currently check if oracle prices are out of date. This is probably the right thing to do, since we would probably want to fail to live if chainlink goes down, but it would be good to monitor the last update times and amounts in external monitoring. This applies to OUSD as well.

Given the poor oracles we have, OETH at launch will be much less depeg resistant than OUSD is. The redeem fee should act against depegs that are less than the size of the redeem fee.

Between the redeem price and the area where the oracles update, we should expect to see a shift in the assets held by OETH. This shift is somewhat self limiting, since the more bad assets are put in with minting, the less of good coins the arber gets back on redeeming.

If this shift happened, and the asset later repegged, we would make a lot of money.

This stable point depends on how bad the depeg is and what the redeem fee is. For example, at 0.98, and 0.5%, 25% of assets should still be good coins. Arber mints for 0.98, gets back 75% coins worth 0.98, 25% worth 1.00, and pays a 0.5% fee that cancels out the their projected gains. (A 0.25% fee, like ousd, would leave 12.5% good assets)

Deployment Considerations

🔆 We should monitor, with alerting, the pool balances for the three collateral we use, like we do for LUSD/threepool on OUSD.

Internal State

The only internal state is the cached decimals per oracle. If these are not setup via cacheDecimals then price getting will break until it is. This is easily fixable, with no permissions, so we should be good.

Attack

As discussed in the requirements, this really only comes into play if a coin is out of peg, or reported out of peg.

If we had a false positive high report, it would make redeeming uneconomical.

🔆 Another obvious problem would be an oracle not getting updated. It would be good to record the time since last oracle update in our monitoring.

A MEV bot could do actions to OETH strategically around price updates. This should not affect the protocol though, just get a little more profits than they otherwise would.

Logic

Are there bugs in the logic?

We require that feed != address(0) in production however there's no way to reach that state. However, it's a really cheap sanity check, so let's keep it.

Tests

• Each publicly callable method has a test
• Each logical branch has a test
• Each require() has a test
• Edge conditions are tested

Flavor

🔴 We added a new OETHOracleRouter for the new launch. It is correct for OETH. We still have the old OracleRouter, however changes that were made in in the base code, make it neither compatible with the deployed OUSD code, nor correct for a new deploy of the new shared vault code.
- We should move the price impl from OETHOracleRouter to OracleRouter
- We should get rid of the no longer needed price impl in OracleRouterBase and just have an empty method.

I've created an issue for this. Since the current code works correctly for OETH, we won't fix now.

We should throw isStablecoin into the fire, but that's already covered by the above. That functionally has moved into the vault, and it's really gas inefficient.

Overflow

• Math is good

Proxy

• Make sure proxy implementation contracts don't initialize variable state on variable declaration and do it rather in initialize function.

Black magic

• Does not contain selfdestruct
• Does not use delegatecall outside of proxying
• Address.isContract should be treated as if could return anything at any time, because that's reality.

Dependencies

• Checked

Deploy

No deployer permissions

Oracle has been tested live

Authentication

• Never use tx.origin
• Check that every external/public function should actually be external
• Check that every external/public function has the correct authentication

Cryptographic code

No crypto

Gas problems

No loops

Might be nice to sort the feeds and put core price feeds first. Okay to leave as is though.

External calls

• Contract addresses passed in are validated
• Unsafe external calls
• Could fail from stack depth problems (low level calls must require success)
• No slippage attacks (we need to validate expected tokens received)
• Oracles, one of:
  • No oracles
  • Oracles can't be bent
  • If oracle can be bent, it won't hurt us.
  • I AM ORACLE
• Don't call balanceOf for external contracts to determine what they will do, when they instead use internal accounting?

Ethereum

• Contract does not send or receive Ethereum.
• Contract has no payable methods.

[DanielVF]

Governance Ownership Checks

✅ All looks good

Multisig owned

OETH Proxy
0xbe2ab3d3d8f6a32b96414ebbd865dbd276d3d899

VaultProxy
0xbe2ab3d3d8f6a32b96414ebbd865dbd276d3d899

Vault
0xbe2ab3d3d8f6a32b96414ebbd865dbd276d3d899

VaultAdmin
0xbe2ab3d3d8f6a32b96414ebbd865dbd276d3d899

VaultCore
0xbe2ab3d3d8f6a32b96414ebbd865dbd276d3d899

FraxEthStrategyProxy
0xbe2ab3d3d8f6a32b96414ebbd865dbd276d3d899

Frax Strategy Implementation
0xbe2ab3d3d8f6a32b96414ebbd865dbd276d3d899

Woeth Proxy
0xbe2ab3d3d8f6a32b96414ebbd865dbd276d3d899

Dripper Proxy
0xbe2ab3d3d8f6a32b96414ebbd865dbd276d3d899

Deployer owned:

OETH Impl
0xfd9e6005187f448957a0972a7d0c0a6da2911236

Woeth Impl
0xfd9e6005187f448957a0972a7d0c0a6da2911236

Dripper IMPL
0xfd9e6005187f448957a0972a7d0c0a6da2911236

No Owner:

OETH Oracle Router
No Owner

Zapper
No Owner

OUSD Oracle Router
No Owner

[DanielVF]

Code Review: OETH Vault

Requirements

This change is designed to allow the vault to work with rebasing tokens that are not always worth 1 ETH (or 1 USD). The vault does this by internaly converting these coins into 1e18 units that correspond to an ETH (or USD). This actually simplifies a lot of the code, since before the vault constantly had to do conversions in and out of the token's decimals.

This change is intended to be backwards compatible (outside of the OracleRouter, and price views) with the old code. This means that all existing tests should pass.

Deployment Considerations

🔆 Requires new OracleRouter

Attack

These new changes affect the three most critical parts of the vault - minting, redeeming, and balance calculations for rebasing. A calculation failure in any one of these areas would be GG for the protocol.

An attacker's goal would be to create the impression that their assets are worth too much during mints, worth too much during rebase, or worth too little during a redeem.

There's a discussion of oracles on the OracleRouter review. Even with bad oracles, the total number of stables should only go up -- as long as the exchange rate does not go down.

The fact that an exchange rate could go down is a new twist that we didn't have in OUSD. We also have rebasing tokens that could balance down. To some degree, this "we have less backing funds now" is a core risk we are taking with OETH.

📛 I belive there is a profitable attack that would require several things to line up. Attack goes like this:

Attacker knows that a bigger than redeem fee exchange rate drop on reth is coming. Attacker can force the chainlink oracle to update to reflect the new exchange rate, but the Attacker can buy/sell reth at approximately the same price before and after the exchange rate drop.

In this case, the attacker would wrap the exchange rate and oracle update transactions, acquiring a large amount of RETH before the updates and depositing into OETH. Then after the update, they would redeem. Given the new exchange rate, OETH would treat rETH as less valuable, and send more of it to make up an ETH. In the right circumstances then, the attacker would net more than they put in.

If the chainlink oracle doesn't update at the same time, this attack does not work. If the oracle is at the new low price, the mint gets subtracted from, if the oracle is at the old high price, the redeem gets subtracted from. If the oracle is in between, it affects both sides.

We do have defenses intended to limit the scope of such an attack. If the vault value is too low of a percentage of the ousd total supply, then the mints will revert. However this vault check happened before the redeem happened. Here's why this is bad:

What	Total Supply	Vault Value
Initial	10	10
Attacker moves in	110	110
Bad thing reduces vault value	110	106
Attacker moves 100 worth back out	10	6

The vault value check should happen after the redeem, not before.

I've added this change here:

508921b

Logic

I've simmed out the math on paper and it seems to work. Old OUSD tests pass, and new exchange rates test pass.

Tests

• Each publicly callable method has a test
• Exchange rates tests pass
• Past OUSD tests pass

Flavor

Why do we have all the virtuals in VaultCore? Those should not be needed?

Overflow

• Never use "+" or "-", always use safe math or have contract compile in solidity version > 0.8
• Check that all for loops use uint256

Proxy

• Make sure proxy implementation contracts don't initialize variable state on variable declaration and do it rather in initialize function.

Black magic

• Does not contain selfdestruct
• Does not use delegatecall outside of proxying
• Address.isContract should be treated as if could return anything at any time, because that's reality.

🔆 We don't have our vault core proxy fix merged in. This means that we MUST have multisig ownership of the VaultCore implementation contract. We do have this in the current live contract.

Dependencies

No new dependancies, and no change in dependancies.

Deploy

• Check that any deployer permissions are removed after deploy

Authentication

• Never use tx.origin
• Check that every external/public function should actually be external
• Check that every external/public function has the correct authentication

Cryptographic code

No crypto

Gas problems

• Contracts with for loops must have either:
  • A way to remove items
  • Can be upgraded to get unstuck
  • Size can only controlled by admins
• Contracts with for loops must not allow end users to add unlimited items to a loop that is used by others or admins.

External calls

• Contract addresses passed in are validated
• Reentrancy guards on all state changing functions (True in vault core, issue in vault noted elsewhere.)
• Could fail from stack depth problems (low level calls must require success)
• No slippage attacks (we need to validate expected tokens received)
• Oracles, one of:
  • No oracles
  • Oracles can't be bent
  • If oracle can be bent, it won't hurt us.
• Don't call balanceOf for external contracts to determine what they will do, when they instead use internal accounting?

As in the long discussion in the OracleRouters review, our current oracles are bad. However, we should still keep at least the same assets.

Ethereum

• Contract does not send or receive Ethereum.
• Contract has no payable methods.

[sparrowDom]

LGTM

Lets merge this to master since it is already live on mainnet for some time