Vault will stop participating in draws in case if they deposited maximum assets to the underlying vault

[code423n4]

Lines of code

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

Vulnerability details

Impact

Vault will stop participating in draws in case if they deposited maximum assets to the underlying vault.

Proof of Concept

Vault contract has maxMint function. This function first checks allowed amount to mint in the PtVault and then also checks amount allowed in underlying vault. It is very likely that this value will be less than in PtVault as some vaults can have restriction for 1 staker.

This means that once such amount of shares is minted, then it's not allowed to mint anymore, so each deposit/mint will revert.

In order to contribute earned yield to the prize pool, liquidation pair should call liquidate function. It's the only way to send earned yield to the prize pool. The process is next: liquidator will provide POOl tokens on behalf of Vault and Vault should mint corresponding amount of shares for liquidator. In case if this call will be done after max amount is minted, then it will revert, which means that it will be not possible to send earned yield to the pool and earn prizes. In this moment the main purpose of vault stopped working: users earned yield to participate in prize distributions, but they can't do that.

Of course, when someone will withdraw, then it will be possible to liquidate again, but that will lead to bad user experience and also someone can ddos pool, by depositing again to not allow send contributions to the prize pool.

Another thing that is related to this issue is that in case if maxMint was reached, then mintYieldFee can't be called as well, as it also mints shares.

Tools Used

VsCode

Recommended Mitigation Steps

Maybe it will be best to not check for maxMint for liquidations and fee minting.

Assessed type

Error

[c4-sponsor]

asselstine marked the issue as sponsor confirmed

[djb15]

Nice find! Although I'm not sure if this is a bug or intended behaviour? If you don't check maxMint then liquidations and fee minting don't conform to the ERC4626 standard, which is a bug in itself?

And also the V5 vault owner has no control of the underlying yield vault, so if the maxMint of the underlying yield vault has been reached due to other users (i.e. not from PoolTogether vaults) depositing in the yield vault then that's just how it is.

And finally maxMint of the underlying vault is probably going to be a uint256...even if it's a uint96 like the V5 vaults, that's still 11 decimals of tokens with 18 decimals of precision.

[stalinMacias]

And finally maxMint of the underlying vault is probably going to be a uint256...even if it's a uint96 like the V5 vaults, that's still 11 decimals of tokens with 18 decimals of precision.

I agree with dirky's comments, in addition to his comments, this looks to be also an administration thing, it's the vault's deployer dutty to verify that the underlying vault is compliant with the standard.

I think the severity of this finding should be assessed based on the likelihood of the underlying vault reaching the maxMint, if it's very unlikely (as per dirkky's comments), then, could it be a low sev?

[rvierdiiev]

The solution that i proposed is only one of several possibilities how you can fix that issue. For example another solution can be to transfer yield to the liquidator directly, without minting new shares. In such way, vault will still be working fine even when limit is reached. This issue describes edge case, which still can happen. And in case if it will, then vault will not work as it is designed. I would like the judge look into this issue.

[PierrickGT]

Interesting reasoning.

Vault shares must be backed 1:1 by underlying assets deposited into the Yield Vault. If we mint more Vault shares than underlying assets withdrawable, we will enter in an undercollateralized state.

That being said, yield that accumulated in the Yield Vault is liquidated in exchange of Prize Tokens by minting more Vault shares. So even if maxMint is reached, we should be able to liquidate this yield.

Fixed in this PR: https://github.com/GenerationSoftware/pt-v5-vault/pull/43

[c4-judge]

Picodes marked the issue as satisfactory

[Picodes]

Interesting issue. Considering the sponsor's answer I consider this a bug and fulfilling requirements for Med severity under broken functionality with external requirements.

[thebrittfactor]

Judge requested via DM for this to be selected for report.