mintYieldFee function does not check for the maxMint amount.

[code423n4]

Lines of code

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

Vulnerability details

Impact

Theoretically, it is possible to mint more than the maxMint amount using the mintYieldFee function in the Vault contract.

Proof of Concept

The functions in Vault contract like mint, mintWithPermit call the _beforeMint function which checks whether _shares are greater than maxMint amount and reverts if it's indeed the case. But, the mintYieldFee amount does not check this. The yieldFeeTotalSupply is a unit256 which gets subtracted with shares, but the maxMint only allows for uint96. So theoretically, it is possible to mint more shares than maxMint, because shares are a uint256 value, not uint96 in the function.

But, there is a scenario where the protocol should be mindful. There is no limit on the decimals of the asset tokens that are used as the vaults can be created permissionless by anyone. And shares also have the same amount of decimals. So, yieldFeeTotalSupply could accumulate in amount (even if the underlying value is less) to be greater than max(uint96) value, which makes it possible for a share greater than maxMint(max(uint96)) amount to be subtracted from yieldFeeTotalSupply and hence possible to mint shares greater than max(uint96) value.

Tools Used

Manual review

Recommended Mitigation Steps

It is recommended that the protocol add the maxMint check in the mintYieldFee function.

Assessed type

Invalid Validation

[c4-sponsor]

asselstine marked the issue as sponsor confirmed

[c4-judge]

Picodes marked the issue as satisfactory

[Picodes]

This finding, https://github.com/code-423n4/2023-07-pooltogether-findings/issues/89 and https://github.com/code-423n4/2023-07-pooltogether-findings/issues/458 would all be mitigated by having the check at the _mint level. Because of this I'll flag them as duplicates.

[c4-judge]

Picodes marked the issue as primary issue

[c4-judge]

Picodes marked issue #458 as primary and marked this issue as a duplicate of 458

[PierrickGT]

I've moved the maxMint check into the _mint function in this PR: https://github.com/GenerationSoftware/pt-v5-vault/pull/11

There is also another edge case we need to check for. When the Vault is partially collateralized, meaning users can withdraw their full deposit but there are not enough yield available in the YieldVault to back the amount of shares minted by mintYieldFee, then the transaction should revert to avoid minting more shares and enter in an undercollateralization state. This issue has been fixed in the following PR: https://github.com/GenerationSoftware/pt-v5-vault/pull/13