maxDeposit() uses yieldVault.maxDeposit() but _depositAndMint() uses yieldVault.mint()

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVault.sol#L383

Vulnerability details

Impact

maxDeposit() might return a value greater than can be deposited, violating EIP-4626.

Proof of Concept

maxDeposit() returns up to yieldVault.maxDeposit(address(this)). However, _depositAndMint() deposits using yieldVault.mint() which may have a stricter limit than yieldVault.deposit(). In that case depositing maxDeposit() would revert, which violates EIP-4626.

Recommended Mitigation Steps

Use yieldVault.previewRedeem(yieldVault.maxMint()).

Assessed type

ERC4626

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

maxDeposit() (same as maxMint()) is to check the maximum withdrawable for each account where yieldVault.mint would simply revert if the cap has not been conformed to.

[c4-judge]

hansfriese marked the issue as unsatisfactory:
Invalid

[c4-judge]

hansfriese marked the issue as satisfactory

[hansfriese]

It's a valid concern and QA is more appropriate due to the low impact.

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[hansfriese]

I will mark as grade-a with some unique issues.

[c4-judge]

hansfriese marked the issue as grade-a

[d3e4]

Isn't a violation of EIP-4626, for a vault that claims to be compliant, at least a Medium severity because of the integration issues it implies?

Note that being EIP-4626 compliant is explicitly stated in the README and that adherence to this was listed as one of the Attack ideas.

This comment also applies to #336.

Here are a few previous examples awarded High or Medium:
code-423n4/2022-09-y2k-finance-findings#47
code-423n4/2023-05-maia-findings#585
code-423n4/2023-02-ethos-findings#247
code-423n4/2022-06-notional-coop-findings#155

[hansfriese]

After checking again, I agree Medium is more appropriate as it may violate ERC4626 compliance.

[c4-judge]

hansfriese removed the grade

[c4-judge]

This previously downgraded issue has been upgraded by hansfriese

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as selected for report

[trmid]

After further evaluation, the suggested mitigation seems to cause issues in common ERC4626 yield vaults since maxMint commonly returns type(uint256).max and calling previewRedeem or previewMint with such a high value also commonly causes an overflow error on conversion.

As long as the yield vault maxDeposit function takes into account any internal supply limits, the current implementation is unlikely to have any compatibility issues and will be left as-is.

[c4-sponsor]

trmid (sponsor) acknowledged