MiloTruck - Distributing liquidity based on the last `grandPrizePeriodDraws` days post-shutdown is problematic

[sherlock-admin3]

MiloTruck

medium

Distributing liquidity based on the last grandPrizePeriodDraws days post-shutdown is problematic

Summary

After a prize pool shuts down, its funds are distributed based on draws before it shut down. This could cause a loss of funds for prize pools that contribute new liquidity to the pool post-shutdown.

Vulnerability Detail

After a vault is shutdown, all existing and new liquidity is distributed based on a user and vault's time-weighted prize contribution during the last grandPrizePeriodDraws before shutdown:

PrizePool.sol#L873-L895

  function computeShutdownPortion(address _vault, address _account) public view returns (ShutdownPortion memory) {
    uint24 drawIdPriorToShutdown = getShutdownDrawId() - 1;
    uint24 startDrawIdInclusive = computeRangeStartDrawIdInclusive(drawIdPriorToShutdown, grandPrizePeriodDraws);

    (uint256 vaultContrib, uint256 totalContrib) = _getVaultShares(
      _vault,
      startDrawIdInclusive,
      drawIdPriorToShutdown
    );

    (uint256 _userTwab, uint256 _vaultTwabTotalSupply) = getVaultUserBalanceAndTotalSupplyTwab(
      _vault,
      _account,
      startDrawIdInclusive,
      drawIdPriorToShutdown
    );

    if (_vaultTwabTotalSupply == 0) {
      return ShutdownPortion(0, 0);
    }

    return ShutdownPortion(vaultContrib * _userTwab, totalContrib * _vaultTwabTotalSupply);
  }

As seen from above, the draw ID used as the ending draw is getShutdownDrawId() - 1, which is the draw before the pool shut down.

However, this method of allocating liquidity is problematic when considering that new liquidity can be contributed to the prize pool, even after it is shutdown.

Firstly, if a vault shuts down before any liquidity is contributed through contributePrizeTokens(), all future liquidity contributed to the prize pool will be permanently stuck. For example:

• A prize pool and vault are newly deployed.
• Assume that drawTimeout = 1, which means the prize pool shuts down after one draw is missed.
• In the first draw period, the prize pool does not generate any yield, and therefore, it does not contribute any liquidity.
• The first draw is missed as there is no prize liquidity, so the pool shuts down.
• In the second draw period, the prize pool generates some yield and calls contributePrizeTokens() to add it to the prize pool.
• When users of the vault attempt to withdraw the yield from the prize pool through withdrawShutdownBalance():
  • In computeShutdownPortion(), totalContrib = 0 as no liquidity was contributed before the pool shut down.
  • In shutdownBalanceOf(), shutdownPortion.denominator = 0, so the withdrawable balance for all users is 0.
• As such, since all users cannot withdraw any balance from the prize pool, the new prize liquidity is effectively stuck forever.

Secondly, prize pools that did not contribute any liquidity before the pool shut down, but start doing so post-shutdown will always lose their yield. For example:

• Assume two vaults are deployed, called vault A and B.
• Before the pool shuts down:
  • Vault A calls contributePrizeTokens() and contributes some prize liquidity.
  • Vault B does not generate any yield, so it does not contribute any prize liquidity.
• After the pool shuts down, vault B starts generating yield and consistently calls contributePrizeTokens().
• However, since vault B did not contribute any liquidity before the pool shut down, all contributions from vault B are only withdrawable by users of vault A.
• This causes a loss of yield for users in vault B.

Note that since vaults call contributePrizeTokens() without checking if the pool has shut down and the liquidation of yield is performed automatically by liquidation bots, it is not unlikely for a vault to contribute liquidity to a pool after it is shut down.

Impact

When a vault contributes to the prize pool post-shutdown but did not do so before shutdown, all users of the vault lose their yield, causing a loss of funds.

In the case where the prize pool did not receive any liquidity before shutdown, all funds contributed post-shutdown will be permanently stuck in the pool, causing a loss of yield for all users.

Code Snippet

Tool used

Manual Review

Recommendation

Consider a different method of allocating funds in the prize pool after shutdown. For example, the ending draw in computeShutdownPortion() could include the latest draw.

[trmid]

The prize pool can enter shutdown mode for two reasons:

1. the draw timeout has been reached due to the termination of the RNG service
2. the TWAB timestamps have reached their max limit

The second is guaranteed to happen at the end of life for a TWAB controller and was a major consideration in the design of the shutdown logic. With this consideration in mind, the computed shutdown balances cannot look at TWAB past the shutdown timestamp, and therefore must use past TWAB to allocate balances on a best-effort basis. While the shutdown allocations can be unfair to edge cases, it provides simple rules for distributions that consider the historical weight of a vault's contributions while remaining within the limitations of a shutdown environment.

The shutdown logic is not designed to support contributions from new vaults that have never contributed before. It's a bit like trying to open an account at an institution that just declared bankruptcy. While it may not be ideal for everyone, prize vault owners that don't like the shutdown distribution can change the liquidation strategy on the vault to redirect any yield to a different distribution system.

[nevillehuang]

Since the following unfairness to edge cases wasn’t mentioned as a design choice, I will consider this still valid medium given liquidations can occur at any point if time.

While the shutdown allocations can be unfair to edge cases, it provides simple rules for distributions that consider the historical weight of a vault's contributions while remaining within the limitations of a shutdown environment

[10xhash]

Since the following unfairness to edge cases wasn’t mentioned as a design choice, I will consider this still valid medium given liquidations can occur at any point if time.

While the shutdown allocations can be unfair to edge cases, it provides simple rules for distributions that consider the historical weight of a vault's contributions while remaining within the limitations of a shutdown environment

Escalate

It is up to the protocol to determine how the yield after a vault shutdown should be distributed and the chosen way is completely valid (they have explicitly chosen to not consider the contributions after shutdown, instead rewarding it to earlier user's). The scenario's mentioned above doesn't lead to any unforeseen behavior in the system

[sherlock-admin3]

Since the following unfairness to edge cases wasn’t mentioned as a design choice, I will consider this still valid medium given liquidations can occur at any point if time.

While the shutdown allocations can be unfair to edge cases, it provides simple rules for distributions that consider the historical weight of a vault's contributions while remaining within the limitations of a shutdown environment

Escalate

It is up to the protocol to determine how the yield after a vault shutdown should be distributed and the chosen way is completely valid (they have explicitly chosen to not consider the contributions after shutdown, instead rewarding it to earlier user's). The scenario's mentioned above doesn't lead to any unforeseen behavior in the system

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[nevillehuang]

From my understanding, liquidations can occur at any point of time and contributions to prize pool can occur even after post shutdown. Since there is no information revolving this being protocol decision, I made the choice to validate it.

[0x73696d616f]

I believe this issue is too unlikely to be medium, but leave this interpretation to the HoJ.

The second scenario mentioned is a design choice, taking a snapshot of the liquidity and then distributing it. This does not need any documentation because the code is doing exactly what is mentioned.

The first scenario, which is the problematic one, only happens when no one has contributed during the first drawTimeout draws, which is very, very unlikely (at least a few days). Also, why will users contribute to the vault if they had 0 contributions pre shutdown? This reduces the likelihood further.

[WangSecurity]

I have a question about what does the vault do after the shutdown? And want to clarify "didn't generate any yield before shutdown, but starts after the shutdown" means that there were depositors, but just no yield?

So if there are depositors in the pool, no yield is generated, the pool is shutdown, what is the expected functionality of the pool now? Is it expected to generate any yield or it's now a non-functioning contract?

[trmid]

I have a question about what does the vault do after the shutdown? And want to clarify "didn't generate any yield before shutdown, but starts after the shutdown" means that there were depositors, but just no yield?

So if there are depositors in the pool, no yield is generated, the pool is shutdown, what is the expected functionality of the pool now? Is it expected to generate any yield or it's now a non-functioning contract?

The shutdown logic was added to the prize pool as a way to do something with the remaining liquidity in the case that the pool becomes non-operational. The prize pool should be viewed as non-operational when shutdown and the shutdown logic defines the rules for distribution of the remaining funds. Any new contributions are not recommended.

[WangSecurity]

When a vault contributes to the prize pool post-shutdown but did not do so before shutdown, all users of the vault lose their yield, causing a loss of funds.

Therefore, in the case above, there should be no yield and it's intended, hence, there's no loss of funds (yield in that case)?

[WangSecurity]

Since there should be no yield after the shutdown and the vault acts as non-operational, I believe it would be a user mistake to deposit into it. Hence, I believe this is low severity.

Planning to accept the escalation and invalidate the issue.

[WangSecurity]

Result: Invalid Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 10xhash: accepted