Dust sweeping may exceed yield vault deposit limit

[c4-bot-7]

Lines of code

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

Vulnerability details

Impact

Deposits may be DoS-ed.

Proof of Concept

In _depositAndMint(), previously accumulated dust is swept into the yield vault along with the deposit:

// Previously accumulated dust is swept into the yield vault along with the deposit.
uint256 _assetsWithDust = _asset.balanceOf(address(this));
_asset.approve(address(yieldVault), _assetsWithDust);

// The shares are calculated and then minted directly to mitigate rounding error loss.
uint256 _yieldVaultShares = yieldVault.previewDeposit(_assetsWithDust);
uint256 _assetsUsed = yieldVault.mint(_yieldVaultShares, address(this));

_assetsWithDust should generally be small enough, but if it is large, e.g. if assets are donated to PrizeVault by an attacker, this amount may exceed a potential deposit or mint limit of the yield vault, causing yieldVault.previewDeposit() or yieldVault.mint() to revert. This would thus DoS all deposits into PrizeVault.

Recommended Mitigation Steps

Cap _assetsWithDust accordingly, or add a function to manually deposit an excessive asset balance in smaller parts.

Assessed type

DoS

[raymondfam]

The caller will have to pre-determine this via maxDeposit() that will have _latentBalance taken care of:

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

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[hansfriese]

maxDeposit() will return 0 when _latentBalance exceeds a deposit or mint limit of the yield vault.

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid

[c4-judge]

hansfriese marked the issue as satisfactory

[hansfriese]

QA is appropriate as there would be no advantage to the attacker after a donation of a non-dust amount.

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

hansfriese marked the issue as grade-a

[d3e4]

QA is appropriate as there would be no advantage to the attacker after a donation of a non-dust amount.

That would then be a griefing attack, i.e. "the function of the protocol or its availability could be impacted", which is Medium. But it is not correct to say there would be no advantage to the attacker (besides satisfying his spite). One can imagine a yield vault which is about to distribute a fixed sum to its holders, or a similar situation. It would then be in the interest of a current holder to prevent further deposits until after the distribution. This bug would greatly assist him in that exploit.

Note that

maxDeposit() will return 0 when _latentBalance exceeds a deposit or mint limit of the yield vault.

is an illustration of the same problem. The protocol should not allow itself to be DoS-ed by blindly trying to deposit the entire latent balance when this is not possible without splitting it up.

[hansfriese]

I understand your concern but the impact is low.

My primary concern is why the attacker would donate assets rather than depositing them into the vault. If they were to deposit assets instead of donating, the PrizeVault would still reach the deposit/mint limit, resulting in the attacker obtaining more shares.

I will maintain it as QA.