Open code423n4 opened 12 months ago
Picodes marked the issue as primary issue
Hmm not sure about this one. Will mark as confirm and figure it out later
asselstine marked the issue as sponsor confirmed
@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.
Picodes marked the issue as satisfactory
Picodes marked the issue as selected for report
@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.
The code has been improved and clarified in the following PR: https://github.com/GenerationSoftware/pt-v5-vault/pull/13
Keeping high severity here because of the issue in case of temporary under-collateralization.
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
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.Tool used
Manual Review
Recommended Mitigation Steps
Remove these lines of code that limit the
_withdrawableAssets
Assessed type
Context