Closed c4-bot-3 closed 5 months ago
raymondfam marked the issue as insufficient quality report
raymondfam marked the issue as primary issue
QA at best as that's the intended design per the readme: This buffer should never run dry during normal operating conditions and expected yield rates.
Intended behavior. The yield buffer is just to cover the rounding errors during deposits/withdrawals.
hansfriese marked the issue as unsatisfactory: Invalid
Intended behavior. The yield buffer is just to cover the rounding errors during deposits/withdrawals.
This is what is stated, but errors may be more general than just due to rounding. It is also not only about errors, but about an actual price fluctuation.
There are two problems leading to this issue.
The first is that the assumption that there are only rounding errors is incorrect. yieldVault.convertToAssets()
may have an error greater than a simple rounding error.
If the protocol was unaware of this, then this alone is an issue. But this can of course, at first, be accommodated for by just having a sufficiently large yield buffer.
The second problem is that the yield buffer is constant and immutable, but the error is not necessarily so. The simplest example is that the yieldVault.convertToAssets()
may have, say, an error of 1% (of its assets/supply). The yield buffer would then always have to be at least 1% of the PrizeVault's balance. It is obvious this is not possible with a constant yield buffer when the PrizeVault is expected to grow.
Also note that a small price drop according to yieldVault.convertToAssets()
has the same effect as an error. The yield buffer must therefore also accommodate both proportional errors and small price fluctuations (even if the trend is overall a stable accumulation of yield).
The core reason is yieldVault.convertToAssets() is approximate per EIP-4626
which is the same as #336.
It's appropriate to merge this one and #336 into one issue of "previewRedeem
should be used over convertToAssets
in functions that require precise accounting".
hansfriese removed the grade
hansfriese marked the issue as satisfactory
hansfriese marked the issue as duplicate of #336
The core reason is
yieldVault.convertToAssets() is approximate per EIP-4626
which is the same as #336. It's appropriate to merge this one and #336 into one issue of "previewRedeem
should be used overconvertToAssets
in functions that require precise accounting".
@hansfriese You are correct that both issues are related to yieldVault.convertToAssets()
, but this is a function we can do nothing about since it is an EIP-4626 function in an external contract.
Within PrizeVault the core reasons are different and need different fixes. In #336 it is that _maxYieldVaultWithdraw()
might return too much, which is entirely distinct from the the issue here in #337 which is that the yield buffer is constant (we cannot improve totalAssets()
to fix the problem).
Is anything wrong if totalAssets()
is modified like _maxYieldVaultWithdraw()
?
function totalAssets() public view returns (uint256) {
return yieldVault.previewRedeem(yieldVault.balanceOf(address(this))) + _asset.balanceOf(address(this));
}
@hansfriese, yes it is. A constant buffer cannot take into account that the error might be proportional.
I understand what you are saying.
Originally, this report says two concerns.
totalAssets
is only approximate since yieldVault.convertToAssets()
is approximate.totalAssets()
may be proportional to the value of totalAssets()
, for example if a price oracle is used in the yield vault.1 can be mitigated if totalAssets()
uses previewRedeem()
instead of convertToAssets()
like #336 suggests.
2 might be correct but it's the protocol's choice to use a constant or proportional buffer and it seems reasonable to include in the analysis report.
FYI, here is the sponsor's comment.
#337 is very similar to #336. They can be summed up as "previewRedeem should be used over convertToAssets in functions that require precise accounting"
1 can be mitigated if
totalAssets()
usespreviewRedeem()
instead ofconvertToAssets()
like #336 suggests.
In #336 this fix is to prevent _maxYieldVaultWithdraw()
from returning too much, but here the problem is when totalAssets()
returns too little for the yield buffer to handle. So this fix does not apply here, and the sponsor's comment is mistaken.
2 might be correct but it's the protocol's choice to use a constant or proportional buffer and it seems reasonable to include in the analysis report.
It is mistake to use a constant buffer since the error might be proportional. It is impossible to predict the needed size of the yield buffer in advance, since we don't know how much the vaults will grow (even if we can put a reasonable bound on the proportion of the error), so the yield buffer would have to be set with a huge margin. This was clearly not anticipated by the sponsor, who intended the yield buffer only to account for small rounding errors (seemingly not even taking into account the other errors at all). They therefore risk running into this issue in the future and wouldn't be able to increase the tiny yield buffer.
Thank you for your additional context.
It's true that convertToAssets
is approximate and can cause unexpected fluctuations to perceived redemption rates, but previewRedeem
returns the current redemption rate based on onchain conditions.
In https://github.com/code-423n4/2024-03-pooltogether-findings/issues/336 this fix is to prevent _maxYieldVaultWithdraw() from returning too much, but here the problem is when totalAssets() returns too little for the yield buffer to handle.
Since previewRedeem
returns the current redemption price of yield vault shares, if it fluctuates downward, then that implies that the yield vault has lost funds and lossy yield vault strategies are not supported by the prize vault. The prize vault has emergency fallbacks for lossy yield vaults, but is not designed to operate normally under those conditions.
As stated, the yield buffer is intended to mitigate rounding error loss, not other losses.
@trmid Keep in mind that the latent assets are regularly redeposited, so any fluctuation downward will eat directly into the yield buffer. [Random] tiny fluctuations in convertToAssets()
are not to be considered losses, but errors. A tiny yield buffer for rounding errors is inadequate to account for this, and the PrizeVault would therefore very often (~half of the time if gaussian, possibly more if systematic) think the yield vault has lost funds, when it really is just price noise.
The prize vault yield buffer strategy only aims to mitigate against rounding error loss. Any other errors or downward fluctuations in calculations no matter how big or small are not intended to be covered by the yield buffer and should be detected and evaluated prior to prize vault deployment. If any of the planned yield vault integrations have errors that fit this behaviour, it would help determine their compatibility with the prize vault.
@trmid This means you cannot integrate general EIP-4626 compliant yield vaults. This error is permitted by EIP-4626, and you would have to carefully audit each yield vault's implementation of its convertToAssets()
and make sure that it cannot drop (as an error) more than the yield buffer.
I still fear there might be future unintended consequences if this issue is not fixed.
I would like to clarify that the intended fix is to use previewRedeem
instead of convertToAssets
for internal calculation of totalAssets
. This addresses the issue with the yield vaults that use time-weighted estimates of the exchange rate in convertToAssets
, but use current block exchange rates for previewRedeem
as the spec requires. If there is still a possibility of downward fluctuation in the exchange rate of the yield vault shares, then the yield vault will be deemed lossy and should not be integrated with the prize vault.
This means you cannot integrate general EIP-4626 compliant yield vaults.
That's exactly right @d3e4; we are vetting integrations because they need to be compatible with our prize vault in addition to having up only yield vs assets trend upward. The Prize Vault supports the general 4626 spec, but it prevents deposits when it detects loss. This means that vaults whose assets trend upward would prevent deposits during a dip. Not an ideal user experience!
This was a design decision we made to keep the vault simple; but a more flexible vault could be built in the future that supports deposits during a "loss" and is able to recover.
@trmid @asselstine previewRedeem
may (per EIP-4626) return less than what is actually redeemed (it must never return more). PrizeVault may therefore always sense a certain "loss". You will have to audit the yield vault to make sure that this "loss" is always smaller than the yield buffer, and is indeed only a rounding error.
And using previewRedeem
here also potentially introduces a new issue. previewRedeem
may (per EIP-4626) revert for reasons which would cause redeem
to revert. In this sense previewRedeem
is incompatible with withdrawals and might cause an incorrect revert.
You can of course audit the yield vaults for all these aspects to prevent this issue, but I would also like to ask whether I am missing some reason why it is important that the yield buffer be immutable?
There are an infinite number of ways an 4626-compliant yield vault could be designed such that it won't be compatible with the prize vault in a way that results in normal prize generation. The 4626 standard is just that flexible.
Although it is important to design the prize vault to gracefully deal with as many yield vault designs as possible, we can never guarantee compatibility with every single one. As a result, it is more effective to design it to work with 90% of existing vaults and then identify the breaking points that can be used to evaluate if a new vault is compatible or not.
The wiggle room on previewRedeem
is a good example of such a limitation. Most vaults that exist now use the same exact output from previewRedeem
in a redeem
call, so it is simpler to declare that yield vaults that do not follow this design pattern are not compatible than it would be to try and design the prize vault to handle the unknown.
As for the revert concerns, any reversions on previewRedeem
calls can be caught by the calling function and handled however needed.
Lines of code
https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVault.sol#L874 https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVault.sol#L337
Vulnerability details
Impact
DoS of deposits.
Proof of Concept
When depositing it is required that
totalAssets() >= totalDebt()
.totalAssets()
increases overtotalDebt()
when yield is generated, but this yield is constantly withdrawn, up to leaving the yield buffer. This yield buffer, which is intended to be very small, is the only margin in this inequality. However,is only approximate since
yieldVault.convertToAssets()
is approximate, per EIP-4626. And the error oftotalAssets()
may be proportional to the value oftotalAssets()
, for example if a price oracle is used in the yield vault.As more and more is deposited into the PrizeVault,
totalAssets()
andtotalDebt()
will both increase equally, but since the yield buffer is constant their difference will stay the same. Eventually the error intotalAssets()
will exceed the yield buffer difference between them, and thus break the inequality condition. This DoS-es deposits.Recommended Mitigation Steps
The yield buffer can certainly be used to account for small drops in
totalAssets()
due to noise/errors. Allow theyieldBuffer
to be increased/decreased by the owner. (Another reason it should not be immutable is that the initial conditions used to decide its value may change further down the line, e.g. because the exchange rate increases.)Assessed type
ERC4626