PoolTogether - dirk_y's results

Lines of code

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

Vulnerability details

Impact

Every vault has a yield fee recipient that should receive the fee amount when yield is captured. However, at the moment the vault method mintYieldFee is unprotected and allows anyone to pass in an arbitrary recipient address. The result is that any user can steal fees that should belong to the yield fee recipient address (which is controlled by the vault owner).

Proof of Concept

This vulnerability is relatively simple to demonstrate:

function mintYieldFee(uint256 _shares, address _recipient) external {
    _requireVaultCollateralized();
    if (_shares > _yieldFeeTotalSupply) revert YieldFeeGTAvailable(_shares, _yieldFeeTotalSupply);

    _yieldFeeTotalSupply -= _shares;
    _mint(_recipient, _shares);

    emit MintYieldFee(msg.sender, _recipient, _shares);
  }

As can be seen from the above method, there is no access control for this method and the caller is able to specify an arbitrary _recipient address. The end result is that anyone can mint the yield fee to an arbitrary address.

Tools Used

Manual Review

Recommended Mitigation Steps

The mintYieldFee method should be changed to the below:

diff --git a/src/Vault.sol b/src/Vault.sol
index e92ecaf..413c584 100644
--- a/src/Vault.sol
+++ b/src/Vault.sol
@@ -391,12 +391,12 @@ contract Vault is ERC4626, ERC20Permit, ILiquidationSource, Ownable {
    * @param _shares Amount of shares to mint
    * @param _recipient Address of the yield fee recipient
    */
-  function mintYieldFee(uint256 _shares, address _recipient) external {
+  function mintYieldFee(uint256 _shares) external {
     _requireVaultCollateralized();
     if (_shares > _yieldFeeTotalSupply) revert YieldFeeGTAvailable(_shares, _yieldFeeTotalSupply);
 
     _yieldFeeTotalSupply -= _shares;
-    _mint(_recipient, _shares);
+    _mint(_yieldFeeRecipient, _shares);
 
     emit MintYieldFee(msg.sender, _recipient, _shares);
   }

Assessed type

Invalid Validation

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#L648-L664

Vulnerability details

Impact

The default delegate value for a user is address(0) which maps to the user delegating to themselves. If a user had delegated to another address and wanted to reset their delegated balance back to themselves they would lose all of their funds contributed to the vault.

Proof of Concept

As mentioned above, the default behaviour for a user is that they delegate their balance to themselves, where the actual default value in storage is the 0 address:

function _delegateOf(address _vault, address _user) internal view returns (address) {
    address _userDelegate;

    if (_user != address(0)) {
      _userDelegate = delegates[_vault][_user];

      // If the user has not delegated, then the user is the delegate
      if (_userDelegate == address(0)) {
        _userDelegate = _user;
      }
    }

    return _userDelegate;
  }

When a user wants to delegate their balance, they call delegate in TwabController.sol and specify which vault they want to delegate the balance of, and to which address they want to delegate to. This calls _delegate under the hood:

function _delegate(address _vault, address _from, address _to) internal {
    address _currentDelegate = _delegateOf(_vault, _from);
    if (_to == _currentDelegate) {
      revert SameDelegateAlreadySet(_to);
    }

    delegates[_vault][_from] = _to;

    _transferDelegateBalance(
      _vault,
      _currentDelegate,
      _to,
      uint96(userObservations[_vault][_from].details.balance)
    );

    emit Delegated(_vault, _from, _to);
  }

If a user wanted to reset the delegation to themselves they would specify _to as address(0). However, the issue with this is that the underlying _transferDelegateBalance call will mistakenly move the delegated user funds to the 0 address.

At this point the user might try to call delegate again with their actual address, however now the (_to == _currentDelegate) check will be true and revert, because of the behaviour specified earlier. The user also can't delegate to any other address because they don't own their own delegate balance anymore. Their funds are officially lost forever.

Below is a change to the existing test suite that can be executed with forge test -vvv --match-path test/unit/Vault/Withdraw.t.sol to demonstrate this issue:

diff --git a/test/unit/Vault/Withdraw.t.sol b/test/unit/Vault/Withdraw.t.sol
index 6a15a59..3cec9e3 100644
--- a/test/unit/Vault/Withdraw.t.sol
+++ b/test/unit/Vault/Withdraw.t.sol
@@ -47,6 +47,36 @@ contract VaultWithdrawTest is UnitBaseSetup {
     vm.stopPrank();
   }
 
+  function testFundsLostForever() external {
+    vm.startPrank(alice);
+    uint256 _amount = 1000e18;
+    underlyingAsset.mint(alice, _amount);
+
+    // Alice deposits as usual
+    _deposit(underlyingAsset, vault, _amount, alice);
+
+    // Alice decides she wants to delegate to bob
+    twabController.delegate(address(vault), bob);
+
+    // Alice now tries to reset her delegation
+    twabController.delegate(address(vault), address(0));
+
+    // At this point the funds are lost!! Alice tries to recover her funds in any way...
+    
+    // Alice tries to delegate back to herself but can't
+    vm.expectRevert();
+    twabController.delegate(address(vault), alice);
+
+    // Alice also can't delegate to any other address
+    vm.expectRevert();
+    twabController.delegate(address(vault), bob);
+    
+    // Alice can't withdraw because her funds have been lost forever :(
+    // Expecting a revert with "DelegateBalanceLTAmount(0, 1000000000000000000000)"
+    vault.withdraw(vault.maxWithdraw(alice), alice, alice);
+    vm.stopPrank();
+  }
+
   function testWithdrawMoreThanMax() external {
     vm.startPrank(alice);
 

Tools Used

Manual review + foundry

Recommended Mitigation Steps

The simplest way to fix this issue is to prevent delegating back to the 0 address. If a user delegates away from the default, then they can delegate back to themselves by specifying their own address:

diff --git a/src/TwabController.sol b/src/TwabController.sol
index a7e2d51..ae7b9ea 100644
--- a/src/TwabController.sol
+++ b/src/TwabController.sol
@@ -646,6 +646,7 @@ contract TwabController {
    * @param _to the address to delegate to
    */
   function _delegate(address _vault, address _from, address _to) internal {
+    require(_to != address(0), "Cannot delegate back to 0 address");
     address _currentDelegate = _delegateOf(_vault, _from);
     if (_to == _currentDelegate) {
       revert SameDelegateAlreadySet(_to);

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

When anyone calls the increaseReserve method in PrizePool.sol the accounted balance in the prize pool isn't properly updated. This allows a vault to effectively steal the prize token contribution and this contribution gets distributed during draws, effectively double counting the initial injection into the reserves. The actual prize token balance of the prize pool will be below the accounted balance of the prize pool as time goes on.

Proof of Concept

As mentioned in the contest README, "the balance of prize tokens held by the contract must always be equal to the sum of the available tier liquidity and the reserve. When contributing liquidity the prize pool will temporarily hold a balance greater than the accounted balance, but otherwise the two should match". Unfortunately this is broken when anyone directly contributes directly to the reserve by calling increaseReserve.

In the normal contest flow, the reserve is increased when a draw is closed by the draw manager. During calls to closeDraw, the next draw is started with a given number of tiers and the contributions for the round are calculated and split across the tiers and the reserve:

_nextDraw(_nextNumberOfTiers, uint96(_contributionsForDraw(lastClosedDrawId + 1)));

Under the hood, this calls _computeNewDistributions which calculates the amount by which to increase the reserves based on the number of reserve shares and the new prize token liquidity being contributes in this round. During this flow, the actual balance of reward tokens held in the prize pool are equal to the accounted balance.

The break in accounting occurs when calling increaseReserve:

  function increaseReserve(uint104 _amount) external {
    _reserve += _amount;
    prizeToken.safeTransferFrom(msg.sender, address(this), _amount);
    emit IncreaseReserve(msg.sender, _amount);
  }

As you can see, the prize tokens are transferred into the pool and the reserve increased. But the accounted balance is unchanged:

  function _accountedBalance() internal view returns (uint256) {
    Observation memory obs = DrawAccumulatorLib.newestObservation(totalAccumulator);
    return (obs.available + obs.disbursed) - _totalWithdrawn;
  }

Because the accounted balance is unchanged, any vault can now call contributePrizeTokens to effectively steal the funds meant for the reserve:

function contributePrizeTokens(address _prizeVault, uint256 _amount) external returns (uint256) {
    uint256 _deltaBalance = prizeToken.balanceOf(address(this)) - _accountedBalance();

This increases the relevant vault accumulator and the total accumulator, thereby effectively double counting the same prize tokens (since we've already increased _reserve).

Tools Used

Manual Review

Recommended Mitigation Steps

The accounted balance of the prize pool should be updated when increaseReserve is called. I think the easiest way of achieving this is having a tracker for "reserve injections":

diff --git a/src/PrizePool.sol b/src/PrizePool.sol
index a42a27e..3c14476 100644
--- a/src/PrizePool.sol
+++ b/src/PrizePool.sol
@@ -233,6 +233,9 @@ contract PrizePool is TieredLiquidityDistributor {
   /// @notice The total amount of prize tokens that have been claimed for all time.
   uint256 internal _totalWithdrawn;
 
+  /// @notice The total amount of reserve injections that have been performed for all time.
+  uint256 internal _reserveInjections;
+
   /// @notice The winner random number for the last closed draw.
   uint256 internal _winningRandomNumber;
 
@@ -497,6 +500,7 @@ contract PrizePool is TieredLiquidityDistributor {
   /// @param _amount The amount of tokens to increase the reserve by
   function increaseReserve(uint104 _amount) external {
     _reserve += _amount;
+    _reserveInjections += amount;
     prizeToken.safeTransferFrom(msg.sender, address(this), _amount);
     emit IncreaseReserve(msg.sender, _amount);
   }
@@ -742,7 +746,7 @@ contract PrizePool is TieredLiquidityDistributor {
   /// @return The balance of tokens that have been accounted for
   function _accountedBalance() internal view returns (uint256) {
     Observation memory obs = DrawAccumulatorLib.newestObservation(totalAccumulator);
-    return (obs.available + obs.disbursed) - _totalWithdrawn;
+    return (obs.available + obs.disbursed) - _totalWithdrawn + _reserveInjections;
   }
 
   /// @notice Returns the start time of the draw for the next successful closeDraw

Assessed type

Math

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-L1074 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L653-L657

Vulnerability details

Impact

A user can set arbitrary vault hooks that are called when a prize is claimed by the claimer. A malicious user can use these hooks to gas grief the claimer and cause all the other claims in the call to fail. A smart attacker could craft the hooks to only grief when claiming small prizes but succeed when claiming prizes that are of a high value (i.e. from lower tiers).

Proof of Concept

The Vault contract has the concept of hooks that are called when a prize is claimed for a specific winner. These hooks can be used to execute custom logic before/after a prize is claimed for the winner. A winner is able to set a hook by calling setHooks in Vault.sol.

When the claimer claims prizes for an array of winners, the hooks for the relevant winner are called if they have been enabled:

VaultHooks memory hooks = _hooks[_winner];
    address recipient;
    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
      );
    }

A malicious user can set the implementation afterClaimPrize and beforeClaimPrize methods to execute arbitrary logic like burn gas and revert execution. Like I mentioned above, a smart attacker could craft the hook logic to only grief the claimer when the prize total is below a certain value so that they can still gain from high value prize winnings.

Below is a diff to the existing test suite that can be executed with forge test -vvv --match-path test/unit/Vault/Vault.t.sol that demonstrates this behaviour:

diff --git a/test/unit/Vault/Vault.t.sol b/test/unit/Vault/Vault.t.sol
index d0d6d30..aa26bae 100644
--- a/test/unit/Vault/Vault.t.sol
+++ b/test/unit/Vault/Vault.t.sol
@@ -5,6 +5,26 @@ import { UnitBaseSetup, LiquidationPair, PrizePool, TwabController, VaultMock, E
 import { IVaultHooks, VaultHooks } from "../../../src/interfaces/IVaultHooks.sol";
 import "src/Vault.sol";
 
+contract MaliciousHook is IVaultHooks {
+  function beforeClaimPrize(
+    address winner,
+    uint8 tier,
+    uint32 prizeIndex
+  ) external returns (address) {
+    revert();
+  }
+
+  function afterClaimPrize(
+    address winner,
+    uint8 tier,
+    uint32 prizeIndex,
+    uint256 payout,
+    address recipient
+  ) external {
+    revert();
+  }
+}
+
 contract VaultTest is UnitBaseSetup {
   /* ============ Events ============ */
 
@@ -179,20 +199,15 @@ contract VaultTest is UnitBaseSetup {
     VaultHooks memory hooks = VaultHooks({
       useBeforeClaimPrize: true,
       useAfterClaimPrize: false,
-      implementation: IVaultHooks(makeAddr("hooks"))
+      implementation: IVaultHooks(new MaliciousHook())
     });
     vault.setHooks(hooks);
     vm.stopPrank();
 
-    vm.mockCall(
-      address(hooks.implementation),
-      abi.encodeWithSelector(IVaultHooks.beforeClaimPrize.selector, alice, 1, 0),
-      abi.encode(bob)
-    );
-
     vm.startPrank(address(claimer));
 
     mockPrizePoolClaimPrize(uint8(1), alice, 0, bob, 1e18, address(claimer));
+    vm.expectRevert();
     claimPrize(uint8(1), alice, 0, 1e18, address(claimer));
 
     vm.stopPrank();

Tools Used

Manual review + foundry

Recommended Mitigation Steps

There are 2 main options to mitigate this griefing attack vector:

1. Remove the hook logic altogether
2. Add protection to limit the damage

For option 2 it's important to note that some level of gas griefing is not impossible to avoid, but it can be massively reduced. My suggestion to limit the damage of this attack vector would be to:

1. Safely handle the case where the call reverts and continue claiming rewards for the other winners
2. Specify a limit on the gas passed to the hook calls. This will limit the scope of the hooks but it will also limit the amount of gas griefing that the called hook contract can perform.

Assessed type

call/delegatecall

Lines of code

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

Vulnerability details

Impact

Any users that try to call mintWithPermit will have their transaction reverted. This will drain gas unnecessarily.

Proof of Concept

The ERC20 Permit extension allows approvals to be made via signatures. The idea behind this is to only require the user to submit 1 transaction on chain and therefore only pay the gas for a single transaction rather than having to perform an approval step in another transaction.

The vault contract has 2 permit methods: mintWithPermit and depositWithPermit. With the former you specify the number of shares you want to mint whereas with the latter you specify the number of assets you want to deposit.

The issue with the mintWithPermit method is that the signature for the approval has to be signed with the value of _assets that you're approving to the contract, however the value of _assets isn't known in advance. The check for a valid signature will always fail because there isn't a public method for the user to convert between assets and shares, and most users will probably sign a message with the number of shares instead.

Tools Used

Manual review

Recommended Mitigation Steps

The mintWithPermit method should probably be removed. Even if there was a public view to convert between assets and shares, there is a chance that the exchange rate changes after the user reads the view and before the user signs the message and calls mintWithPermit.

Assessed type

ERC20

Lines of code

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

Vulnerability details

Impact

The following points are stated in the docs:

1. The highest standard prize tier is the most common prize: occurring every single draw.
2. The canary tier has odds of occurring daily (as if it were the highest tier)

Therefore, both the highest standard tier and the canary tier should have odds of 1, however at the moment only the canary tier has odds of 1. The result is that the protocol is not distributing prizes as intended.

Proof of Concept

The issue with the odds calculations can be seen in the TieredLiquidityDistributor.sol contract. Below is a short snippet for the tier odds when there are 3 tiers:

  SD59x18 internal constant TIER_ODDS_0_3 = SD59x18.wrap(2739726027397260);
  SD59x18 internal constant TIER_ODDS_1_3 = SD59x18.wrap(52342392259021369);
  SD59x18 internal constant TIER_ODDS_2_3 = SD59x18.wrap(1000000000000000000);

The canary tier odds are 1 as intended, however the highest normal prize tier odds are incorrect. If the tier odds were being calculated correctly, we should see the last two odds for each number of tiers being 1 (i.e. 1000000000000000000).

Tools Used

Manual review

Recommended Mitigation Steps

Since the TierCalculationLib.getTierOdds only seems to be used by the GenerateConstants script, I suggest modifying the script used to generate the constants that are placed in TieredLiquidityDistributor.sol:

diff --git a/script/generateConstants.s.sol b/script/generateConstants.s.sol
index fe1d4ed..fb9f90a 100644
--- a/script/generateConstants.s.sol
+++ b/script/generateConstants.s.sol
@@ -38,16 +38,24 @@ contract GenerateConstants is Script {
     MAX_TIERS = 15;
     // Precompute the odds for each tier
     for (uint8 numTiers = MIN_TIERS; numTiers <= MAX_TIERS; numTiers++) {
-      for (uint8 tier = 0; tier < numTiers; tier++) {
+      for (uint8 tier = 0; tier < numTiers - 1; tier++) {
         console.log(
           "SD59x18 internal constant TIER_ODDS_%d_%d = SD59x18.wrap(%d);",
           uint256(tier),
           uint256(numTiers),
           uint256(
-            SD59x18.unwrap(TierCalculationLib.getTierOdds(tier, numTiers, GRAND_PRIZE_PERIOD_DRAWS))
+            SD59x18.unwrap(TierCalculationLib.getTierOdds(tier, numTiers - 1, GRAND_PRIZE_PERIOD_DRAWS))
           )
         );
       }
+      console.log(
+          "SD59x18 internal constant TIER_ODDS_%d_%d = SD59x18.wrap(%d);",
+          uint256(numTiers - 1),
+          uint256(numTiers),
+          uint256(
+            SD59x18.unwrap(TierCalculationLib.getTierOdds(numTiers - 1, numTiers, GRAND_PRIZE_PERIOD_DRAWS))
+          )
+        );
     }
   }
 }

Assessed type

Math

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L361 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L446-L448 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L781-L810

Vulnerability details

Impact

As mentioned in the docs, "The Canary Tier is a special prize tier that receives a smaller amount of prize liquidity. The Canary Tier informs the Prize Pool as to whether it's worth increasing the number of prize tiers. The canary tier's role is to let us know whether it's worth offering n+1 tiers of prizes".

The intended behaviour is that the number of tiers only increase if a high enough portion of both the highest non-canary tier prizes and the canary tier prizes are claimed, so that prizes scale with demand and liquidity. However, there is a bug that causes the number of tiers to increase if at least 1 canary prize is claimed. Therefore it is highly likely that it will take just 12 draws to reach the cap of 15 prize tiers (or an attacker could force the situation by claiming canary prizes), at which point the prize sizes will be broken based on the liquidity provided and the protocol will become almost unusable.

Proof of Concept

When a draw has finished/elapsed, the draw manager closes the draw by calling closeDraw. During this closing process the next number of tiers is computed:

_nextNumberOfTiers = _computeNextNumberOfTiers(_numTiers);

Before we dive into this computation, it's worth exploring the claimPrize method. Whenever a prize is claimed there is a largestTierClaimed state variable that is set to the highest tier that has been claimed for (which can include the canary tier):

    if (largestTierClaimed < _tier) {
      largestTierClaimed = _tier;
    }

Now, the issue titled in this report exists because of how the next number of tiers is calculated in the _computeNextNumberOfTiers function:

function _computeNextNumberOfTiers(uint8 _numTiers) internal view returns (uint8) {
    UD2x18 _claimExpansionThreshold = claimExpansionThreshold;

    uint8 _nextNumberOfTiers = largestTierClaimed + 2; // canary tier, then length
    _nextNumberOfTiers = _nextNumberOfTiers > MINIMUM_NUMBER_OF_TIERS
      ? _nextNumberOfTiers
      : MINIMUM_NUMBER_OF_TIERS;

    if (_nextNumberOfTiers >= MAXIMUM_NUMBER_OF_TIERS) {
      return MAXIMUM_NUMBER_OF_TIERS;
    }

    // check to see if we need to expand the number of tiers
    if (
      _nextNumberOfTiers >= _numTiers &&
      canaryClaimCount >=
      fromUD60x18(
        intoUD60x18(_claimExpansionThreshold).mul(_canaryPrizeCountFractional(_numTiers).floor())
      ) &&
      claimCount >=
      fromUD60x18(
        intoUD60x18(_claimExpansionThreshold).mul(toUD60x18(_estimatedPrizeCount(_numTiers)))
      )
    ) {
      // increase the number of tiers to include a new tier
      _nextNumberOfTiers = _numTiers + 1;
    }

    return _nextNumberOfTiers;
  }

For the sake of argument let's say the prize pool had 3 tiers, so the canary tier is tier 2. If there is at least 1 canary tier prize that has been claimed for the previous draw then largestTierClaimed = 2 and therefore _nextNumberOfTiers = 4. What's interesting here is that even if the check to expand the number of tiers fails, we still return _nextNumberOfTiers which has just been set to 4. So we're expanding the number of tiers even if the claim count isn't higher than the claim expansion thresholds.

I have made a tiny change to the existing test suite to demonstrate that the number of tiers will increase with just 1 canary prize claim. It can be executed with forge test -vvv --match-path test/PrizePool.t.sol:

diff --git a/test/PrizePool.t.sol b/test/PrizePool.t.sol
index 45d8606..36063eb 100644
--- a/test/PrizePool.t.sol
+++ b/test/PrizePool.t.sol
@@ -561,25 +561,12 @@ contract PrizePoolTest is Test {
   function testCloseAndOpenNextDraw_expandingTiers() public {
     contribute(1e18);
     closeDraw(1234);
-    mockTwab(address(this), address(this), 0);
-    claimPrize(address(this), 0, 0);
-    mockTwab(address(this), sender1, 1);
-    claimPrize(sender1, 1, 0);
-    mockTwab(address(this), sender2, 1);
-    claimPrize(sender2, 1, 0);
-    mockTwab(address(this), sender3, 1);
-    claimPrize(sender3, 1, 0);
-    mockTwab(address(this), sender4, 1);
-    claimPrize(sender4, 1, 0);
+
+    // Just 1 canary prize tier claim increases the number of tiers
 
     // canary tiers
     mockTwab(address(this), sender5, 2);
     claimPrize(sender5, 2, 0);
-    mockTwab(address(this), sender6, 2);
-    claimPrize(sender6, 2, 0);
-
-    vm.expectEmit();
-    emit DrawClosed(2, 245, 3, 4, 7997159090909503, UD34x4.wrap(7357954545454530000), prizePool.lastClosedDrawEndedAt());
 
     closeDraw(245);
     assertEq(prizePool.numberOfTiers(), 4);

Tools Used

Manual review + foundry

Recommended Mitigation Steps

The _computeNextNumberOfTiers logic should be updated to the below:

diff --git a/src/PrizePool.sol b/src/PrizePool.sol
index a42a27e..016124a 100644
--- a/src/PrizePool.sol
+++ b/src/PrizePool.sol
@@ -791,19 +791,22 @@ contract PrizePool is TieredLiquidityDistributor {
     }
 
     // check to see if we need to expand the number of tiers
-    if (
-      _nextNumberOfTiers >= _numTiers &&
-      canaryClaimCount >=
-      fromUD60x18(
-        intoUD60x18(_claimExpansionThreshold).mul(_canaryPrizeCountFractional(_numTiers).floor())
-      ) &&
-      claimCount >=
-      fromUD60x18(
-        intoUD60x18(_claimExpansionThreshold).mul(toUD60x18(_estimatedPrizeCount(_numTiers)))
-      )
-    ) {
-      // increase the number of tiers to include a new tier
-      _nextNumberOfTiers = _numTiers + 1;
+    if (_nextNumberOfTiers > _numTiers) {
+      if (canaryClaimCount >=
+        fromUD60x18(
+          intoUD60x18(_claimExpansionThreshold).mul(_canaryPrizeCountFractional(_numTiers).floor())
+        ) &&
+        claimCount >=
+        fromUD60x18(
+          intoUD60x18(_claimExpansionThreshold).mul(toUD60x18(_estimatedPrizeCount(_numTiers)))
+        )
+      ) {
+        // increase the number of tiers to include a new tier
+        return _numTiers + 1;
+      }
+      else {
+        return _numTiers;
+      }
     }
 
     return _nextNumberOfTiers;

Assessed type

Math

Lines of code

https://github.com/GenerationSoftware/pt-v5-claimer/blob/57a381aef690a27c9198f4340747155a71cae753/src/Claimer.sol#L169-L185 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L641-L647 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L415-L419

Vulnerability details

Impact

Vaults can be created permissionlessly in PoolTogetherV5 and are assigned an owner at creation time. The owner can abuse the lack of validation for the claimer contract and the size of the fee in the prize pool calculations to steal prizes from users. For example, the owner of the vault can even steal the grand prize from the winner.

Proof of Concept

When a vault is deployed through the vault factory, the owner of the vault can be set to any address. The owner of the vault has some privileges that include setting the address of the claimer. The claimer is a contract that handles the prize claiming process and incentivises callers to claim prizes through fees.

At the moment there is no validation as to whether the claimer contract is non-malicious when it is set by the owner:

function setClaimer(address claimer_) external onlyOwner returns (address) {
    address _previousClaimer = _claimer;
    _setClaimer(claimer_);

    emit ClaimerSet(_previousClaimer, claimer_);
    return claimer_;
  }

A smart (and malicious) vault owner will set the claimer contract to a benign looking claimer contract that operates broadly like the claimer contracts are designed to.

The Claimer.sol contract that is designed by the PoolTogether team broadly uses a VRGDA auction mechanism to incentivise prize claims. The fee calculation of the claimer contract is designed so that the maximum fee per claim is capped at a predefined portion of the prize size of the highest tier (for non-canary tiers):

function _computeMaxFee(uint8 _tier, uint8 _numTiers) internal view returns (uint256) {
    uint8 _canaryTier = _numTiers - 1;
    if (_tier != _canaryTier) {
      // canary tier
      return _computeMaxFee(prizePool.getTierPrizeSize(_canaryTier - 1));
    } else {
      return _computeMaxFee(prizePool.getTierPrizeSize(_canaryTier));
    }
  }

  /// @notice Computes the maximum fee that can be charged.
  /// @param _prize The prize to compute the max fee for
  /// @return The maximum fee that can be charged
  function _computeMaxFee(uint256 _prize) internal view returns (uint256) {
    // compute the maximum fee that can be charged
    return UD60x18.unwrap(maxFeePortionOfPrize.intoUD60x18().mul(UD60x18.wrap(_prize)));
  }

However, since the vault owner is able to specify an arbitrary claimer contract for a vault, they have the ability to change this logic. Specifically, they can set maxFeePortionOfPrize to 100% and use the actual tier being claimed for rather than the highest non-canary tier:

return _computeMaxFee(prizePool.getTierPrizeSize(_tier));

This exploit path is possible because the claimPrize method in PrizePool.sol doesn't restrict the size of the fee as much as the intended Claimer.sol contract:

    Tier memory tierLiquidity = _getTier(_tier, numberOfTiers);

    if (_fee > tierLiquidity.prizeSize) {
      revert FeeTooLarge(_fee, tierLiquidity.prizeSize);
    }

Rather than capping the max fee at the prize size of the highest non-canary tier, it instead caps the fee at the prize size of the _tier argument. The end result is that the vault owner can steal prizes from any winners that belong to the vault.

As touched on above, a smart vault owner will design the malicious claimer contract in a way where it appears to operate like it should and still provides a portion of prizes to its winners, whilst stealing a portion of the prizes for themselves.

Tools Used

Manual review

Recommended Mitigation Steps

Firstly, I would suggest making the fee check in the PrizePool.sol contract more aggressive to cap the fee at the prize size of the highest non-canary tier rather than the tier being claimed for.

Secondly, I would highly recommend using a registry pattern for the Claimer contract. Rather than allowing the vault admin to set an arbitrary claimer contract, the vault should fetch an implementation from a registry controlled by PoolTogether and deploy a clone. The registry could even offer multiple different claimer contracts that have slightly different behaviours that can be chosen by the vault owner. This way you know a vault's claimer will always use audited and safe logic.

Assessed type

Invalid Validation

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L366 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L421 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L903-L908

Vulnerability details

Impact

When claiming prizes, the higher the contribution percentage of the given vault, the higher the odds of a user contributing to that vault receiving a prize. Because the vault contributions are wrongly calculated, a vault gets credit for distributions that haven't actually been made yet. A vault can swing the odds in its favour by contributing prize tokens to the prize pool in a recent round before claiming prizes (based on a normal value of alpha). The is particularly important when considering prize tiers that occur frequently (the highest tiers).

Proof of Concept

A draw is closed by the draw manager by calling closeDraw. During this process, new liquidity is distributed among the tiers to be available for prize claims:

_nextDraw(_nextNumberOfTiers, uint96(_contributionsForDraw(lastClosedDrawId + 1)));

In the above line the contributions are calculated based on lastClosedDrawId + 1 because lastClosedDrawId has not yet been incremented. It is incremented at the end of the _nextDraw method.

A prize is claimed when a vault calls (via a claimer) claimPrize. In this process the contribution percentage of the vault is calculated because it affects the winning region (i.e. odds) for a user:

(SD59x18 _vaultPortion, SD59x18 _tierOdds, uint16 _drawDuration) = _computeVaultTierDetails(
      msg.sender,
      _tier,
      numberOfTiers,
      lastClosedDrawId
    );

As can be seen above, the vault portion is calculated based on lastClosedDrawId which is the value which was incremented when closing the draw previously. This method makes the following underlying call:

vaultPortion = _getVaultPortion(
      _vault,
      uint16(drawDuration > _lastClosedDrawId ? 0 : _lastClosedDrawId - drawDuration + 1),
      _lastClosedDrawId + 1,
      smoothing.intoSD59x18()
    );

This calculates the value dispersed between a start and end draw INCLUSIVE. Therefore, this is actually calculating the vault contributions based on the current in-progress round as well that hasn't yet been dispersed to the tiers. In fact, it also wrongly increments the start round id as well.

Tools Used

Manual review

Recommended Mitigation Steps

The _computeVaultTierDetails method should not increment the start and end draw ids:

diff --git a/src/PrizePool.sol b/src/PrizePool.sol
index a42a27e..34548c9 100644
--- a/src/PrizePool.sol
+++ b/src/PrizePool.sol
@@ -902,8 +902,8 @@ contract PrizePool is TieredLiquidityDistributor {
     drawDuration = uint16(TierCalculationLib.estimatePrizeFrequencyInDraws(tierOdds));
     vaultPortion = _getVaultPortion(
       _vault,
-      uint16(drawDuration > _lastClosedDrawId ? 0 : _lastClosedDrawId - drawDuration + 1),
-      _lastClosedDrawId + 1,
+      uint16(drawDuration > _lastClosedDrawId ? 0 : _lastClosedDrawId - drawDuration),
+      _lastClosedDrawId,
       smoothing.intoSD59x18()
     );
   }

Assessed type

Math

Summary

Overall I would say that the PoolTogetherV5 protocol achieves its goal of being autonomous and as decentralised as possible. The core architecture doesn't need any major changes in my opinion, and besides the prize pool lacking some comments around the tier system, there isn't anything basic I'd change.

Despite this, I have managed to find a fair number of significant vulnerabilities/issues that I encourage you to explore. I don't think the issues are a reflection of the quality of the architecture, but rather that the edge cases aren't sufficiently tested yet.

Codebase quality

Overall I would consider the codebase to be of high quality. However, based on the number of findings I have submitted, I do think the test suite could be improved to look at more edge cases; at the moment the test cases mainly focus on "happy path" execution and don't really look at the extreme scenarios that could occur.

Also, given the codebase consists of a large amount of math, I would have expected some more comments to explain the choices behind some of the math. As a result of the lack of comments it took me longer to fully understand the protocol function compared to other protocols, and I relied heavily on the official documentation to supplement the code. I well commented codebase should make it easy to understand the entire functionality without having to refer to external sources.

Splitting the core maths and accumulator logic into specific libraries significantly improves the readability of the code, as well as making it easy to test each component in isolation.

The tiers could definitely be explained better in the codebase. It is currently confusing when the canary tier is included and when it is excluded, and having this special case tier does make the logic more confusing to follow.

Architecture review

Splitting the core components of PoolTogetherV5 into completely independent projects is a smart architecture move in my opinion. It keeps the sections of the protocol that are logically unrelated separate and easier to reason.

The claimer contract is fairly simple by design and achieves its goal of incentivising prize claims without the need of an admin.

The vault logic is significantly more complicated given it utilises the ERC4626 standard, and uses the TWAB controller to keep track of balances. Conforming to the ERC4626 standard is a smart decision given it should increase the ability for the protocol to be interoperable with other protocols going forwards. The TWAB controller balance tracking significantly increases the complexity, but it does help reduce the ability for attackers to abuse the share mechanism whilst still keeping vaults separate from one another.

The TWAB controller is arguably the most important part of the protocol architecture, and given I couldn't find any way for users to manipulate their TWAB, I believe this is very well designed and tested. Keeping vault TWABs independent also makes a lot of sense when it comes to computing prize winners and ensures that only legitimate vaults can be used to significantly change user prize odds. The use of ring buffers both here and in the prize pool are a smart way of capping state usage over time and as far as I could tell are well implemented.

Finally, the prize pool is also well implemented in terms of minimising the number of entry and exit points. My biggest concern with the prize pool were that there was a way to break the balance tracking, however besides this bug, the logic of continually incrementing the distributed balances is a smart way to ensure you're distributing liquidity accurately over time. Particularly given the remainders were also accounted for an included in any calculations. As mentioned above, the complexity of the canary tier does make the prize pool harder to reason about isn't sufficiently commented in my opinion, but I can understand the purpose of the canary tier to solve the autonomy problem.

Overall there are no significant architecture changes I would make, besides including a registry of claimer contracts, which I talk more about in the following section.

Centralisation risks

As intended, the PoolTogetherV5 contracts have no major centralisation risks. The contracts are all autonomous and adapt based on usage.

However, as mentioned in one of my reports, I believe there should be a registry of suitable claimer contracts that is managed by the PoolTogether team. This introduces a minimal amount of centralisation (which doesn't effect the day-to-day operation and success of the protocol) whilst protection users of vaults from malicious vault admins. The centralisation risk of this additional should be near 0, especially if the registry doesn't overwrite previous implementations. This way, even if the admin were compromised new vaults could still be deployed with previously approved claimer implementations.

Recommendations

Besides resolving the issues I've submitted in dedicated reports, my main recommendations (that I've touched on above) would be to increase the comments in the prize pool contract regarding the tier system (in particular the canary tier) and to increase the number of edge case tests. Test coverage of 99.9% doesn't necessarily mean that a codebase is sufficiently tested, and this is a good example.

Time spent:

24 hours