Open c4-bot-10 opened 7 months ago
raymondfam marked the issue as insufficient quality report
raymondfam marked the issue as primary issue
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.
hansfriese marked the issue as unsatisfactory: Invalid
hansfriese marked the issue as satisfactory
It's a valid concern and QA is more appropriate due to the low impact.
hansfriese changed the severity to QA (Quality Assurance)
I will mark as grade-a with some unique issues.
hansfriese marked the issue as grade-a
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: https://github.com/code-423n4/2022-09-y2k-finance-findings/issues/47 https://github.com/code-423n4/2023-05-maia-findings/issues/585 https://github.com/code-423n4/2023-02-ethos-findings/issues/247 https://github.com/code-423n4/2022-06-notional-coop-findings/issues/155
After checking again, I agree Medium is more appropriate as it may violate ERC4626 compliance.
hansfriese removed the grade
This previously downgraded issue has been upgraded by hansfriese
hansfriese marked the issue as satisfactory
hansfriese marked the issue as selected for report
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.
trmid (sponsor) acknowledged
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 toyieldVault.maxDeposit(address(this))
. However,_depositAndMint()
deposits usingyieldVault.mint()
which may have a stricter limit thanyieldVault.deposit()
. In that case depositingmaxDeposit()
would revert, which violates EIP-4626.Recommended Mitigation Steps
Use
yieldVault.previewRedeem(yieldVault.maxMint())
.Assessed type
ERC4626