Implement or recommend mitigations for ERC4626 inflation attacks

frangio commented 2 years ago

Empty ERC4626 vaults can be manipulated to inflate the price of a share and cause depositors to lose their deposits due to rounding in favor of the vault. In particular, this can be done as a frontrunning attack if the vault is used without specifying a minimum of shares to be received using an ERC4626 router.

We caution users against slippage in the documentation but we could offer concrete mitigations either in code or as recommendations. Morpho Labs mentions they have made some changes on top of our implementation as a mitigation, we should look into that (where is the code?). See also https://github.com/transmissions11/solmate/issues/178.

zt-9 commented 2 years ago

the morpho erc4626 vault contract should be in this repo. https://github.com/morpho-dao/morpho-tokenized-vaults

Amxx commented 2 years ago

As the name suggest, the inflation attack comes from the amount of underlying assets changing without the amount of shares reflecting that. This inflation makes all the share more valuable, which means that someone buying shares will get less than they would have expected.

The bigger the inflation, the bigger the effect of the attack, which is why this is particularly sensitive when then vault is its early stages, with low liquidity.

A few mitigations strategies have been mentioned:

Using an ERC4626 router
Keeping track of the total assets internally
Creating "dead shares"

The first solution is the most simple one, but also possibly the less satisfying. It doesn't actually address the issue as much as it goes around it.

Advantage:

Already available
Easy to reason by
Nothing to change to the vault's code

Drawback:

Uses more gas (router needs to take custody before performing vault operations)
Additional trust assumption (the router is a critical element that will be allowed to spend user token)
User are still subject to slippage (they can limit it, but there is still room for "MEV")

Keeping track of the assets held by the vault internally remove the effect of direct transfers. Tokens transferred directly are not accounted for (unlike tokens that are explicitly transferred during a mint/deposit operation), which completely removes the risk of inflation attack.

This however causes other issues. Transferring tokens is still possible, and if they are not accounted for automatically, they might not be recoverable. To recover them would likely require some form of access control, which might not be desirable when building trust minimizing applications.

In addition to direct transfers, this also apply to re-basing tokens!

Advantage:

Completely removes the inflation, removing all slippage
Easy to reason by
Self contained to the vault (no dependency on third party contract)
Same workflow for EOAs and smart contracts
This solution can provided as an optional module, that devs would add on top of the base implementation.

Drawback:

Not generic (does not work with re-basing tokens)
Risk of token lock (burn) OR needs a trusted admin
Still some gas cost overhead (but less than the previous option)

The last option is inspired by Uniswap V2, which created some "dead LP shares" when the first liquidity is deposited. That basically means that the first liquidity provider is giving away a fraction of its shares for monetary stability.

A similar approach could be implemented in ERC4626 vaults, by minting "dead shares" on the first deposit/mint. For any assets that are then transferred to the vault, a fraction would be associated to these dead shares, and not redeemable by anyone.

In addition to not solving the actual problem (it is still possible to do inflation attacks, the profit is just reduced) this approach disincentivize any early liquidity providing that is not part of an inflation attack. It asks a lot of questions with the amount of "dead shares" to mint that possibly don't have an absolute good answer, and must be answered on a case-by-case basis.

Advantage:

Very limited gas overhead.
This solution can provided as an optional module, that devs would add on top of the base implementation.

Drawback:

Doesn't truly resolve the issue
Disincentivize legitimate early liquidity providers
Locks a fraction of the assets in the vault (that can never be withdrawn)

Any other solution I should study?

Amxx commented 2 years ago

How they fixed it

Morpho-dao

At initialization (eq. construction) deposit some assets into the vault. The corresponding shares are minted to the vault itself, which acts as a dead address.

This is similar to the 3rd option above, but with the loss taken by the project. The more is deposited initially, the more difficult it is to perform an inflation attack ... but funds needs to be available for that (they are lost).

Note:

Implementation is very simple.
It is necessary to precompute the address of the vault, and approve it in advance (before deploying it)

YieldBox

When computing the exchange rate, add a "virtual" amount of shares and assets to the vault. That is equivalent to the behavior of option 3, but without the need to actually burn any tokens to mint the "dead shares".

This fixes the second drawback, but a fraction of any tokens that the vault receives from direct transfer, or re-basing, will be lost.

frangio commented 2 years ago

There is also another option, though it's arguable a "non-solution": leaving totalAssets unimplemented and have users implement it to their needs. I've argued before that most vaults will need a custom totalAssets function anyway (we should evaluate existing concrete ERC4626 vaults to understand if this is true).

Of the three options presented, I favor tracking the asset balance internally.

Amxx commented 2 years ago

Of the three options presented, I favor tracking the asset balance internally.

By default, or as an option?

I would go for the second, because the internal tracking has a cost, and devs that don't need it should not pay for it. You can override the totalAsset function, but you'd still pay for the sstore in deposit/mint/withdraw/redeem

frangio commented 2 years ago

The problem is that the default should be risk-minimized... If we want to make it optional, the default should be abstract.

Amxx commented 2 years ago

The problem is that the default should be risk-minimized

I don't fully disagree with that. IMO the default should be feature-minimal, with security issues clearly identified, and option to address the issues through additional feature. That is what we did with Ownable/Ownable2Step.

frangio commented 2 years ago

Does that mean you would agree with leaving totalAssets as an abstract function in the default? In that case do you see it as necessary to also provide an option with internal balance tracking?

Amxx commented 2 years ago

I'm not sure about the abstract version. It feels like we purposefully remove one line of code just to force users to read the doc ...

daejunpark commented 2 years ago

@Amxx the second option alone (keeping track of the assets) may not be effective to prevent various ways of inflating the price. Especially, adversaries could still donate to the place where the vault assets are deployed and yields are accumulated, e.g., https://www.rileyholterhus.com/writing/bunni

IMO, the price inflation is hard to completely prevent, since it is not easy to distinguish genuine vs fake yield.

So, I think a separate mitigation to make it hard to profit from price inflation would be still needed, e.g., requiring a minimum initial deposit, or extra slippage protection.

frangio commented 2 years ago

Is the minimum initial deposit the standard accepted way to mitigate this? It seems like a really suboptimal approach. (It's literally burning money? I guess you can call it a "security budget". :grimacing:)

We can adopt Morpho's solution which is really clean and can be easily disabled by users who don't want to make an initial deposit for whatever reason (e.g., testing purposes).

Amxx commented 2 years ago

YieldBox's option is also really nice:

It achieve the same result without the need to actually burn/lock any real underlying asset. It also greatly simplify the conversion function fallbacks.

function _convertToShares(uint256 assets, Math.Rounding rounding) internal view virtual returns (uint256 shares) {
    return assets.mulDiv(
        supply, 
        totalSupply() + _supplyOffset(),
        totalAssets() + _assetsOffset(),
        rounding
    );
}

function _convertToAssets(uint256 shares, Math.Rounding rounding) internal view virtual returns (uint256 assets) {
    return assets.mulDiv(
        shares, 
        totalAssets() + _assetsOffset(), 
        totalSupply() + _supplyOffset(),
        rounding
    );
}

function _supplyOffset() internal virtual returns (uint256) {
    return 1e18;
}

function _assetsOffset() internal virtual returns (uint256) {
    return 1e18;
}

You could override the offset, or use an immutable variable set at construction.

This removes the need for _initialConvertToShares and _initialConvertToAssets. The downside is that the offset "captures" a part of the price change (it acts as a "buffer"). So this is not suitable for all usages.

frangio commented 2 years ago

Can you give a concrete example with numbers of how the virtual offsets would eat part of the profits from a rebasing token? How much would be lost? My guess is the fraction lost would be "assetsOffset / (totalAssets + assetsOffset)"?

Amxx commented 2 years ago

My guess is the fraction lost would be "assetsOffset / (totalAssets + assetsOffset)"?

Yes,

Which is the same as what would be lost with the current implementation if you deposited "assetsOffset" in the constructor (and locked the corresponding shares). Its the same security, without the "cost".

daejunpark commented 2 years ago

I like YieldBox's approach. As @Amxx said, it is essentially a virtual initial deposit that is locked forever (as long as the offset is immutable). So it can effectively mitigate both this issue and #3800 at no cost (zero "security budget")!

However, in its current form, I don't think it properly handles the negative interest case. Suppose that a vault deploys its assets to under-collateralized lending protocols (for better APY) and later some of their loans become default, resulting in a loss of principal, i.e., totalAssets < totalSupply. Now, suppose that all the users attempt to withdraw all their (remaining) assets. Then, the last few unlucky users will never be able to withdraw, and their shares will be permanently locked.

For example, after the loss of principal, suppose that totalAssets = 1e18, totalSupply = 3e18, and assetOffset = supplyOffset = 1e18, where Alice, Bob, and Charlie's share is equally 1e18 respectively. Suppose Alice and Bob redeem all their shares, then they will get 5e17 respectively. Later, Charlie attempts to redeem his shares, but it will revert because the vault's real balance is zero, and asset transfer will fail (even if convertToAsset(1e18) = 5e17 = 1e18 * (0 + 1e18) / (1e18 + 1e18)).

Simply put, the "virtual initial depositor" can only gain but not lose, so some real users (i.e., the last remaining users) will lose more than they should, if not all, in case of negative interest.

Amxx commented 2 years ago

Interesting. That is definitely not a generic solution then...

matthewlilley commented 2 years ago

Interestingly relevent thread for us!

BentoBox was pre ERC-4626, but had a somewhat similar protection to the Morpho-dao solution.

Yieldbox solution was an attempt to improve on that, initial thoughts were it was a step in the right direction, but we hadn't considered all cases.

Okavango commented 2 years ago

In original YieldBox solution assetsOffset = 1. Look: https://github.com/boringcrypto/YieldBox/blob/master/contracts/YieldBoxRebase.sol#L24 That means for your example that Alice, Bob, and Charlie's share is equally 1e36 respectively, and after loosing assets 3e18->1e18 each user will recieve 1e36*(1e18+1)/(3e36+1e18) = (1/3)e18 That is absolutely fair

YieldBox solution fixes the issue

frangio commented 2 years ago

Thanks @Okavango, very important observation. YieldBox uses assetsOffset = 1 and supplyOffset = 1e8. This does seem to be a pretty great solution. The only "issue" is that the number of decimals of the vault token will have 8 more decimals than the assets, which should probably be reflected in decimals(), even though people don't like tokens with decimals other than 18.

daejunpark commented 2 years ago

I agree, it's a great solution! Thanks @Okavango! Now I can see that the assetsOffset = 1 and supplyOffset = 1e8 setup works as an internal fixedpoint representation for shares. So, it's like we have 1 wei assets and 1.00000000 wei shares for the virtual initial deposit. This mitigates the inflation attacks because now we can represent as small as 0.00000001 wei share. This also addresses the issue #3800, without suffering from the negative interest problem because the amount of virtual asset is just 1, so the loss of real users, if any, will never be greater than 1, which means nothing in practice.

I may add more test cases to the erc4626-tests to fuzz these specific scenarios more effectively, so that others can find these subtle issues more easily from their vaults.

frangio commented 2 years ago

Fuzzing would be very nice, we were thinking about that earlier today in conversation with @xaler5. Do you think we can model a general property that a user's deposit of x assets can't be stolen by an attacker through frontrunning with a deposit of y and a donation of z (for arbitrary x, y, z)?

I initially thought this issue was just about slippage and that rounding errors weren't a big part of it, but I can see that I was wrong. My understanding of the attack is now as follows: the attacker mints some shares, then manipulates the share price through a donation, so that a subsequent user deposit of a "large" amount of assets ends up minting a "very small" amount of shares (possibly but not necessarily <1 in real terms), causing the depositor to lose the fractional portion of their shares due to truncation, and allowing the attacker to claim the assets corresponding to that fraction, since they hold the majority of the vault's shares and the losses to precision are distributed proportionaly to shareholders. This is why making 0.00000001 share representable fixes the attack, though it will only do it up to 8 decimals.

If this is the case, there should be an alternative fix where a user's deposit is done only up to the amount of assets that can be losslessly represented by an integer number of shares at the current share price. That is, defining deposit(assets) as mint(previewDeposit(assets)). In this case the assets that are actually deposited are not exactly the requested amount, I don't know if this is allowed by ERC4626 but it should be.

frangio commented 2 years ago

According to EIP-4626:

Mints shares Vault shares to receiver by depositing exactly assets of underlying tokens.

MUST revert if all of assets cannot be deposited (due to deposit limit being reached, slippage, the user not approving enough underlying tokens to the Vault contract, etc).

So the alternative fix I suggested above would not be compliant. However, we can make it compliant with a few adjustments. deposit should have a threshold of how much of assets it's acceptable to lose to precision. If the amount lost is below the threshold, deposit succeds even though a small amount will be socialized to the vault. If the amount lost is above the threshold, deposit reverts to protect the depositor.

Is there something I'm missing that would make this a bad solution?

~Another point that this seems to suggest is that it's always safer to use mint instead of deposit... Is there a reason this shouldn't be a blanket recommendation for users?~ Edit: Doesn't sound like a good idea due to slippage given that users probably think of their deposits denominated not in the shares but in the asset. However, an interesting observation is that mint + finite (but not exact) ERC20 allowance of the underlying asset gives you a sort of "built-in" slippage protection without the need of a router.

frangio commented 1 year ago

To summarize, my proposal for a fix is as follows:

uint256 private _maxRoundingLoss = 0.05e18; // configurable

function deposit(uint256 assets, address receiver) public virtual override returns (uint256) {
    require(assets <= maxDeposit(receiver), "ERC4626: deposit more than max");

    uint256 shares = previewDeposit(assets);
    _deposit(_msgSender(), receiver, assets, shares);

    // Check that shares are redeemable for roughly the amount deposited, with minimal rounding error.
    uint256 redeemable = previewRedeem(shares);
    require(redeemable + _maxRoundingLoss >= assets, "ERC4626: deposit loses assets from rounding");

    return shares;
}

Note the check after _deposit.

daejunpark commented 1 year ago

Great discussion!

Re: fuzzing, yes, I have been thinking of working on that, and will let you know if I find a good way to fuzz such scenarios.

Re: the loss, I thought that the 8 decimals precision for shares is enough for practical purposes; e.g., given the victim's deposit of 1000 ETH, the adversaries need to donate at least 100M ETH to steal 1 ETH; 10M ETH to steal 0.1 ETH, 1M ETH to steal 0.01 ETH. In general, they need to put 10^8 * <the amount to steal> upfront at least.[^1] (I guess they will likely have a better way to utilize such a large amount of capital.) The number of decimals should be adjusted based on the value and total supply of the underlying assets, though.

[^1]: https://www.desmos.com/calculator/ezavnc4lbh where s is the total supply of shares ~~(including the offset)~~, d is the victim's deposit, x is the attacker's donation, and y is the attacker's profit.

Re: the mint vs deposit, I've indeed thought about a similar thing: the mint() method seems more secure than the deposit() method when the vault doesn't have enough liquidity yet, as it gives users more control of the cost basis and proceeds. I'm not sure if it provides the ultimate security though, especially against the price reset (#3800).

That all said, I like your extra check which prevents this slightly different type of slippages. Some thoughts:

The threshold may be given as a percentage instead of an absolute value.
Which would be a better place to put this check: router vs vault? I mean, putting it in the vault would provide a better integrated security, but I'm wondering if this could be exploited for griefing or have UX issues. If it is put in router, users can choose the threshold per deposit.
Would you handle the price reset issue separately?

daejunpark commented 1 year ago

Wait, I think I missed another important effect of virtual offsets for this inflation attack. If we have the virtual offsets (even with just assetsOffset = supplyOffset = 1), the known attack scenario cannot be profitable, since the attacker cannot fully withdraw their donation, because a significant part of the donation goes to the "virtual depositor" and will be locked in the vault forever. Of course, users may still lose their funds if the attacker donates a lot of assets (roughly supplyOffset * <user deposit>), but in that case the attacker's loss will be much more than that. Also, if we set supplyOffset large enough, e.g., 100x of the total supply of underlying assets, that is, supplyOffset = 1e10 for ETH, then the max possible loss will be effectively limited to < 0.01 ETH. So, I think the YieldBox approach is a good solution (provided that supplyOffset is properly set depending on the underlying asset.)

TChairman commented 1 year ago

@frangio what about the option of just preserving the ratio when the vault is emptied? You mentioned this in https://github.com/OpenZeppelin/openzeppelin-contracts/issues/3800. Something like:

    uint256 private lastKnownShares;
    uint256 private lastKnownAssets;

    function _initialConvertToShares(
        uint256 assets,
        Math.Rounding /*rounding*/
    ) internal view virtual returns (uint256 shares) {
        return lastKnownAssets > 0 ? assets.mulDiv(lastKnownShares, lastKnownAssets, rounding) : assets;
    }

    function _initialConvertToAssets(
        uint256 shares,
        Math.Rounding /*rounding*/
    ) internal view virtual returns (uint256) {
        return lastKnownShares > 0 ? assets.mulDiv(lastKnownAssets, lastKnownShares, rounding) : shares;
    }

    function _withdraw(
        address caller,
        address receiver,
        address owner,
        uint256 assets,
        uint256 shares
    ) internal virtual {
        if (caller != owner) {
            _spendAllowance(owner, caller, shares);
        }

        _burn(owner, shares);
        SafeERC20.safeTransfer(_asset, receiver, assets);

         // preserve share price on empty vault
        if (totalAssets()==0 || totalShares() == 0) {
            lastKnownShares = shares;
            lastKnownAssets = assets;
        }

        emit Withdraw(caller, receiver, owner, assets, shares);
    }

TChairman commented 1 year ago

On reconsideration, seems like my solution solves the sudden price change problem (outlined in https://github.com/OpenZeppelin/openzeppelin-contracts/issues/3800) but not the rounding problem. FWIW, on the rounding problem, I prefer the idea of checking for slippage to the idea of some initial deposit.

OpenZeppelin / openzeppelin-contracts