PoolTogether - 0xWaitress's results

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L567

Vulnerability details

Impact

liquidate at Vault wrongly uses liquidatable asset to compare with the _amountout which is shareToMint leading to potentially over-mint and under-collateralization

liquidate is assumed to be called by liquidator, with _amountOut as the expected shares to mint for _account. This is verified by the line 584 _mint(_account, _amountOut).

In the context, the _amountOut is checked against the _liquidableYield to ensure the vault remains collateralized after the execution.

uint256 _liquidableYield = _liquidatableBalanceOf(_tokenOut);
    if (_amountOut > _liquidableYield)
      revert LiquidationAmountOutGTYield(_amountOut, _liquidableYield);

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L567

However when we dive into the function _liquidatableBalanceOf(_tokenOut), the function actually calculate the excess amount of assets, but not the excess amount of shares as a result of excess asset

_liquidatableBalanceOf, accounting the yieldFee from the result of availableYieldBalance()

  function _liquidatableBalanceOf(address _token) internal view returns (uint256) {
    if (_token != address(this)) revert LiquidationTokenOutNotVaultShare(_token, address(this));

    uint256 _availableYield = availableYieldBalance();

    unchecked {
      return _availableYield -= _availableYieldFeeBalance(_availableYield);
    }
  }

As u can see, availableYieldBalance returns the totalAsset, minus the asset equivalent from the current totalShare.

  function availableYieldBalance() public view returns (uint256) {
    uint256 _assets = _totalAssets();
    uint256 _sharesToAssets = _convertToAssets(_totalShares(), Math.Rounding.Down);

    return _sharesToAssets > _assets ? 0 : _assets - _sharesToAssets;
  }

If the share is worth more than 1 unit of underlying, which is typically the case for yield-bearing vault, The impact is that an over-mint happens. At worst, then the Vault would be rendered under-collateralized immediately after an liquidate call.

Example:

1. Vault A has share 1000, with underlying Asset 2000.
2. After time T, Vault accrues 20 yield -> totalUnderlying 2020.
3. Liquidator calls liquidate with _amountout as 20.
4. Assume a yieldFeePercentage of 0 for simplicity, availableYieldBalance() returns 20 since it returns the excess amount of assets, based on number of shares, which is the figure at step1.
5. The condition check at if (_amountOut > _liquidableYield) revert LiquidationAmountOutGTYield(_amountOut, _liquidableYield); would pass, thus 20 shares are minted to the _account.
6. With the original exchange rate (1 share to 2 underlying token). The vault is under-collateralization now.

Proof of Concept

NA

Tools Used

Recommended Mitigation Steps

Consider converting the _amountout into share before minting, if the input is expected to be compared against asset. Alternatively convert the excess asset into excess share before comparing against the input (_amountout).

converting the _amountout into share

...
+++ uint256 _shareOut = _convertToShares(_amountOut, Math.Rounding.Down);
_mint(_account, _shareOut);

In the main contest page, vault security is highlighted to be the main concern:

The Vaults hold user funds, so it's critical that there can be no loss of funds. The liquidator is able to liquidate the accrued yield on the vault, so it's a second possible ingress point for an attacker. We want to make sure that the liquidator can't access more than the yield, and that users can't access each other's principal.

With due respect, and to harden the invaraiant, i would also recommend to move the modifier _requireVaultCollateralized() to the end of the liquidate function to ensure post-liquidate collaterialization of the vault

Assessed type

Math

[L-1] At PrizePool, _isWinner passes a uint16 of _drawDuration to _getVaultUserBalanceAndTotalSupplyTwab() which takes a uint256 of _drawDuration.

  function _isWinner(
...
    uint16 _drawDuration
  )
...
    (uint256 _userTwab, uint256 _vaultTwabTotalSupply) = _getVaultUserBalanceAndTotalSupplyTwab(
      _vault,
      _user,
      _drawDuration
    );

_getVaultUserBalanceAndTotalSupplyTwab

  function _getVaultUserBalanceAndTotalSupplyTwab(
    address _vault,
    address _user,
    uint256 _drawDuration
  )

Since the maximum value of _drawDuration is only 366 so the severity is low.

Recommendation Please align data type for consistency.

[L-2] estimatePrizeFrequencyInDraws returns uin256 but casted to uint16 at getTierAccrualDurationInDraws.

  function getTierAccrualDurationInDraws(uint8 _tier) external view returns (uint16) {
    return
      uint16(TierCalculationLib.estimatePrizeFrequencyInDraws(_tierOdds(_tier, numberOfTiers)));
  }

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L551-L554

  function estimatePrizeFrequencyInDraws(SD59x18 _tierOdds) internal pure returns (uint256) {
    return uint256(fromSD59x18(sd(1e18).div(_tierOdds).ceil()));
  }

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/libraries/TierCalculationLib.sol#L32-L34

[QA-1] _tierOdds of tier 0 at every tier_num is the same, thus can be reduced to 1 variable

all tier odds at tier 0, for example TIER_ODDS_0_3 and TIER_ODDS_0_4 and so on so forth, has the same value 2739726027397260, it's advisable to reduce the number of variable to TIER_ODDS_0 and return this for all _tierOdds(0, x).

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/abstract/TieredLiquidityDistributor.sol#L807

Recommendation

function _tierOdds() {
if (_tier == 0) return TIER_ODDS_0;
...
...

[QA-2] this line in Vault can be re-written for better clarity

if (_withdrawableAssets > _totalSupplyToAssets) {
      _withdrawableAssets = _withdrawableAssets - (_withdrawableAssets - _totalSupplyToAssets);
    }

is no different from making _withdrawableAssets as _totalSupplyToAssets

if (_withdrawableAssets > _totalSupplyToAssets) {
      _withdrawableAssets = _totalSupplyToAssets;
    }

I am submitting a report since i dont think this is an issue but would find it worth a second thought in terms of design:

Summary:

the smoothing curve combined with calculation of _getVaultPortion may create confusion on the initial lower-than-expected probability of winning (In the initial stage of joining, vault does not stand as much chance winning big prizes as vaults that join earlier with same contribution)

The vaultPortion (and its user collectively), is calculated by its contribution over the entire contribution for a particular draw. This vaultPortion represents the entitled expectedAmount and is translated into the probability of winning.

When a vault makes some contribution, the amount is split into contribution for multiple upcoming draws using the smoothing factor and formula. Generally speaking the contribution is bigger in the beginning and decays gradually with the passage of time.

https://dev.pooltogether.com/protocol/next/design/prize-pool#yield-smoothing-example

During the calculation of a particular tier prize, based on the expected occurrence, or the (expected) duration the prize, it would trace back up to the duration to find the cumulative contribution of a vault relative to the cumulative overall contribution to compute the winning probability.

a vault would basically have negligible change of winning grand prize at the beginning. Since its contribution for the entire duration is 0, except after its contribution which is recent. This is fair, since its TWAB contribution would then gradually move up with the passage of time, just like any other contributors has experienced. However, this delay due to smoothing factor + retrospective calculation of winning probability is not specified (explicitly), and might not be expected by contributor.

Recommendation

Consider evenly distribute the contribution of a newly joined vault over a period of X draws (where X covers some 1 prize cycle, 365 in this set up).

The smoothing factor is nice, as it gets the new comer started winning smaller prizes as soon as it participated, however it seems to create an infinite residuals and potentially leave accounting problems for the future. It is advised to format the vesting/distribution schedule on a rigid percentage similar to the tier probability, such that the contribution runs completely at a finite number of draws.

Time spent:

8 hours