Price feeds for WETH deployment + Bulker changes for OZ audit #625
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kevincheng96
commented
Nov 22, 2022
| function scalePrice(int256 price) internal view returns (int256) { | ||
| int256 scaledPrice; | ||
| if (shouldUpscale) { | ||
| scaledPrice = price * rescaleFactor; |
There was a problem hiding this comment.
Are we ever going to upscale prices?
It seems like the only use case for this contract is truncating an 18 decimal price feed to 8 decimals.
Would it be simpler/more efficient to have scalePrice always return price / rescaleFactor?
There was a problem hiding this comment.
Yeah maybe simpler is better. But my goal was to make this flexible in case there was ever a need to scale up. There's not much extra gas costs to it, so it just depends on how much we value the extra flexibility.
Personally, I rather have the flexibility (hardcoding price feed decimals has made the WETH deployment trickier, for example), but not a super strong opinion if we think it's not worth it.
jflatow
reviewed
Nov 22, 2022
jflatow
reviewed
Nov 22, 2022
|
The description of this PR is out of date now, right? This is now a PR to do truncated USD prices, not ETH |
The description should still be accurate since this PR is for ETH-denominated prices, not USD. The |
|
Updated this PR with a new |
💪 Thanks for making this change! |
jflatow
reviewed
Dec 12, 2022
This PR contains changes to the following (mostly stylistic) issues highlighted in OZ's audit: - **L-04 Missing docstrings** - We added a comprehensive set of docstrings to the various constants and functions in the `BaseBulker` and `MainnetBulker` contracts. - **L-06 Possible mismatch between native and wrapped tokens in different chains** - It is true that the `BaseBulker` is only intended to work for EVM chains with a native token and wrapped native token that implements the IWETH interface. We have documented this requirement more clearly. - **L-07 sweepToken can potentially call a malicious token** - We expect the admin of the `Bulker` (the `Timelock`) to never sweep malicious tokens, but we added a cautionary note just in case. That being said, we think that a malicious token is unable to attack the contract and steal user’s funds since the `Bulker` never delegatecalls to any external contracts. - **L-08 Unsafe explicit casting of integers** - We purposely avoided the safe conversion of `wstETHScale` because it is always going to be less than `type(int256).max`. We added a note to explain this. - **N-01 Coding style could be improved** - We updated the `wstETHPriceScale` immutable variable to be an int256. - **N-02 Confusing documentation** - We fixed the documentations mentioned in this issue.
jflatow
added a commit
that referenced
this pull request
Dec 15, 2022
* Update Github Actions to include mainnet-eth * ETH-base bulker scenario * WstETHPriceFeed + tests (#600) * Non-ETH and ETH bulker scenarios (all actions in one txn) * Mainnet WETH Bulker (#611) * Add a supply cap constraint and set initial caps to 0 for cWETHv3 * Update the collateral params based on Gauntlet recommendations (#628) https://hackmd.io/wncIvkFTReWUe2AMMK2ezA?view * Price feeds for WETH deployment + Bulker changes for OZ audit (#625) This PR implements and modifies price feeds to support the upcoming WETH deployment. The favored plan so far is to use ETH-denominated price feeds as opposed to USD price feeds, but stick with using 8 decimals for prices to avoid having to change the `Comet` and `Configurator` implementations. This would require: - A new wrapper price feed (`ScalingPriceFeed.sol`) that scales prices up or down to 8 decimals - A new `ConstantPriceFeed` that always returns 1e8 for the `WETH` base asset, since should always hold a 1:1 value with ETH - Modifications to the `WstETHPriceFeed` to return prices in terms of ETH instead of USD This is an alternative approach to #626, which is a more complex change but could be a better long-term solution. *Note: This PR also now contains the changes from #634 and #635, which address some suggestions made by OZ for their audit of `WstETHPriceFeed` and `Bulker`.*
jflatow
added a commit
that referenced
this pull request
Dec 19, 2022
* Update Github Actions to include mainnet-eth * ETH-base bulker scenario * WstETHPriceFeed + tests (#600) * Non-ETH and ETH bulker scenarios (all actions in one txn) * Mainnet WETH Bulker (#611) * Add a supply cap constraint and set initial caps to 0 for cWETHv3 * Update the collateral params based on Gauntlet recommendations (#628) https://hackmd.io/wncIvkFTReWUe2AMMK2ezA?view * Price feeds for WETH deployment + Bulker changes for OZ audit (#625) This PR implements and modifies price feeds to support the upcoming WETH deployment. The favored plan so far is to use ETH-denominated price feeds as opposed to USD price feeds, but stick with using 8 decimals for prices to avoid having to change the `Comet` and `Configurator` implementations. This would require: - A new wrapper price feed (`ScalingPriceFeed.sol`) that scales prices up or down to 8 decimals - A new `ConstantPriceFeed` that always returns 1e8 for the `WETH` base asset, since should always hold a 1:1 value with ETH - Modifications to the `WstETHPriceFeed` to return prices in terms of ETH instead of USD This is an alternative approach to #626, which is a more complex change but could be a better long-term solution. *Note: This PR also now contains the changes from #634 and #635, which address some suggestions made by OZ for their audit of `WstETHPriceFeed` and `Bulker`.*
jflatow
added a commit
that referenced
this pull request
Dec 23, 2022
* Update Github Actions to include mainnet-eth * ETH-base bulker scenario * WstETHPriceFeed + tests (#600) * Non-ETH and ETH bulker scenarios (all actions in one txn) * Mainnet WETH Bulker (#611) * Add a supply cap constraint and set initial caps to 0 for cWETHv3 * Update the collateral params based on Gauntlet recommendations (#628) https://hackmd.io/wncIvkFTReWUe2AMMK2ezA?view * Price feeds for WETH deployment + Bulker changes for OZ audit (#625) This PR implements and modifies price feeds to support the upcoming WETH deployment. The favored plan so far is to use ETH-denominated price feeds as opposed to USD price feeds, but stick with using 8 decimals for prices to avoid having to change the `Comet` and `Configurator` implementations. This would require: - A new wrapper price feed (`ScalingPriceFeed.sol`) that scales prices up or down to 8 decimals - A new `ConstantPriceFeed` that always returns 1e8 for the `WETH` base asset, since should always hold a 1:1 value with ETH - Modifications to the `WstETHPriceFeed` to return prices in terms of ETH instead of USD This is an alternative approach to #626, which is a more complex change but could be a better long-term solution. *Note: This PR also now contains the changes from #634 and #635, which address some suggestions made by OZ for their audit of `WstETHPriceFeed` and `Bulker`.*
jflatow
added a commit
that referenced
this pull request
Jan 4, 2023
* Update Github Actions to include mainnet-eth * ETH-base bulker scenario * WstETHPriceFeed + tests (#600) * Non-ETH and ETH bulker scenarios (all actions in one txn) * Mainnet WETH Bulker (#611) * Add a supply cap constraint and set initial caps to 0 for cWETHv3 * Update the collateral params based on Gauntlet recommendations (#628) https://hackmd.io/wncIvkFTReWUe2AMMK2ezA?view * Price feeds for WETH deployment + Bulker changes for OZ audit (#625) This PR implements and modifies price feeds to support the upcoming WETH deployment. The favored plan so far is to use ETH-denominated price feeds as opposed to USD price feeds, but stick with using 8 decimals for prices to avoid having to change the `Comet` and `Configurator` implementations. This would require: - A new wrapper price feed (`ScalingPriceFeed.sol`) that scales prices up or down to 8 decimals - A new `ConstantPriceFeed` that always returns 1e8 for the `WETH` base asset, since should always hold a 1:1 value with ETH - Modifications to the `WstETHPriceFeed` to return prices in terms of ETH instead of USD This is an alternative approach to #626, which is a more complex change but could be a better long-term solution. *Note: This PR also now contains the changes from #634 and #635, which address some suggestions made by OZ for their audit of `WstETHPriceFeed` and `Bulker`.*
jflatow
added a commit
that referenced
this pull request
Jan 5, 2023
* Update Github Actions to include mainnet-eth * ETH-base bulker scenario * WstETHPriceFeed + tests (#600) * Non-ETH and ETH bulker scenarios (all actions in one txn) * Mainnet WETH Bulker (#611) * Add a supply cap constraint and set initial caps to 0 for cWETHv3 * Update the collateral params based on Gauntlet recommendations (#628) https://hackmd.io/wncIvkFTReWUe2AMMK2ezA?view * Price feeds for WETH deployment + Bulker changes for OZ audit (#625) This PR implements and modifies price feeds to support the upcoming WETH deployment. The favored plan so far is to use ETH-denominated price feeds as opposed to USD price feeds, but stick with using 8 decimals for prices to avoid having to change the `Comet` and `Configurator` implementations. This would require: - A new wrapper price feed (`ScalingPriceFeed.sol`) that scales prices up or down to 8 decimals - A new `ConstantPriceFeed` that always returns 1e8 for the `WETH` base asset, since should always hold a 1:1 value with ETH - Modifications to the `WstETHPriceFeed` to return prices in terms of ETH instead of USD This is an alternative approach to #626, which is a more complex change but could be a better long-term solution. *Note: This PR also now contains the changes from #634 and #635, which address some suggestions made by OZ for their audit of `WstETHPriceFeed` and `Bulker`.*
jflatow
added a commit
that referenced
this pull request
Jan 6, 2023
* Update Github Actions to include mainnet-eth * ETH-base bulker scenario * WstETHPriceFeed + tests (#600) * Non-ETH and ETH bulker scenarios (all actions in one txn) * Mainnet WETH Bulker (#611) * Add a supply cap constraint and set initial caps to 0 for cWETHv3 * Update the collateral params based on Gauntlet recommendations (#628) https://hackmd.io/wncIvkFTReWUe2AMMK2ezA?view * Price feeds for WETH deployment + Bulker changes for OZ audit (#625) This PR implements and modifies price feeds to support the upcoming WETH deployment. The favored plan so far is to use ETH-denominated price feeds as opposed to USD price feeds, but stick with using 8 decimals for prices to avoid having to change the `Comet` and `Configurator` implementations. This would require: - A new wrapper price feed (`ScalingPriceFeed.sol`) that scales prices up or down to 8 decimals - A new `ConstantPriceFeed` that always returns 1e8 for the `WETH` base asset, since should always hold a 1:1 value with ETH - Modifications to the `WstETHPriceFeed` to return prices in terms of ETH instead of USD This is an alternative approach to #626, which is a more complex change but could be a better long-term solution. *Note: This PR also now contains the changes from #634 and #635, which address some suggestions made by OZ for their audit of `WstETHPriceFeed` and `Bulker`.*
jflatow
added a commit
that referenced
this pull request
Jan 24, 2023
* Mainnet ETH Deployment * Update Github Actions to include mainnet-eth * ETH-base bulker scenario * WstETHPriceFeed + tests (#600) * Non-ETH and ETH bulker scenarios (all actions in one txn) * Mainnet WETH Bulker (#611) * Add a supply cap constraint and set initial caps to 0 for cWETHv3 * Update the collateral params based on Gauntlet recommendations (#628) https://hackmd.io/wncIvkFTReWUe2AMMK2ezA?view * Price feeds for WETH deployment + Bulker changes for OZ audit (#625) This PR implements and modifies price feeds to support the upcoming WETH deployment. The favored plan so far is to use ETH-denominated price feeds as opposed to USD price feeds, but stick with using 8 decimals for prices to avoid having to change the `Comet` and `Configurator` implementations. This would require: - A new wrapper price feed (`ScalingPriceFeed.sol`) that scales prices up or down to 8 decimals - A new `ConstantPriceFeed` that always returns 1e8 for the `WETH` base asset, since should always hold a 1:1 value with ETH - Modifications to the `WstETHPriceFeed` to return prices in terms of ETH instead of USD This is an alternative approach to #626, which is a more complex change but could be a better long-term solution. *Note: This PR also now contains the changes from #634 and #635, which address some suggestions made by OZ for their audit of `WstETHPriceFeed` and `Bulker`.* * Refactor deploy to support second markets (#639) * Refactor deploy to support second markets * Add a fromDep to deployment manager which can be used for sharing dependency contracts from another deployment * Always attempt to initialize storage if needed, and not necessarily as gov * Always deploy an initial implementation contract instead of using the factory marker * Only take gov admin actions if we own the cometAdmin * Expose the primitives necessary for a proposal to configurate later, if/when admin cannot * Add an env flag for migration constraint to skip non-migration scenarios * Defer reading current config in modern constraint (bugfix) * Add recipient to token sourcing blacklist * WETH deployment audit changes (#641) These are changes in response to the WETH deployment [audit](https://gist.github.com/andresbach/a01686b1ef8bdf51d46046f2cfc93307) conducted by OZ: - **L01 - Lack of input validation** - We originally avoided zero address checks because there are a vast amount of accidental addresses that can be set here and checking for a specific one seems unnecessary. However, we have reconsidered that position after seeing the reasons for including a zero address check listed in this [post](https://forum.openzeppelin.com/t/removing-address-0x0-checks-from-openzeppelin-contracts/2222/13). - **L02 - Improper implementation of Chainlink AggregatorV3Interface** - We updated the hard-coded return values for `roundId` and `answeredInRound` to be 1 to comply with the Chainlink specification. - **L04 - Incomplete/confusing documentation** - We updated the documentation based on the provided suggestions. Our full audit responses are [here](https://docs.google.com/document/d/1A8TrUdTtD7DXr1U5HOqSS1Xl81rjBZdlDC7LS4No4Bw). * Spider USDC deployment before calling `fromDep` (#644) We need to spider the `USDC` deployment before calling `fromDep` on it so all the aliases are populated in the cache. Otherwise, `fromDep` may try to import an `undefined` contract. There are many approaches to this and this PR takes the simplest approach of spidering in the deploy script. Another approach could be to handle the spidering directly in `fromDep`. * Convert from scientific notation to bigint in configuration (#643) This PR adds a new `ScientificNotation` string type to `NetworkConfiguration` that is required for certain fields (e.g. `supplyCap`, `baseMinForRewards`). The configuration parser will safely convert these values into bigints without losing precision; with one caveat, scientific notations that use decimals in the coefficient still run the risk of precision loss because those are converted directly to javascript numbers. * Explicit supply cap constraint for failing bulker scen (#648) * Add tracking index bounds unit tests for WETH (#645) * Update params and rate model final numbers for deployment * OnChainLiquidator (#642) Update the reference liquidator to source from more pools and generally be more robust * Fix fromDep to spider the dependency and load the contract from it (#652) * bump targetReserves in LiquidationBotScenarios (#653) * delete CURVE_REGISTRY (#654) * OnChainLiquidator, not Liquidator (#655) * Combined model based on Gauntlet's first and second recommendations (#656) * Wrap and unwrap native token as base asset in Bulker (#659) * Add tests for supply native token; clean up * Change supplyStETH to use stETH amount instead of wstETH amount; add scenarios * Tweak comments * Supplying uint256.max collateral reverts in WETH deposit not Comet * Modified deployment roots from GitHub Actions * Proposal to enable cWETHv3 market (#608) * Proposal to enable cWETHv3 market * Modify proposal to add rewards * Update the supply caps based on Gauntlet recommendations (#629) https://hackmd.io/wncIvkFTReWUe2AMMK2ezA?view * Refactor proposal to make market live (#640) * If there are migrations, don't run the non-migrations This allows a proposal branch to pass CI if the migration is required and is generally the intention for migration branches. However an env var is still supported to override and force running the non-migration, too. * Final changes to supply caps; proposal text; WETH transfer amt (#662) Also copy bulker to usdc roots and fix scen test fixes * Deploy cWETHv3 to Goerli (#646) This PR adds a new `goerli-weth` base and deploy script for the Goerli cWETHv3 deploy. The deploy script is very similar to the mainnet cWETHv3 script. The main differences are: - Deploying price feeds for stETH / ETH and cbETH / ETH - Pulling in extra testnet/cross-chain contracts such as `fauceteer` and `fxRoot` using `fromDep` - Cloning over `cbETH` from mainnet - Minting `cbETH` to `fauceteer` * Modified migration from GitHub Actions Co-authored-by: Kevin Cheng <kevincheng96@hotmail.com> Co-authored-by: scott-silver <scott@compound.finance> Co-authored-by: GitHub Actions Bot <>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements and modifies price feeds to support the upcoming WETH deployment. The favored plan so far is to use ETH-denominated price feeds as opposed to USD price feeds, but stick with using 8 decimals for prices to avoid having to change the
CometandConfiguratorimplementations.This would require:
ScalingPriceFeed.sol) that scales prices up or down to 8 decimalsConstantPriceFeedthat always returns 1e8 for theWETHbase asset, since should always hold a 1:1 value with ETHWstETHPriceFeedto return prices in terms of ETH instead of USDThis is an alternative approach to #626, which is a more complex change but could be a better long-term solution.
Note: This PR also now contains the changes from #634 and #635, which address some suggestions made by OZ for their audit of
WstETHPriceFeedandBulker.