DISCREPENCY BETWEEN DOCUMENTATION AND `Vault._currentExchangeRate` FUNCTION IMPLEMENTATION

[code423n4]

Lines of code

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

Vulnerability details

Impact

The Vault._currentExchangeRate function, is used to calculate the exchange rate between the amount of assets withdrawable from the YieldVault and the amount of shares minted by this Vault. In the Natspec comment the below function beharviour is explained as follows:

    * @dev We exclude the amount of yield generated by the YieldVault, so user can only withdraw their share of deposits.
    *      Except when the vault is undercollateralized, in this case, any unclaim yield fee is included in the calculation.

The function implementation for the above expected behaviour is depicted below:

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

if (_withdrawableAssets > _totalSupplyToAssets) {
  _withdrawableAssets = _withdrawableAssets - (_withdrawableAssets - _totalSupplyToAssets);
}
//@audit-issue - unclaimed yield fee is not included in the calculation.
if (_totalSupplyAmount != 0 && _withdrawableAssets != 0) {
  return _withdrawableAssets.mulDiv(_assetUnit, _totalSupplyAmount, Math.Rounding.Down);
}

The undercollateralization happens when _withdrawableAssets < _totalSupplyToAssets. But in this case the current exchange rate is calculated normally without accounting for the unclaimed yield fee in the calculation as shown above.

Hence the function execution does not follow the expected functionality of the protocol. As a result the calculated currentExhangeRate will be errorneous.

Proof of Concept

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

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

   * @dev We exclude the amount of yield generated by the YieldVault, so user can only withdraw their share of deposits.
   *      Except when the vault is undercollateralized, in this case, any unclaim yield fee is included in the calculation.

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

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to update the Vault._currentExchangeRate function, so that in case of undercollateralization, unclaimed yield fee will be included in the calculation of the Vault._currentExchangeRate by adding that amount to _withdrawableAssets amount as mentioned in the Natspec comments above.

Assessed type

Other

[asselstine]

Perhaps the warden mis-understood the comment: the unclaimed fee is included in the sense that it is consumed if the vault is under-collateralized. It's not about the value being used in the calculation, but rather that the fee is ignored so that the liquidity is consumed to cover any missing deposits.

[c4-sponsor]

asselstine marked the issue as sponsor disputed

[Picodes]

Note that "function incorrect as to spec, issues with comments" is of Low severity according to the rules.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)