PoolTogether - GalloDaSballo's results

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

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

// 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