Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preventing vault price reset in ERC4626 #3800

Closed
daejunpark opened this issue Nov 3, 2022 · 5 comments
Closed

Preventing vault price reset in ERC4626 #3800

daejunpark opened this issue Nov 3, 2022 · 5 comments
Milestone

Comments

@daejunpark
Copy link
Contributor

馃 Motivation

New security feature recommendation preventing the ERC4626 vault price reset.

馃摑 Details

I'm concerned with the possibility of ERC4626 vaults being emptied during their lifecycle. This is because, once a vault is emptied, the conversion rate of shares (or the share price) is reset to the initial value (which is currently 1:1 by default). The change in the share price could be significant especially when the vault has generated a high yield until the reset (in which case the share price will be drastically decreased).

The sudden change in share price (especially the big price drop) could be a problem and may be exploited in some ecosystem where the vault is used as price oracle for other protocols.

A potential exploit scenario is as follows:

  1. Adversaries create a vault, deposit a good amount of initial assets, and inflate the share price by "donating" more assets or generating a (fake) yield. (Say, the initial deposit is 10 ETH and the fake yield is 40 ETH, thus the inflated price is 5 = 50/10.)
  2. They register their vault for some lending protocols that admit arbitrary ERC4626 vault tokens as collateral and use the vault share's conversion rate as price oracle for their TWAP.
  3. Then, they wait until the lending protocols' TWAP fully reflects the inflated price.
  4. Then, they atomically withdraw all the assets from the vault (which immediately resets the conversion rate to the initial value 1:1), mint a lot of shares with the initial rate 1:1, and use them as collateral to borrow much more from the lending protocols while their TWAP is still high reflecting the inflated price. (Say, they mint 100 shares by depositing only 100 ETH after the vault is emptied, and use the 100 shares as collateral to borrow 250 ETH (assuming 200% collateralization ratio). This is possible because the 100 shares is valued at ~500 ETH at the moment since the TWAP is still (close to) 5 because it doesn't quickly reflect the sudden price drop. Then, the adversaries walk away with making a profit of 150 ETH.)

Recommendation

You may add a requirement that, once a vault is initialized (and funded), the totalSupply must remain to be greater than a certain threshold (say 1 gwei = 10^9). This way, the conversion rate will be kept being track of, even in the case that (almost) all the shares have been redeemed.

This requirement could be made optional if it is too restrictive for some use cases.

Remark

Note that the above requirement implies the minimum initial deposit being greater than the threshold, which can also mitigate the price inflation front-running attack issue #3706.

Also note that step (1) in the above scenario is feasible even if the vault explicitly tracks of totalAssets, since the adversaries can still utilize the yield generating mechanism to increase totalAssets legitimately (in a similar way as described in https://www.rileyholterhus.com/writing/bunni by @rholterhus).

@frangio
Copy link
Contributor

frangio commented Nov 3, 2022

Thanks, this is interesting and we will consider it along with #3706. (We're actively discussing mitigations for #3706 and will be acting on that in the next 2 weeks.)

In the exploit described the adversary is the vault deployer so even if we implement a mitigation for this issue it's still something that ERC4626 integrators should consider in their threat model, as the adversary can simply use a non-OpenZeppelin implementation. An important exception for this is if the vault deployer is a factory (the vulnerable contract might even automatically whitelist all vaults created by the factory), in which case the adversary could be a user of the factory and that is something that we would want to protect our users against.

@frangio frangio changed the title Additional security feature request to prevent the ERC4626 vault price reset Preventing vault price reset in ERC4626 Nov 3, 2022
@frangio
Copy link
Contributor

frangio commented Nov 3, 2022

Would it be an option to preserve the prior share price after the vault is emptied, instead of resetting to 1:1?

@daejunpark
Copy link
Contributor Author

Would it be an option to preserve the prior share price after the vault is emptied, instead of resetting to 1:1?

I think that could be also an option if the additional storage cost is acceptable.

@urica12

This comment was marked as spam.

@frangio frangio added this to the 4.9 milestone Jan 5, 2023
@frangio
Copy link
Contributor

frangio commented Feb 21, 2023

Fixed by #3979.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants