0x73696d616f - PrizeVault::maxDeposit() is not ERC4626 compliant as it does not consider rounding errors nor restricted receivers, nor maxMint()

[sherlock-admin4]

0x73696d616f

medium

PrizeVault::maxDeposit() is not ERC4626 compliant as it does not consider rounding errors nor restricted receivers, nor maxMint()

Summary

PrizeVault::maxDeposit() returns 0 if the total assets are smaller than the debt, to consider lossy deposits, but this will overestimate the amount that may be deposited if the yield vault incurs in rounding errors and the yield buffer is depleted. Additionally, some receivers are restricted, such as address(0) and SPONSORSHIP_ADDRESS, which are not considered. And finally, it does not check maxMint().

The buffer may be depleted if the underlying yield vault registers losses.

Vulnerability Detail

EIP4626 specifies that maxDeposit() must return the maximum amount of assets that may be deposited and underestimate if necessary, but this is not the case.

MUST return the maximum amount of assets deposit would allow to be deposited for receiver and not cause a revert, which MUST NOT be higher than the actual maximum that would be accepted (it should underestimate if necessary).

Whenever a yield incurs rounding errors, maxDeposit() will overestimate the allowed deposit up to the mintLimit(), which violates EIP4626. Due to the fact that shares are minted at a 1:1 ratio when depositing/minting, this will always happen. Consider the following example:

assets = 1000
debt = 1000
deposit 1000 assets
rounding error
assets = 1999
debt = 2000
// revert, as it is a lossy deposit

Thus, the maximum assets that may be deposited depend on the rounding error. However, maxDeposit() always returns _mintLimit(), a much bigger value, breaking EIP4626.

Additionally, when depositing/minting, shares are minted through the TwabController, which has mint addresses restrictions, namely address(0) and the SPONSORSHIP_ADDRESS and should return 0.

And finally, it should also call maxMint() and compare it against maxDeposit(), getting the minimum of the two.

Impact

Non EIP4626 compliance in the maxDeposit() function, which always overestimates max deposits when the yield vault incurs rounding errors. And, it does not return 0 if the receiver is address(0) or the SPONSORSHIP_ADDRESS. Finally, it does not call maxMint() to check for more limits.

Code Snippet

https://github.com/sherlock-audit/2024-05-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L386

Tool used

Manual Review

Vscode

Recommendation

When the result from _tryGetTotalPreciseAssets() is equal to the _totalDebt and the buffer is depleted, the rounding error will make deposits revert, so in this case maxDeposit() should return 0. The fix is just checking for equality if (!_success || _totalAssets <= _totalDebt) return 0;. This is EIP4626 compliant because it is stated that maxDeposit() should underestimate the assets if necessary.
Additionally, when the receiver is address(0) or the SPONSORSHIP_ADDRESS, return 0.
And finally, also call previewRedeem(maxMint()) inside a try catch and compare it against maxDeposit(), picking the minimum.

Duplicate of #134

[sherlock-admin3]

1 comment(s) were left on this issue during the judging contest.

infect3d commented:

deposit can only incur rounding issue if yield buffer is depleted but if the buffer is depleted reverts on deposit are expected__ see L112-114 of PrizeVault.sol