PoolTogether - DadeKuma's results

Lines of code

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

Vulnerability details

Impact

Prize claims can be executed by external actors to get some fees as a reward. These fees are dynamic and are limited in range, and some hooks will be executed before and after the claim.

As the fees are deducted from the prizeTotal received by the winner, they could be incentivized to create an afterClaimPrize hook that reverts the transaction when fees are too high, to maximize the total profit.

As the transaction is executed by the claimer, the winner doesn't lose anything by doing so, and at most, they can wait until the last moment before their prize expires to disable the hook.

This results in a very bad experience for claimers as their transaction will revert and they will waste gas, possibly eroding trust in the claiming system.

Proof of Concept

claimPrize is executed between two hooks:

if (hooks.useBeforeClaimPrize) {
  recipient = hooks.implementation.beforeClaimPrize(_winner, _tier, _prizeIndex);
} else {
  recipient = _winner;
}

uint prizeTotal = _prizePool.claimPrize(
  _winner,
  _tier,
  _prizeIndex,
  recipient,
  _fee,
  _feeRecipient
);

if (hooks.useAfterClaimPrize) {
  hooks.implementation.afterClaimPrize(
    _winner,
    _tier,
    _prizeIndex,
    prizeTotal - _fee,
    recipient
  );
}

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

A vault can check the fee paid for this claim by calculating the delta between prizeTotal and prizeTotal - _fee, as they are part of the same transaction.

If this delta is too high, the vault might revert the transaction inside the afterClaimPrize hook until the market conditions are better.

Tools Used

Manual review

Recommended Mitigation Steps

Consider removing the afterClaimPrize hook to avoid this scenario.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

When users deposit their assets, the resulting shares might be zero due to rounding.

If this happens the transaction will not revert and no shares will be minted, but users will still lose their funds and gain nothing.

Proof of Concept

There are no checks for zero shares minted in deposit, nor in _deposit:

function deposit(uint256 _assets, address _receiver) public virtual override returns (uint256) {
    if (_assets > maxDeposit(_receiver))
      revert DepositMoreThanMax(_receiver, _assets, maxDeposit(_receiver));

    uint256 _shares = _convertToShares(_assets, Math.Rounding.Down);
    _deposit(msg.sender, _receiver, _assets, _shares);

    return _shares;
}

The issue can be replicated with the following steps:

1. Bob increased his allowance to the vault, and calls the deposit function.
2. The exchange rate is currently very high, so this line will round to zero:

_assets.mulDiv(_assetUnit, _exchangeRate, _rounding)

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

3. Resulting shares are zero as _rounding is down (this is correct and EIP-4626 compliant), but then the transaction doesn't revert.

4. Bob transfered his _assets but he gains zero shares, losing his funds.

Tools Used

Manual review

Recommended Mitigation Steps

Consider reverting the transaction if a deposit would result in zero shares minted.

Assessed type

Invalid Validation

PoolTogether Analysis

Summary

01 High-level architecture

02 Analysis of the codebase

• The separation of concerns between Vaults, Prize Pool, TWAB, and other components allows for modularity and an easier code review.

• The codebase quality is extremely high, with extensive comments that explain every step in detail.

• Test quality is extremely high, with high coverage and usage of advanced testing techniques (e.g. fuzzing).

03 Architecture feedback

• The architecture of PoolTogether V5 is well thought out and designed for flexibility. It allows for the inclusion of any ERC20-compliant asset and the customization of any ERC-4626 Vault.

• Some of the new features include a novel hand-off approach where users don't need to manually claim their prizes as there are incentives for third parties to do so.

• The use of a weighted average to smooth contributions over time provides a fair distribution of prizes and encourages participation from Vaults with a periodic yield generation.

• The architecture of the TWAB Controller appears well-designed to fulfill its purpose of accurately tracking and calculating time-weighted average balances.

• The utilization of Observations to record relevant data points and the approach of overwriting Observations within periods for gas efficiency are clever design choices. This is ensured by aligning periods with draws, as the TWAB Controller ensures accurate tracking of balances during draw periods.

04 Centralization risks

• The PoolTogether V5 Protocol aims to be autonomous and permissionless, reducing centralization risks.

• It's important to evaluate the dependencies and external services used, such as the RNG service to extract the winners, to ensure they are decentralized and reliable.

05 Systemic risks

• Claimers should always have a high incentive to claim winner prizes as the new V5 version is marketed as a hand-off approach. Failure to do so might result in an expired prize, as (non-claimer) users don't expect to claim manually.

• The random number generation process for Draw completion is critical, and reliance on external RNG services introduces risks if those services are compromised or manipulated.

• Periods are not considered finalized until the full period length has passed. Balances and average balances within a period that has not ended may still be subject to changes due to Observations being overwritten. This can lead to inaccurate balances and average balances for that period.

• Historic balances and average balances are only guaranteed to be accurate within specific conditions. Historic balances are accurate for timestamps falling between the newest Observation recorded before the end of a finalized period and the end time of that period. Average balances across time ranges are accurate if temporary Observations can be accurately recreated on both ends of the range and if time-weighted balances have been captured throughout the range.

• Loss of historic information: due to the compression of information and overwriting of Observations, specific timestamps at which balances were held throughout a period may be lost, so external actors should be aware of it.

Time spent:

12 hours