Exchange Rate Change in Case of Lossy Strategy will cause the Vault to Undercollateralized for generic ERC4626 Yield Vaults

[code423n4]

Lines of code

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

Vulnerability details

For Yield Vaults that use Lossy Strategies, the PT Vault will burn more Yield Vault Shares than intended when processing a withdrawal, socializing a loss at the advantage of early withdrawers

withdraw calls _convertToShares

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

    uint256 _shares = _convertToShares(_assets, Math.Rounding.Up);

Which uses the _currentExchangeRate()

This in turn computes the withdrawableAssets by fetching yieldVault.maxWithdraw and then mapping the principal vs the totalSupply

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

    uint256 _withdrawableAssets = _yieldVault.maxWithdraw(address(this));

    if (_withdrawableAssets > _totalSupplyToAssets) {
      _withdrawableAssets = _withdrawableAssets - (_withdrawableAssets - _totalSupplyToAssets);
    }

    if (_totalSupplyAmount != 0 && _withdrawableAssets != 0) {
      return _withdrawableAssets.mulDiv(_assetUnit, _totalSupplyAmount, Math.Rounding.Down);
    }

This calculation is based on maxWithdraw, which is a view function

Most implementation of maxWithdraw will simply map the available tokens to the shares owned by the owner, see OZ for example:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/121be5dd09caa2f7ce731f0806b0e14761023bd6/contracts/token/ERC20/extensions/ERC4626.sol#L141-L143

    function maxWithdraw(address owner) public view virtual returns (uint256) {
        return _convertToAssets(balanceOf(owner), Math.Rounding.Floor);
    }

Or similarly, Yearn V3

Due to the implementation, losses can be applied during YieldVault.withdraw, causing the burning of more shares than intended

Because the PT Vault computes the shares to burn before accounting for a potential loss:

• No loss will be accounted for in maxWithdraw (since it will use static value for assets in the vault and assets in the strategy)

• The loss will be locked during _withdraw, but it will not be checked, the specifics of the loss are that it will cause burning of more shares in order to receive the intended assets

• This will cause the user to pay shares from PT Vault

• But it will cause PT Vault to pay more yieldVault Shares than intended, effectively diluting everyone else and socializing the losses on the laggards

Proof Of Concept

• Call withdraw
• This will compute the shares to burn to the user via _convertToShares
• PT Vault will call YieldVault.withdraw with the amount necessary to withdraw
• The Vault will take a loss, the loss will cause more shares from PT Vault to be burned, socializing a loss to other depositors

Mitigation

Ensures that the PPFS of the Yield Vault doesn't decrease, or add functions to handle lossy withdrawals

Assessed type

ERC4626

[c4-sponsor]

asselstine marked the issue as sponsor confirmed

[c4-judge]

Picodes marked the issue as satisfactory

[PierrickGT]

I don't entirely understand the issue here, to me it looks like this is the intended behavior. Users knowingly deposited in a Vault relying on a YieldVault that employs Lossy Strategies to generate yield, let's say for example an option protocol, the position of the Vault loses value so all shares are backed by less underlying assets. When withdrawing, people will only be able to withdraw their shares of deposits which indeed socializes the loss. I think for this kind of YieldVaults, we will need to develop custom Vaults cause it's a pretty specific use case.

[asselstine]

Related: https://github.com/code-423n4/2023-07-pooltogether-findings/issues/79

[asselstine]

Fixed in https://github.com/GenerationSoftware/pt-v5-vault/pull/27

[PierrickGT]

Working PR here: https://github.com/GenerationSoftware/pt-v5-vault/pull/37