The yield buffer acts as a constant margin against a proportional lossy deposit error

[c4-bot-3]

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 over totalDebt() 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,

function totalAssets() public view returns (uint256) {
    return yieldVault.convertToAssets(yieldVault.balanceOf(address(this))) + _asset.balanceOf(address(this));
}

is only approximate since yieldVault.convertToAssets() is approximate, per EIP-4626. And the error of totalAssets() may be proportional to the value of totalAssets(), for example if a price oracle is used in the yield vault.

As more and more is deposited into the PrizeVault, totalAssets() and totalDebt() will both increase equally, but since the yield buffer is constant their difference will stay the same. Eventually the error in totalAssets() 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 the yieldBuffer 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

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

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.

[hansfriese]

Intended behavior. The yield buffer is just to cover the rounding errors during deposits/withdrawals.

[c4-judge]

hansfriese marked the issue as unsatisfactory:
Invalid

[d3e4]

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).

[hansfriese]

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".

[c4-judge]

hansfriese removed the grade

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as duplicate of #336

[d3e4]

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 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).

[hansfriese]

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));
    }

[d3e4]

@hansfriese, yes it is. A constant buffer cannot take into account that the error might be proportional.

[hansfriese]

I understand what you are saying.

Originally, this report says two concerns.

1. totalAssets is only approximate since yieldVault.convertToAssets() is approximate.
2. The error of 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"

[d3e4]

1 can be mitigated if totalAssets() uses previewRedeem() instead of convertToAssets() 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.

[trmid]

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 #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.

[d3e4]

@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.

[trmid]

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.

[d3e4]

@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.

[trmid]

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.

[asselstine]

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.

[d3e4]

@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?

[trmid]

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.

[trmid]

mitigation: GenerationSoftware/pt-v5-vault#96 & GenerationSoftware/pt-v5-vault#97