code-423n4 / 2023-07-pooltogether-findings

12 stars 7 forks source link

Vault wrongly stays in collateralized state for next depositor if yield vault loses all assets #369

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

If vault has depositors and its yield vault gets entirely wiped, isVaultCollateralized stays true.

Allowing the next depositor (victim) to have their shares' worth immediately divided by the current amount of shares in that vault.

Proof of Concept

In testUndercollateralizationExchangeRateReset of Undercollateralization.t.sol change the line to burn all assets of the yield vault:

underlyingAsset.burn(address(yieldVault), 20_000_000e18);

observe by running forge test --match-test testUndercollateralizationExchangeRateReset that assertEq(vault.isVaultCollateralized(), false); is not passing anymore.

Since the vault is still considered to be collateralized, a user can come and deposit in this vault that will:

So their shares price will be immediately divided by the total amount of shares in the vault. Loss of funds.

Tools Used

Foundry

Recommended Mitigation Steps

Check _totalSupplyAmount if _withdrawableAssets is 0 in _currentExchangeRate

Assessed type

Invalid Validation

Picodes commented 1 year ago

When minting there will also be a call to https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1176, updating the rate?

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Insufficient proof