`_maxYieldVaultWithdraw()` uses `yieldVault.convertToAssets()`

[c4-bot-5]

Lines of code

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

Vulnerability details

Impact

maxWithdraw() and maxRedeem() may return too much, violating EIP-4626. liquidatableBalanceOf() may return too much.

Proof of Concept

function _maxYieldVaultWithdraw() internal view returns (uint256) {
    return yieldVault.convertToAssets(yieldVault.maxRedeem(address(this)));
}

uses yieldVault.convertToAssets() which per EIP-4626 is only approximate. Especially, it might return too much, and thus _maxYieldVaultWithdraw() might return too much. _maxYieldVaultWithdraw() is used in maxWithdraw(), in maxRedeem(), and in liquidatableBalanceOf() which functions may thus return too much. In the case of maxWithdraw() and maxRedeem() this violates EIP-4626.

Recommended Mitigation Steps

Use yieldVault.previewRedeem(yieldVault.maxRedeem(address(this))).

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]

It will be doing the same thing:

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

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC4626.sol#L166-L168

[c4-judge]

hansfriese marked the issue as unsatisfactory: Insufficient proof

[c4-judge]

hansfriese marked the issue as satisfactory

[hansfriese]

QA is more appropriate.

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

hansfriese marked the issue as grade-a

[d3e4]

See my comment on #335.

[hansfriese]

Upgrade to Medium for the same reason as #335.

[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]

mitigation: https://github.com/GenerationSoftware/pt-v5-vault/pull/97 See https://github.com/GenerationSoftware/pt-v5-vault/pull/96 for more details

[c4-sponsor]

trmid (sponsor) confirmed