Closed sherlock-admin4 closed 2 months ago
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
0x73696d616f
medium
PrizeVault::maxDeposit()
is not ERC4626 compliant as it does not consider rounding errors nor restricted receivers, normaxMint()
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 asaddress(0)
andSPONSORSHIP_ADDRESS
, which are not considered. And finally, it does not checkmaxMint()
.The buffer may be depleted if the underlying yield vault registers losses.
Vulnerability Detail
EIP4626
specifies thatmaxDeposit()
must return the maximum amount of assets that may be deposited and underestimate if necessary, but this is not the case.Whenever a yield incurs rounding errors,
maxDeposit()
will overestimate the allowed deposit up to themintLimit()
, 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: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 againstmaxDeposit()
, 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 isaddress(0)
or theSPONSORSHIP_ADDRESS
. Finally, it does not callmaxMint()
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 casemaxDeposit()
should return 0. The fix is just checking for equalityif (!_success || _totalAssets <= _totalDebt) return 0;
. This is EIP4626 compliant because it is stated thatmaxDeposit()
should underestimate the assets if necessary. Additionally, when the receiver isaddress(0)
or theSPONSORSHIP_ADDRESS
, return 0. And finally, also callpreviewRedeem(maxMint())
inside a try catch and compare it againstmaxDeposit()
, picking the minimum.Duplicate of #134