PoolTogether - squeaky_cactus's results

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L256-L282 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L296-L306

Vulnerability details

Impact

The DrawManager role in PrizePool is pivotal to correct operation, and it can be stolen. Sabotaging the deployment of PrizePool and all other contract deployed that hold an immutable reference to the singleton PrizePool.

Proof of Concept

On creation of the PrizePool, the constructor parameter of DrawManger is left as address(0), a valid creation configuration.

Then call setDrawManager with an unusable address e.g. address(1) https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L296-L306

  function setDrawManager(address _drawManager) external {
    if (drawManager != address(0)) {
      revert DrawManagerAlreadySet();
    }
    drawManager = _drawManager;

    emit DrawManagerSet(_drawManager);
  }  

When setDrawManager is called with the _drawManager address, as the current drawManager is address(0), the given dead address is set as the draw manager and the PrizePool is now broken, as without the draw manager there can be no draws.

The draw manager is responsible for closing draws (which also starts new draws).

Tools Used

Manual review.

Recommended Mitigation Steps

Force the drawManager to be set on creation, make it immutable and delete setDrawManager().

If the address of PrizePool is required by other contracts whose addresses are part of the constructor params, consider using CREATE2 to generate the address for PrizePool before deployment, which would allow decoupling of contract deployments.

Assessed type

Access Control

QA Report

Low Risk

L-1 Enforce offset is in the past

The constructor for the TwabController contains the following NatSpec:

https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/TwabController.sol#L136

* @dev Ensure the periods offset is in the past, otherwise underflows will occur whilst calculating periods.

The requirement of the offset being in the past is not enforced programatically.

Recommendation

Insert a require statement to check _periodOffset is in the past (probably using a custom error rather than a string): https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/TwabController.sol#L143

+   require( _periodOffset < block.timestamp, "Offest must be in the past");

L-2 NatSpec external function incomplete

The NatSpec for the external function getTierAccrualDurationInDraws() is incomplete. When NatSpec is present for function, it is best to be complete and reasonably describe the purpose of the function beyond what can be gleamed from the signature.

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

  /// @notice Returns the
  /// @return The number of draws
  function getTierAccrualDurationInDraws(uint8 _tier) external view returns (uint16) {

Recommendation

-  /// @notice Returns the
+  /// @notice Statistical estimation of time between tier payout, in draws.

L-3 Prevent transfer amount of zero

The external function withdrawClaimRewards allows a claimer to withdraw any fees they have accrued. There is a check that the _amount is not greater than the rewards accrued by the claimer, an amount of zero passes through to the transfer, where a transfer on the prizeToken would be performed.

Certain ERC20 token are know to revert on zero transfers, which ends up wasting even more gas than a pre-condition check.

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

  function withdrawClaimRewards(address _to, uint256 _amount) external {

Recommendation

Add a require check before starting any of the function internals, better to use a custom error, but the idea is:

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

+  require( _amount != 0, "Claim more than nothing");

Non-Critical

NC-1 NatSpec typographical Error

Instances: 3

Function NatSpec contains the incorrect spelling of emitted (missing a 't').

https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/TwabController.sol#L61

   * @notice Emited when a balance or delegateBalance is decreased.

https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/TwabController.sol#L75

   * @notice Emited when an Observation is recorded to the Ring Buffer.

https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/TwabController.sol#L117

   * @notice Emited when a Total Supply Observation is recorded to the Ring Buffer.

Recommendation

Emited -> Emitted

NC-2 NatSpec inconsistent whitespace

Function NatSpec contains two whitespaces separates words, when a single space is consistently being used everywhere else.

https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/TwabController.sol#L300

   * @notice Looks up the newest observation  for a user.

https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/TwabController.sol#L315

   * @notice Looks up the oldest observation  for a user.

Recommendation

-> ``

NC-3 NatSpec ambiguous parameter definition

The function NatSpec is ambiguous regarding whether _tierShares is the total for all tiers or the shares per a tier.

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

  /// @param _tierShares The number of shares allocated to prize tiers

_tierShares is shares per a tier.

Recommendation

-  /// @param _tierShares The number of shares allocated to prize tiers
+  /// @param _tierShares The number of shares allocated to each prize tier

NC-4 Redundant inline comment

The comment line above _consumeLiquidity references amount, a variable that is not the subsequent line. amount does appear later in the function.

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

    // `amount` is a snapshot of the reserve before consuming liquidity
    _consumeLiquidity(tierLiquidity, _tier, tierLiquidity.prizeSize);

Recommendation

Delete the comment.

NC-5 NatSpec description does not match function

The NatSpec of the library function fromUD60x18() has the types around the wrong way, as it convert from UD60x18 into UD34x4.

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/libraries/UD34x4.sol#L26-L29

/// @notice Casts an UD34x4 number into UD60x18.
/// @dev Requirements:
/// - x must be less than or equal to `uMAX_UD2x18`.
function fromUD60x18(UD60x18 x) pure returns (UD34x4 result) {

Recommendation

- /// @notice Casts an UD34x4 number into UD60x18.
+ /// @notice Casts an UD60x18 number into UD34x4.

NC-6 NatSpec dev comment warning function removal

The NatSpec for two library function contain a dev comment about removing them in v4, this version is v5 ...should they have been removed?

Their only references are in test files.

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/libraries/UD34x4.sol#L61-L71

/// @notice Alias for the `convert` function defined above.
/// @dev Here for backward compatibility. Will be removed in V4.
function fromUD34x4(UD34x4 x) pure returns (uint128 result) {
  result = convert(x);
}

/// @notice Alias for the `convert` function defined above.
/// @dev Here for backward compatibility. Will be removed in V4.
function toUD34x4(uint128 x) pure returns (UD34x4 result) {
  result = convert(x);
}

Recommendation

Move the functions into the test files that depend on them, as it will reduce contract gas deployment.

Any comments for the judge to contextualize your findings

Previously being a software developer by trade, I'm not deeply experienced in the open-ended style of writing needed for these analyses, which resulted in it being rather time-consuming. There is no strong guidance on what is wanted in this analysis write up either, unfortunately leading to many hours of procrastination (which I've out of the hour auditing) in deciding the level of detail and volume of content. Hopefully the analysis is inline with expectations you have.

Time ran short on this one, many of the areas I'd flagged for further investigation/testing, sadly I simply didn't get to look into seriously.

Approach taken in evaluating the codebase

Started off aiming to gain a high level understanding, by reading the protocol docs and then watching the Loom video, which did an excellent job explaining parts not entirely covered in the focs, such as the tier resizing and prize eligibility calculations.

Before cloning the repo, I checked the bot report for an overview of the potential avenue for later investigation and familiarize myself with the already public known issues in addition to catching up on the pooltogether-jul07 Discord channel.

Cloned the repo, then proceeded to step through the files line by line, verifying the NatSpec matched the logic and marking with inline comments open questions and LOW and NC issues.

Architecture recommendations

The architecture is sensible, with stateless functions being in libraries, stateful contracts have logically grouped behaviours and state. Coupling exists in some cases, but practically speaking, due to contract size limitation (i.e. Spurious Dragon) and actually getting the contract deployed on mainnet results means division must occur somewhere.

A possible addition to the architecture could be a generic mechanism to sweep accidentally received tokens from each of the contracts, but that is very much being kind to eco-system users who make mistakes.

Codebase quality analysis

The codebase is generally at a very high standard.

There's plenty of decent quality NatSpec comments, and when present they usually meaningfully describes the purpose or ranges of the function, parameter or return value rather than merely re-stating the name in a sentence adding no additional depth of meaning.

By using libraries to move the stateless (functional) behaviour out of the stateful Claimer, TWABController and PrizePool contracts, they each are effectively serving a single purpose, a single are of responsibility. THe result is easier to understand code, where usually mistakes are more easily identified.

Struts are all packed efficiently, with a number of the fields trimmed to their slimmest to minimize storage, particularly relevant for rhe TWABController, whose storage increases linearly with vault and users.

Test coverage is effectively total (over the audited Solidity files), with the only exception being branch coverage in a few of the files. The tests themselves are structured adhering to the Foundry best practices (i.e. including asserting state transitions) and also include fuzzing tests.

Although beyond of the scope of the audit, it is great to see in the package.json commands and dependencies supporting automatic code formatting and Solidity static analysis on git events (with husky and lintstaged).

The only area of friction in reviewing the code was the frequent wrapping and unwrapping between Solidity primitives and decimal representations, they may be an argument for using conversions only at the boundary functions (external and public), while keeping internal to use the decimal representations, as it should reduce the volume of wrapping ...but only a minor suggestion though (e.g. smoothing in PrizePool).

Centralization risks

While the singleton contracts of PrizePool and TWABController are points of centralization for data storage and calculations in the system, the risk is mitigated by having them as immutable, and without any ownership powers (i.e. by either contract or EOA) meaning they are not able to pause, nor upgraded. However PrizePool does have a DrawManager that does have exclusive access to certain function (closing draws and removing reserve), with the potential impact of a misbehaving DrawManager ultimately being the loss of the reserve portion or no draws occurring (locking up of deposited prize tokens).

A single point to create and track those vaults, VaultFactory is a similar central point, but being immutable, without any ownership controls, it poses no centralisation risks.

The section of the audited code to be most wary of for centralisation risk are the vault instances, as the Vault does use the ownership model, granting the owner full control over vault settings of settings the yield fees, who receives the yield fees, liquidation pair and claimer. There is no risks to the vault depositors shares from the ownership powers, nor the underlying assets assuming a sound yield vault was chosen.

Mechanism review

Based on the design resources and code provided, this was my take-away on how the mechanisms involved tie together.

There is a prize token, the abstracted method of account for all Vault shares held by users that determines the odds of winning a prize during a draw, and also is the reward unit for a winning payout.

Users interact with a Vault to deposit their assets for shares in the Vault. These shares are a claim on the underlying assets (the type deposited by users) and shares in the yield bearing vault that uses the underlying token, held by the Vault.
The Vault being a liquidation source, allows a liquidator to interact and perform conversions of prize tokens into shares of the Vault. Whenever there is an update to the shareholdings of the Vaultthe TWABController is informed.

The singleton TWABController keeps a duplicate record of Vault shareholders, with the addition of a rolling 365 count of observation entries (address level shareholder change events) and provides the ability of holders to delegate (giving their chance of winning to another address). All of these data points enable the TWABController to answer questions on what proportion an addresses holds of their Vault total shares and what proportion of their Vault is of the total contribution of prize tokens, within a finite scope of history.

The PrizePool is primarily concerned with managing draws, including whether a particular address (who holds shares in a participating Vault) has won a prize, determining prize distribution across draws and adjusting prize tiers for the next draw. When a draw is closed, a random number is provided. If any address that holds shares in a Vault that registered with the TWABController prior to the draw beginning, has a combination of constant values (tht includes the prize tier and index) that hash to a number below the random number, they win that prize. Hashing algorithms have the potential for collision, to avoid multiple claims on the same prize, a prize is claimable only once, awarded to the first claimer. Prizes come from the prize tokens contributed by a Vault (liquidation of yield from their yield bearing vault they deposit underlying assets with) and may be paid out over a number of draws, depending on a smoothing function (set on PrizePool creation), while the actual amount comes from another share model that splits the prize tokens for that draw across the tiers (amount for each tier total being equal, but the high level tiers consisting of a greater number of smaller prizes) and a portion for the reserve and canary tier. Whether the next draw has fewer or more prize tiers depends on the percentage of highest tier prizes and canary tier prizes claim (minimum of three tiers, maximum of fifteen), as if too fewer of these lowest value prizes are claim, there must be insufficient economic incentive to claim them (e.g. high network fees) and tiers are reduced to increase the size of all prizes, inversely if most are claimed then they could be split for a greater number of winners.

A Vault only allows holders to claim prizes via a claimer, which can be an EOA or a contract and there is support for batching together claims that would reduce gas fees for multiple claims.

The Claimer contract is ones of a convenience, providing a single point of entry for claiming prizes and performing claim fee calculations.

Systemic risks

The risks beyond the scope of the audited contracts include:

Liquidity risk: every vault relies on being able to exchange between their underlying asset, yield bearing vault asset and prize token asset. More specifically there are depth of liquidity (amount available to trade, affect slippage), market volatility (high volatility can deplete liquidity), external factors (rush caused by lack of confidence in a pool asset).

Smart contract risk: as the external dependencies are on other smart contracts there exists risks in their implementations (possible bugs, maybe exploits)

Statistical risk: there is always statistical uncertainty, meaning the results obtained from a statistical analysis may differ from the actual results observed. Given the modelling and monte carlo testing performed by the PooledTogether team, the remaining outstanding factor is variability. The inherent variability or randomness itself, where natural variations and random fluctuations can affect the reliability of statistical estimates and predictions.

RNG Oracle risk: besides the centralization risk (single point of censorship, manipulation, or failure), associated security vulnerabilities (Oracles being targets for hackers) and potential network connectivity problems (Oracle connection to the external source providing the RNG), there is the risk of the RNG not being adequately random enough.

Draw Manager risk: the audited code relies on an honest and reliable Draw Manager. As the Draw Manager has the ability to withdraw the entire reserve they must be trusted to do so, only when necessary for the good of the Prize Pools. The trigger to close the draw and starting a new one is only invokable by the Draw Manager, meaning the Prize Pools depend entirely on the Draw Manager call the appropriate function at the correct time intervals for everything to work as expected.

Time spent:

32 hours