The exchange rate is decreasing in Vault

[code423n4]

Lines of code

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

Vulnerability details

Impact

The exchange rate of the vaults will be decreasing and this will block core functionalities.

Proof of Concept

The exchange rate of the Vault is calculated as follows:

min(_withdrawableAssets, _totalSupplyToAssets) * _assetUnit / _totalSupplyAmount
= min(_withdrawableAssets * _assetUnit / _totalSupplyAmount, _lastRecordedExchangeRate)

So the resulting exchange rate from _currentExchangeRate method is less than or equal to _lastRecordedExchangeRate in normal situation when the total supply of current vault and the withdrawable assets from the yield vault is not zero.

_lastRecordedExchangeRate is only updated by the result of _currentExchangeRate method, so the exchange rate of the vault will be decreasing. It starts with 1, so the exchange rate will be less than 1. In this situation, maxDeposit and maxMint will return 0, so depositing to the vault will not work. The liquidation pair also can't call liquidate. Since these functionalities take important roles in the protocol, so the impact will be huge.

Tools Used

Manual Review

Recommended Mitigation Steps

I think the implementation of _currentExchangeRate is not correct at the moment, but I am not sure how to mitigate this method.

Assessed type

Error

[c4-judge]

Picodes marked the issue as unsatisfactory: Insufficient proof

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as duplicate of #443

[c4-judge]

Picodes marked the issue as partial-50

[Picodes]

Partial credit as it seems the warden as an intuition that something is off but no impact or mitigation is proposed

[auditor0517]

This report explains why the exchange rate is decreasing. And it indicates the impacts related to deposit and liquidations. Although this report did not show the mitigation, mitigation for this issue is not obvious. I think no mitigation is better than the wrong mitigation. partial-50 is too tight for this issue.

[Picodes]

@auditor0517 I respectfully disagree.

As I understand it the High severity (loss of funds) scenario here is the fact that "when the vault is undercollateralized (_currentExchangeRate < _assetUnit), it can't be further collateralized", so users will withdraw less than intended. Here, this report highlights that it'd lead to "maxDeposit and maxMint will return 0, so depositing to the vault will not work. The liquidation pair also can't call liquidate. Since these functionalities take important roles in the protocol, the impact will be huge.", which is a scenario of Medium severity (DoS). In addition to this, the report is vague about the impact and mitigation. So overall partial-50 of a High issue seems justified to me. Note that it is better than Med.

Please let me know if I am missing something here. Also, just out of transparency, is this your report?

[auditor0517]

@Picodes Thanks for your reply. It's not mine but I've checked their issues as I am going to collaborate with them later.