Vault is not compatible with some erc4626 vaults

[code423n4]

Lines of code

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

Vulnerability details

Impact

Vault is not compatible with some erc4626 vaults. Depositors can loose funds.

Proof of Concept

Anyone can build Vault with underlying vault inside, which should earn yields. When user deposits/withdraws then _convertToShares function is used to determine amount of shares that user will receive for provided assets amount. This function calls _currentExchangeRate to find out current rate.

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

  function _currentExchangeRate() internal view returns (uint256) {
    uint256 _totalSupplyAmount = _totalSupply();
    uint256 _totalSupplyToAssets = _convertToAssets(
      _totalSupplyAmount,
      _lastRecordedExchangeRate,
      Math.Rounding.Down
    );

    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);
    }

    return _assetUnit;
  }

As you can see in order to find current exchange rate _yieldVault.maxWithdraw(address(this)) is used. In case if this value(which is supposed to be full amount of deposits + yields inside _yieldVault) is less than _totalSupplyAmount(which is total supply * _lastRecordedExchangeRate), then rate will be decreased, which means that vault lost funds and users should receive less when withdraw. Later, this new _currentExchangeRate will be stored as _lastRecordedExchangeRate.

Now when i explained how rate is changed i can explain the problem with some erc4626 vaults.

There are some erc4626 vaults as DnGmxSeniorVault, that collect deposited funds and borrow them. When you call maxWithdraw for such vaults, then in case if not enough funds are present, because of some borrow percentage on vault, then amount returned can be less than real balance of caller. In case if such wrong value is returned, then depositors of Pool vault will face losses as their exchange rate will be less than 1. When DnGmxSeniorVault will again have enough balance(when debt is repaid by jn vault), then exchange rate will be 1 again.

Another erc4626 vaults that can create problems are vaults that have withdraw limit. In that case if Pool vault has balance inside yield vault that is bigger than borrow limit, then depositors will face same problem which leads to loose of funds.

Tools Used

VsCode

Recommended Mitigation Steps

You need to consider cases, when some vaults can't be used as yield vaults and aware vault creators about that.

Assessed type

Error

[c4-sponsor]

asselstine marked the issue as sponsor confirmed

[c4-judge]

Picodes marked the issue as primary issue

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[asselstine]

This is more a general warning about the possible incompatibilities of 4626 vaults

[asselstine]

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