The _currentExchangeRate of the Vault contract can't increase, and always be lower than or equal to _assetUnit

[code423n4]

Lines of code

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

Vulnerability details

Impact

The _currentExchangeRate of the Vault contract can not increase, and always be lower than or equal to _assetUnit. Therefore, when the vault is undercollateralized (_currentExchangeRate < _assetUnit), it can't be further collateralized.

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

In case _totalSupplyAmount != 0 && _withdrawableAssets != 0, _currentExchangeRate function will return a value _withdrawableAssets * _assetUnit / _totalSupplyAmount.

However _withdrawableAssets can not exceed _totalSupplyToAssets, which is equal to _totalSupplyAmount * _lastRecordedExchangeRate / _assetUnit.

Therefore, _currentExchangeRate always be lower than or equal to _lastRecordedExchangeRate.

Testing:
Add this assert line and run forge test , all tests will passed.

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

Tool used

Manual Review

Recommended Mitigation Steps

Remove these lines of code that limit the _withdrawableAssets

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

Assessed type

Context

[c4-judge]

Picodes marked the issue as primary issue

[asselstine]

Hmm not sure about this one. Will mark as confirm and figure it out later

[c4-sponsor]

asselstine marked the issue as sponsor confirmed

[Picodes]

@asselstine did you figure it out?
It seems to me that because of if (_withdrawableAssets > _convertToAssets(_totalSupplyAmount, _lastRecordedExchangeRate, Math.Rounding.Down)), the rate indeed can't increase which is a huge issue in case of a momentary undercollateralization.

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[PierrickGT]

@asselstine did you figure it out? It seems to me that because of if (_withdrawableAssets > _convertToAssets(_totalSupplyAmount, _lastRecordedExchangeRate, Math.Rounding.Down)), the rate indeed can't increase which is a huge issue in case of a momentary undercollateralization.

Indeed, the exchange rate should not be greater than 1 cause users should not be able to claim the yield that has accrued in the YieldVault.
That's why we have the following condition:

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

We subtract the yield from the total amount, the yield being the difference between _withdrawableAssets and _totalSupplyToAssets.

If the YIeldVault becomes undercollateralized, users won't be able to deposit anymore but will be able to withdraw their share of the deposit and any yield that as accrued and has not been claimed yet will be shared proportionally amongst users.

[PierrickGT]

The code has been improved and clarified in the following PR: GenerationSoftware/pt-v5-vault#13

[Picodes]

Keeping high severity here because of the issue in case of temporary under-collateralization.