Revert on Loss until Loss is fully repaid for Yearn V3

[code423n4]

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1007-L1030

Vulnerability details

If a loss is incurred in a Yearn V3 Yield Vault, withdrawing will stop working until the Loss has been recovered

This will require either:

• Yearn paying up for the loss
• PT paying up for the loss
• An emergency fund being spun

This is because the PT Vault uses the standard ERC4626 call to _yieldVault.withdraw(_assets, address(this), address(this));

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1007-L1030

  function _withdraw(
    address _caller,
    address _receiver,
    address _owner,
    uint256 _assets,
    uint256 _shares
  ) internal virtual override {
    if (_caller != _owner) {
      _spendAllowance(_owner, _caller, _shares);
    }

    // If _asset is ERC777, `transfer` can trigger a reentrancy AFTER the transfer happens through the
    // `tokensReceived` hook. On the other hand, the `tokensToSend` hook, that is triggered before the transfer,
    // calls the vault, which is assumed not malicious.
    //
    // Conclusion: we need to do the transfer after the burn so that any reentrancy would happen after the
    // shares are burned and after the assets are transferred, which is a valid state.
    _burn(_owner, _shares);

    _yieldVault.withdraw(_assets, address(this), address(this)); /// @audit Default Withdraw Call
    SafeERC20.safeTransfer(IERC20(asset()), _receiver, _assets);

    emit Withdraw(_caller, _receiver, _owner, _assets, _shares);
  }

Meaning that Yearn V3 (and V2 as well) will interpret this as a No Loss Withdrawal

https://github.com/yearn/yearn-vaults-v3/blob/1e6bc67cf70352e9567e67404d73856cc343b634/contracts/VaultV3.vy#L1539-L1558

  @nonreentrant("lock")
def withdraw(
    assets: uint256, 
    receiver: address, 
    owner: address, 
    max_loss: uint256 = 0,
    strategies: DynArray[address, MAX_QUEUE] = []
) -> uint256:

Yearn V3 sets the default loss to 0 BPS, meaning that any loss in the withdrawal will cause a revert

POC

• Use Yearn V3
• Deposit as intended
• Earn into the Strategy
• Trigger a loss in the Strategy
• Withdraw Stops working even if the Vault is properly colllateralized

Mitigation

Consider whether to use Yearn V3, and whether you should add a Lossy Withdrawal Option, or perhaps a Router that allows loss but protects people from losing too much

Assessed type

ERC4626

[asselstine]

It seems that Yearn V3 does not adhere to the 4626 spec. I believe this is an issue with Yearn V3, and not our code.

ERC-4626:

[maxWithdraw] MUST NOT be higher than the actual maximum that would be accepted (it should underestimate if necessary).

The Yearn V3 maxWithdraw and it's internal implementation

The Yearn V3's 4626 withdraw() function default max_loss to zero, but reverts if there is any loss. This is inconsistent with the ERC-4626 spec.

If Yearn V3 stuck to the spec, instead of a revert we'd see:

// because maxWithdraw is zero, then exchangeRate is zero, therefore _assets = 0
uint256 _assets = _convertToAssets(_shares, Math.Rounding.Down);
_withdraw(msg.sender, _receiver, _owner, _assets, _shares); // no-op

The withdraw would succeed but would burn shares and return zero assets. However, this is the point of allowing undercollateralized exits: to allow users to do whatever they want to do. It's a double-edge sword in that sense. Technically speaking, this is behaving as intended and should be fine for other 4626 vaults.

It seems to me we might need a custom Vault or adapter for Yearn V3.

[c4-sponsor]

asselstine marked the issue as sponsor disputed

[Picodes]

@asselstine was it in your stated intentions to do something compatible with Yearn V3?

I do agree with the sponsor here. Ultimately considering the diversity of ERC4626 implementations it's fair to consider that the vault will need to be adapted for custom implementations like Yearn V3. I'll downgrade to Low.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-b

[asselstine]

@Picodes Yes! We will write a custom adapter for Yearn V3.

The Vault works as intended for yield sources that strictly stick to 4626, so I don't see an issue. That being said if you want to retain the issue under 'QA' for posterity I understand.