Dynamic maxWithdraw causes previous users loss funds

[code423n4]

Lines of code

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

Vulnerability details

Impact

The vault contract is designed to interact with any standard ERC4626 vault and relies on the ratio of _withdrawableAssets(maxWithdraw) and _totalSupplyAmount when calculating exchangeRate. For maxWithdraw, the ERC4626 standard describes it this way:

Maximum amount of the underlying asset that can be withdrawn from the owner balance in the Vault, through a withdraw call. MUST factor in both global and user-specific limits, like if withdrawals are entirely disabled (even temporarily) it MUST return 0.

When the vault restricts partial withdrawal, maxWithdraw decreases but does not set to 0. At this time, exchangeRate calculation decreases: the shares minted by the same amount of asset increase, while the same amount of shares withdrawn decreases, resulting in capital loss. When users withdraw funds at this time, they will suffer losses, and the lost funds will be permanently stuck in the underlying vault and cannot be withdrawn, because of the burn of shares.

Proof of Concept

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

  function _isVaultCollateralized() internal view returns (bool) {
    return _currentExchangeRate() >= _assetUnit;
  }

ExchangeRate calculations depend on maxWithdraw. When the underlying vault restricts withdrawals, maxWithdraw and exchangeRate decrease, resulting in manipulated ratios. Although the code is protected by _isVaultCollateralized, but which will pass if exchangeRate greater than assetUnit.

Tools Used

Manual review

Recommended Mitigation Steps

exchangeRate should not rely on variable maxWithdraw

Assessed type

ERC4626

[Picodes]

The hypothesis is that withdrawals are not restricted and that the call to maxWithdraw represents the underlying assets owned by the PT vault

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid