yDAI and yCrvBUSD vaults withdraw typically fail due to slippage error

[kx9x]

yCrvBUSD seems to use some yVaultCheck contract at https://etherscan.io/address/0xE309978497DfC15bb4F04755005F6410CAdB4103, for withdrawals.

Here is the relevant snippet from store.jsx

{
          id: 'crvBUSD',
          name: 'curve.fi/busd LP',
          symbol: 'crvBUSD',
          description: 'yDAI/yUSDC/yUSDT/yBUSD',
          vaultSymbol: 'ycrvBUSD',
          erc20address: '0x3B3Ac5386837Dc563660FB6a0937DFAa5924333B',
          vaultContractAddress: '0x2994529c0652d127b7842094103715ec5299bbed',
          vaultContractABI: config.vaultContractV3ABI,
          balance: 0,
          vaultBalance: 0,
          decimals: 18,
          deposit: true,
          depositAll: true,
          withdraw: true,
          withdrawAll: true,
          depositDisabled: false,
          lastMeasurement: 10709740,
          measurement: 1e18,
          price_id: 'lp-bcurve',
          yVaultCheckAddress: '0xe309978497dfc15bb4f04755005f6410cadb4103'
       },

If you check the tx history on this yVaultCheck contract, you can see that many withdrawAll calls fail due to slippage errors:

Here is one for example: https://etherscan.io/tx/0x0b50ec3208f9223414ca1c7193ff8afff49ef6b5ed1b0e0ca8742f5867f51518

One user came to Discord unable to withdraw:

Eventually we figured out that the TX was going to a contract other than the yCrvBUSD vault:

Eventually the user was able to withdraw using Etherscan and the yCrvBusd contract directly:

Here is their TX on the yCrvBUSD contract that ended up succeeding: https://etherscan.io/tx/0x92997f68db47d4bb8e38d97b3b59c5430e7af7c794e5aaeb557777876c2d4354

It seems like the yVaultCheck contract is faulty at the moment and users who use the yearn.finance to withdraw may not be able to withdraw right now.

What is the purpose of this contract? Why don't we just hook the UI into calling withdraw on the yCrvBUSD contract directly?

[kx9x]

From the yVaultCheck contract:

 // No rebalance implementation for lower fees and faster swaps
    function withdraw(uint _shares) public {
        IERC20(address(vault)).safeTransferFrom(msg.sender, address(this), _shares);
        IERC20 _underlying = IERC20(vault.token());

        uint _expected = vault.balanceOf(address(this));
        _expected = _expected.mul(vault.getPricePerFullShare()).div(1e18);
        _expected = _expected.mul(9999).div(10000);

        uint _before = _underlying.balanceOf(address(this));
        vault.withdrawAll();
        uint _after = _underlying.balanceOf(address(this));
        require(_after.sub(_before) >= _expected, "slippage");
        _underlying.safeTransfer(msg.sender, _underlying.balanceOf(address(this)));
    }

I am wondering how this 99.99% check is supposed to be expected to pass. The 0.5% withdrawal fee would always make this fail, right? Wouldn't a user's withdrawal only work if there was enough not-yet-deployed asset to avoid the withdrawal fee?

Is the solution to this problem perhaps to work the withdrawal fee into the acceptable slippage equation?

[kx9x]

This also may affect DAI vault users since that also seems to use a yVaultCheck contract for the withdraw

[banteg]

First time I see this facade contract. Why not withdraw from the vault directly?

[kx9x]

First time I see this facade contract. Why not withdraw from the vault directly?

That is what I was thinking. The user had to go to Etherscan and withdraw directly from the yCrvBUSD vault, which worked.

It looks like this yVaultCheck was an attempt at protecting the user against sudden drops in getPricePerFullShare? But that would only happen if there were a bug and this probably isn't the way to defend against that.

Right now it is inadvertently only allowing users to withdraw when they don't have to pay a fee. Which is a cool idea for perhaps a different button in the UI, but definitely not a good UX for a normal withdraw/withdraw all experience.

[kx9x]

Also, this yVaultCheck contract isn't on the protocol repo and I was scared for a second that the user got scammed into using some fake contract. Turns out this contract was deployed 18 days ago by the Yearn Deployer though, which is relatively recently.

[kx9x]

@antonnell Hi, I believe you added this yVaultCheck contract at https://github.com/iearn-finance/iearn-finance/commit/17d33fd520b2dfe4fcc295455caa0768a24943c6#diff-c7206dd7714c5f5f83802b466ae2cdd0

I think that the slippage constraint is actually causing some unintended issues where users cannot withdraw if they have to pay the 0.5% withdrawal fee.

I think for the short-term, it might be a good idea to switch the UI to using the vault contracts (crvBUSD and DAI) directly for withdrawal and then perhaps reevaluated the intended benefits of the yVaultCheck and adjusting the slippage to account for withdrawal fee if possible.

I could make a PR for the short-term change if you'd all like

[milkyklim]

Can you open a PR @listonjesse?

[kx9x]

@milkyklim, sure just opened one here

[kx9x]

Another user ran into this issue: https://etherscan.io/tx/0x0d76f42171f1e08bf85f73abd3bcb9f5045fbf928e920d5602d7b7894f51a7da

Yearn ended up missing out on another withdrawal fee and the user lost gas.

[kx9x]

yVaultCheck for the yDAI vault just had a ton of failed TXs:

Another user bringing this up in the Discord:

[kx9x]

Total wasted gas by failed TXs in yDAI yVaultCheck in the last 5 hours is 1.309814142 ETH.

Total wasted gas by failed TXs in yCrvBUSD yVaultCheck in the last few days is 0.237159047 ETH.

[kx9x]

Merge into y branch: https://github.com/iearn-finance/iearn-finance/commit/23f321084289b1e9548bc127a65e36af6152cc17

[ghost]

I discussed this with Banteg/protocol team. The yVaultCheck contract is failing a slippage check when the user needs to withdraw from the underlying strategy rather than the vault itself. We have decided to remove this check and call withdraw on the vault contracts directly.

As @listonjesse pointed out removing this check will reduce user gas costs and reduce the number of failed withdraw transactions.

In the future we may wish to calculate slippage on the front-end and warn the user if the expected slippage is high.

commit 23f321084289b1e9548bc127a65e36af6152cc17 (HEAD -> y, origin/y)
Author: Jesse Liston <jeliston@microsoft.com>
Date:   Sun Oct 11 12:38:01 2020 -0700

    Add yVaultCheckDisabled attribute (#104)