PoolTogether - Jeiwan's results

Lines of code

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

Vulnerability details

Impact

Anyone can claim the yield fees and mint the extra shares to their address. The shares can then be redeemed for assets.

Proof of Concept

The Vault.mintYieldFee function allow minting the yield fee that was set out during liquidations. The function can be called by anyone and an arbitrary fee recipient address can be provided, allowing anyone to steal accumulated yield fee.

The contract defines the _yieldFeeRecipient state variable, which is the address that's expected to receive yield fees. However, the variable is not used in the Vault.mintYieldFee function.

Tools Used

Manual review

Recommended Mitigation Steps

In the Vault.mintYieldFee function, consider minting shares only to the address specified in the _yieldFeeRecipient state variable.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Anyone can revoke the chances to win for any other user. A malicious actor can use the vulnerability to exclude big depositors from the draws, thus increasing the malicious actor's chances to win prizes.

Proof of Concept

The Vault.sponsor function allows the caller to:

1. deposit funds into the yield vault (Vault.sol#L983):

   uint256 _shares = deposit(_assets, _receiver);

2. and delegate all their funds to the sponsorship address (Vault.sol#L985-L989):

   if (
       _twabController.delegateOf(address(this), _receiver) != _twabController.SPONSORSHIP_ADDRESS()
   ) {
       _twabController.sponsor(_receiver);
   }

As per the definition of the sponsorship address (TwabController.sol#L23-L24):

Allows users to revoke their chances to win by delegating to the sponsorship address.

The vulnerability is that the sponsor function can be called on any address, without the address' owner permission. A malicious actor can call the sponsor address with the amount of assets set to 0 (or 1 wei) and the receiver address set to the address of a whale (the top holder of shares). _twabController.sponsor will be called with the whale's address, and all their shares will be delegated to the sponsorship address, thus completely preventing the whale from participating in draws.

As can be seen from the _transferDelegateBalance function, when delegating to the sponsorship address, the delegated balance of the user and the total supply is decreased. This will create a new observation with the delegate balance of the whale set to 0. The TWAB of a user is the most important metric when determining if the user is a winner in a draw or not. Thus, by delegating user's balance to the sponsorship address, the TWAB of the user will be reduced, thus reducing their chances to win draws. Since the sponsor function can be called on any address, it can also be used to un-delegate all the tokens delegated to the victim, to reduce the victim's delegated balance to 0 and totally exclude them from competing for draws.

Tools Used

Manual review

Recommended Mitigation Steps

In the Vault.sponsor and Vault.sponsorWithPermit functions, consider setting the receiver parameter to msg.sender and consider disallowing the caller to specify it.

Assessed type

Other

Lines of code

https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/TwabController.sol#L596-L599 https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/TwabController.sol#L622-L626 https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/TwabController.sol#L633-L637

Vulnerability details

Impact

Users cannot re-delegate their tokens after delegating them to the zero address. Such users will forever burn their delegated tokens and won't be allowed to participate in draws, since their delegateBalance will always be 0.

Proof of Concept

The TwabController.delegate function lets users delegate their tokens to any address. One of the option is to delegate to the zero address, in case a user wants to opt out of participating in draws: when delegating to the zero address, the user's balance of delegated tokens is decreased and the total supply of delegated tokens is also decreased. The function also allows delegating from the zero address: the total supply is expected to be increased. The function is deliberately designed to handle the cases when either "to delegate" or "from delegate" addresses is zero, thus delegating to the zero address seems like a valid use case.

However, the "from delegate" address can never be the zero address: when taking the current delegate, _delegateOf is called, which returns the user's address if the delegate address is the zero address. Thus, after delegating to the zero address, the "from delegate" address will be set to the user's address, leading to the following outcomes:

1. if the user is trying to un-delegate from the zero address and delegate to self, this line will cause a revert;
2. if the user is trying to re-delegate to another address, this line will revert because the user won't have enough delegateBalance to delegate (since the _fromDelegate is set to the user's address, not to the zero address).

Thus, the delegated tokens will be lost forever, which will reduce the user's TWAB and will prevent them from participating in draws.

If at this point the user receives or mints more tokens, their delegateBalance will be increased as expected. However, it will be smaller than their token balance, and they won't be able to delegate at all since, when delegating, it's the token balance that's delegated, not delegateBalance.

Tools Used

Manual review

Recommended Mitigation Steps

The simplest solutions seems disallowing delegating to the zero address: since delegating to the sponsorship address is used to opt out of draws, allowing transfers to the zero address seems unnecessary. Alternatively, delegating to the zero address could mean irrevocable burning of delegateBalance. In this case, the code needs to be updated to not allow delegating from the zero address, and the users need to be able to delegate newly minted or received tokens.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The prize tokens deposited directly to the prize pool can be contributed to the prize pool by anyone. This frees liquidators from providing prize tokens, while they still receive vault shares. Another impact is the reserves of the prize pool become inflated: the actual amounts of tokens in reserves will be smaller. This can break claiming of prizes when there's not enough tokens in the pool and the reserves are used to pay the prize.

Proof of Concept

The PrizePool.increaseReserve function allows anyone to deposit prize tokens directly to the reserves of the prize pool. However, these tokens are not accounted in the balance of the prize pool: the _accountBalance function doesn't count the reserves. As a result, the tokens deposited directly to the reserves can also be contributed to the prize pool: the contributePrizeTokens function computes the amount of tokens to contribute as the difference between the current contract's balance and the amount of accounted balances:

uint256 _deltaBalance = prizeToken.balanceOf(address(this)) - _accountedBalance();
if (_deltaBalance < _amount) {
    revert ContributionGTDeltaBalance(_amount, _deltaBalance);
}

What this means is that, after the PrizePool.increaseReserve function is called and some amount of tokens is deposited, the Vault.liquidate function can be called by anyone to contribute the tokens, basically making the liquidation free for the caller. The user who deposited the tokens to the reserve loses them.

Tools Used

Manual review

Recommended Mitigation Steps

There doesn't seem to be a quick solution. It's clear that the reserves shouldn't be accounted because they're mainly made from the contributed tokens (after shrinking unused tiers). Thus, it seems that donated reserves should be counted in a separate state variable and the state variable should be accounted in _accountedBalance.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Any winner can intentionally or not intentionally block claiming of prized for themselves and other winners. The caller of claimPrizes would have to exclude the winner from the list to claim prizes for other winners.

Proof of Concept

The Vault.claimPrizes function allows to claim prizes for multiple winners. The function calls _claimPrize for each winner. In _claimPrize, user-specified hooks are called to determine the prize recipient address and perform post-claim operations (Vault.sol#L1052-L1075):

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

The hooks are set by users and can point at any address that implements the IvaultHooks interface. However, the hooks can fail, but the _claimPrize function won't detect that and will revert the entire transaction. Even if a user doesn't have hooks set, claiming a prize for them may fail because of a failing hook of another user.

Tools Used

Manual review

Recommended Mitigation Steps

For the beforeClaimPrize hook, consider wrapping the call in try/catch and skipping the claiming of the prize if the call fails. For the afterClaimPrize hook, consider wrapping the call in try/catch and silently skipping failed calls (since prize claiming cannot be reverted after it was done).

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

A malicious vault can be deployed via the official VaultFactory contract. Users may lose funds while interacting with such vaults.

Proof of Concept

The VaultFactory.deployVault function allows deploying Vault contracts. The factory contract maintains a mapping to verify that a vault has been deployed via the factory–this allows users to check the authenticity of a vault to ensure the that implementation of a vault is authentic (i.e. not altered/malicious).

However, the business logic of vaults is split into multiple contracts: Vault, TwabController, and PrizePool. TwabController tracks the historical balances of users to determine their chances of winning prizes. PrizePool runs regular draws and distributes prizes among winners. Thus, it's critical that, in every authentic Vault contract, the implementations of TwabController and PrizePool are also authentic. Otherwise, a malicious actor could deploy an authentic vault via the official VaultFactory and provide malicious TwabController and PrizePool contracts, which, for example, incorrectly determine user balances, favors some specific addresses when determining winners, or steals the prize token. In the current implementation, users are be able to check the authenticity of a vault contract, but not the authenticity of the TwabController and the PrizePool contracts a vault integrates with.

Tools Used

Manual review

Recommended Mitigation Steps

Consider implementing a TwabController and a PrizePool factory contracts. In the contracts, consider tracking the addresses of deployed TwabController and PrizePool contracts. In the VaultFactory.deployVault function, consider checking that the passed _twabController and _prizePool address were deployed via the respective factory contracts.

Assessed type

Other

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/libraries/DrawAccumulatorLib.sol#L190-L192

Vulnerability details

Impact

Calling PrizePool.getTotalContributedBetween or PrizePool.getContributedBetween can revert when a correct _endDrawIdInclusive is provided. Third-party integration and off-chain analysis tools won't be able to obtain the total contributed amounts or the contributed amounts of a vault.

Proof of Concept

The DrawAccumulatorLib.getDisbursedBetween function limits the _endDrawId argument: it cannot be more than draw before the last observed draw. To validate the value of the argument, the function calls readDrawIds obtain the oldest and the newest observations (the newest is stored in drawIds.second) and then compares the draw ID of the newest observation with the value of _endDrawId (DrawAccumulatorLib.sol#L190-L192):

if (_endDrawId < drawIds.second - 1) {
    revert InvalidDisbursedEndDrawId(_endDrawId);
}

However, not every draw has an observation, and a previous observation can have a draw ID that's smaller than the newest observation's draw ID - 1. Observations are created in the DrawAccumulator.add function, which is only called when a new contribution is made to the prize pool (PrizePool.sol#L311-L327). Thus, the following situation is possible:

1. the draw ID of the newest observation is 10 (a contribution was made in draw 10);
2. the draw ID of the observation before the newest one is 8 (a contribution was made in draw 8).

I.e., there were no contributions in draw 9 and no observations were created for the draw. In this case, when calling DrawAccumulatorLib.getDisbursedBetween, a correct value of _endDrawId would be 8 (the ID of the observation before the newest observation), however the function won't allow values smaller than 9, even though there's no observation for draw 9.

Tools Used

Manual review

Recommended Mitigation Steps

When validating the value of _endDrawId in DrawAccumulatorLib.getDisbursedBetween, consider comparing it to the actual draw ID of the observation before the newest one, e.g. using:

_accumulator.drawRingBuffer[
    RingBufferLib.offset(indexes.second, 1, ringBufferInfo.cardinality)
];

To obtain the draw ID of the observation before the newest one.

Assessed type

Other

The audited PoolTogether protocol can be described as an advanced ERC4626 vault with extra mechanics and features. The main Vault contract is compatible with the ERC4626 standard, however it adds multiple unique and tricky features.

Foremost, the Vault sets the share-to-asset exchange rate to 1 or less than 1. This means that a deposited amount of assets cannot be later withdrawn as a bigger amount. During withdrawing, users receive the exact same amount of assets as they deposited, besides the situations when the exchange rate goes below 1 (i.e. when the vault becomes undercollateralized).

Second, the yield generated and collected by the vault is not distributed among the depositors of the vault: it can only be liquidated. Liquidating yield means swapping it for an amount of prize tokens and contributing the prize tokens into the prize pool. The liquidator converts the yield into prize tokens and deposits them to the prize pool, while receiving an equal amount of shares as reward.

Another big change is that the vault contract doesn't mint and manage shares–this functionality is delegated to the TwabController contract.

The PrizePool contract is another core contract of the protocol. The contract receives the yield generated by the vault and converted to prize tokens. The prize tokens are then distributed among the depositors of the vault via daily draws. During each draw, the total amount of prize tokens is split into multiple tiers, with each tier having a unique odds of winning, a unique prize size, and a unique number of prizes. A draw is triggered by the draw manager, which is a restricted role that provides a winning random number for each draw. The number randomizes the winners of a draw, however the main factor that contributes to the chances of a depositor to win is the historical vault shares balance of the depositor.

The TwabController contract glues the vault and the prize pool. Its main purpose is to store the vault share balances of depositors and keep the record of their historical balances. The latter is implemented via TWAB, Time-Weighted Average Balances, a mechanism that's identical to Uniswap's Time-Weighted Average Price oracle.

The final core contract is Claimer. The contract allows anyone to claim prizes for all winners in a draw for a fee. The size of the fee is determined via a Linear Variable Rate Gradual Dutch Auction, which was invented by Paradigm. The auction increases the fee as more time passes since the time the draw was closed at. The claimers thus get a higher reward the later they claim prizes.

The codebase as a whole is a solid engineering construction that relies on the standards and battle-tested algorithms, while providing novelty in how they're used together. The vault implements the ERC4626 standard, while significantly modifying it. The prize pool and the TWAB controller contracts implement the battle-tested historical oracle algorithm invented and used by Uniswap. The claimer contract cleverly uses a Variable Rate Gradual Dutch Auction to incentivize prize payouts for winners of draws.

The protocol doesn't expose significant centralization risks. All the audited contracts are permissionless and don't implement restricted functions. The only exception is the PrizePool.closeDraw function that can only be called by the draw manager. To avoid draw results manipulation, a centralized actor is required to provide a random number.

The audit was performed using the standard technique of reading the entire codebase line-by-line, validating each line against the whitepaper and using the common sense, as well as bird's eye view reviewing of the codebase with the goal of finding discrepancies in the integrations of the contracts.

Time spent:

30 hours