PoolTogether - McToady's results

Lines of code

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L611

Vulnerability details

Impact: The function claimYieldFeeShares allows the yieldFeeRecipient to claim shares corresponding to assets in yieldVault. However regardless of the amount of _shares they request (providing _shares <= yieldFeeBalance), the yieldFeeBalance gets reset to zero, meaning any access to the remaining assets in yieldVault is permanently lost in cases where _shares < yieldFeeBalance.

This is likely caused by the function mistakenly subtracting the entire yieldFeeBalance from itself, rather than _shares:

-   yieldFeeBalance -= _yieldFeeBalance;`
+   yieldFeeBalance -= _shares;

Proof of Concept: Add the following test to PrizeVault.t.sol to confirm that yieldFeeRecipient's tokens in the underlying yield vault become unredeemable:

    function test_YieldFeeBalanceAlwaysReturnsToZero() public {
        vault.setYieldFeePercentage(1e8); // 10%
        vault.setYieldFeeRecipient(bob);
        assertEq(vault.totalDebt(), 0);

        // Make an initial deposit
        underlyingAsset.mint(alice, 1e18);
        vm.startPrank(alice);
        underlyingAsset.approve(address(vault), 1e18);
        vault.deposit(1e18, alice);
        vm.stopPrank();

        // Add funds to liquidiate
        underlyingAsset.mint(address(vault), 1e18);
        vault.setLiquidationPair(address(this));
        uint256 maxLiquidation = vault.liquidatableBalanceOf(address(underlyingAsset));
        uint256 amountOut = maxLiquidation;
        vault.transferTokensOut(address(0), bob, address(underlyingAsset), amountOut);

        // Confirm there are now fees to be claimed
        vm.startPrank(bob);
        uint256 claimableFees = vault.yieldFeeBalance();
        assert(claimableFees > 1);

        // Bob claims some amount less than yieldFeeBalance
        vault.claimYieldFeeShares(1);

        // Confirm yieldFeeBalance gets set back to zero despite not claiming all
        assertEq(vault.yieldFeeBalance(), 0);


        // Bob withdraws
        uint256 bobBalance = vault.balanceOf(bob);
        vault.redeem(bobBalance, bob, bob);
        vm.stopPrank();
        checkInvariants();

        // Confirm the left over claimableFees are in the contract
        assert(underlyingAsset.balanceOf(address(vault)) >= claimableFees - 1);

        // Alice withdraws
        uint256 aliceBalance = vault.balanceOf(alice);
        vm.startPrank(alice);
        vault.redeem(aliceBalance, alice, alice);
        vm.stopPrank();

        // Confirm assets in the contract were used cover Alice's redeem
        assertEq(underlyingAsset.balanceOf(address(vault)), 0);
        // Confirm there are still assets in the vault (Alice's deposited 1e18 - the balance that was in the contract)
        assert(vault.totalAssets() >= claimableFees - 1);
        console.log("Total Assets in yield vault", vault.totalAssets());
        // Confirm there is no longer any prizeVault token supply to redeem these assets
        assertEq(vault.totalSupply(), 0);

        // Alice deposits and redeems a single token to attempt to get back the stuck funds
        vm.startPrank(alice);
        console.log("Alice bal", underlyingAsset.balanceOf(alice));
        underlyingAsset.approve(address(vault), 1);
        vault.deposit(1, alice);
        console.log("Contract balance", underlyingAsset.balanceOf(address(vault)));
        vault.redeem(1, alice, alice);
        
        // Confirm that the stuck assets remain in the yield vault
        console.log("Total Assets in yield vault end", vault.totalAssets());
    }

Recommended Mitigation: It's likely that the developers intention was the following:

    function claimYieldFeeShares(uint256 _shares) external onlyYieldFeeRecipient {
        if (_shares == 0) revert MintZeroShares();

        uint256 _yieldFeeBalance = yieldFeeBalance;
        if (_shares > _yieldFeeBalance) revert SharesExceedsYieldFeeBalance(_shares, _yieldFeeBalance);

-       yieldFeeBalance -= _yieldFeeBalance;
+       yieldFeeBalance -= _shares;

        _mint(msg.sender, _shares);

        emit ClaimYieldFeeShares(msg.sender, _shares);
    }

This would allow the yieldFeeRecipient to claim partial amounts of their yieldFeeBalance without losing access to the remainder.

Assessed type

Other

Table of Contents

1. Security Review Approach
2. Protocol Overview
3. Roles
4. Contract Architecture
5. Risks and Recommendations
6. General Comments

Security Review Approach

When undertaking the review of the PoolTogether V5 Vault codebase the following steps were taken:

1. Go through the PoolTogether V5 Vault documentation as well as any further documentation required to gain relevant context about how it's intended to integrate into the overall PoolTogether V5 Protocol (LiquidationPair, PrizePool).
2. Create control flow diagrams of the key user actions to reference back to where necessary. See Contract Architecture
3. Conduct a thorough manual review of the protocols codebase. Taking note of both potential issues and any parts of the codebase that are unclear.
4. Repeat steps 1-3 until a thorough understanding of the codebase has been acheived.
5. Go through the notes of potential issues one by one until each are either validated or invalidated (either by manually breaking down the logic involved or writing edge cases tests in foundry).
6. Write up the findings as well as general comments and potential recommendations to the protocol team.

A total of approximately 30 hours was spent conducting the security review of this codebase over a 7 day period.

Protocol Overview

While this security review was focused specifically on the PoolTogether V5 Vault implementation, it's necessary to first provide some context on the overall PoolTogether protocol.

PoolTogether V5

PoolTogether V5 is a "no loss lottery" which allows users to deposit a specified asset into ERC4626 vaults where the yield earned (from depositing into protocols such as AAVE and Lido) is awarded to a one of the depositors randomly after specified time periods.

The following six contracts were in scope for this security review:

• PrizeVault.sol
• PrizeVaultFactory.sol
• TwabERC20.sol
• Claimable.sol
• HookManager.sol
• IVaultHooks.sol

PrizeVault

The PoolTogetherV5 PrizeVault is an ERC4626 Vault implementation which allows users to deposit a specified asset into the vault and receiving shares in return. A users shares in a vault act as tickets in the overall PoolTogether system and their overall share of the assets in a given pool represent their likelihood of winning that pools generated yield when prizes are drawn. The PrizeVault contract inherits both TwabERC20 and Claimable.

PrizeVaultFactory

PrizeVaultFactory is a relatively stripped back factory implementation that allows anyone to deploy a PrizeVault instance. It is also responsible for keeping track of all the vaults that have been deployed by this factory contract.

TwabERC20

TwabERC20 is a relatively simple ERC20 token implementation with the key difference being that it's accounting logic is passed to the TwabController contract. This is done so that the time weighted average balance of a user can be tracked across the duration of a lottery, ensuring that winners are selected fairly.

Claimable

Claimable implements the logic that allows an external third party to claim lottery wins on behalf of the winner. The claimPrize function also allows the winner to perform pre and post claim hooks which as set in HookManager.

HookManager

HookManager is a contract which allows a specify pre and post claim hooks to be executed when they are the winner of a lottery. The user does this by deploying a smart contract that meets the IVaultHooks interface and calling specifying this contracts address when calling setHooks.

IVaultHooks

IVaultHooks is a simple interface outlining the necessary functionality a user created VaultHook should adhere to.

Roles

The following roles exist within the system:

• Lottery Participants
• Liquidators
• Claimers
• PrizeVault Owner

Lottery Participants

The lottery participants are the primary actors in the PrizeVault system. Their main actions revolve around being able to deposit/withdraw yield tokens from the PrizeVault contract. They are also able to set custom hooks to be called when they are the winner of a lottery.

Liquidators

The liquidators primary role is to take convert the yield generated by a PrizeVaults underlying yield vault and convert it into the prizeToken of a specified prizePool. In doing this they earn a fee making the liquidator role a profitable opportunity for MEV searchers. While their role has an important impact on the PrizeVault system, the entry point for liquidiators LiquidationPair is out of scope of this security review.

Claimers

Claimers are addresses specified by the PrizeVault Owner that are permissioned to claim prizes on behalf of winners.

PrizeVault Owner

The owner of the PrizeVault contract is able to set claimer addresses, set the liquidation pair address, set the percentage yield fee and set the recipient of the generated yield fees.

Contract Architecture

The following diagrams map out the key user facing functionality and give a brief overview of their effects on the contracts state:

Key user actions:

• Depositing Assets / Minting Shares
• Withdrawing Assets / Redeeming Shares

Depositing and Minting

Withdrawing and Redeeming

Risks and Recommendations

Gas limit on VaultHooks

The Claimable contract sets a HOOK_GAS limit of 150,000 gas for both the before and after claim hooks. However it is reasonable to assume that the most desired use of VaultHooks for a user would be to either withdraw their current shares or deposit the tokens they have won. However from assessing actual withdraw/deposit transactions on the Optimism L2, both calls require around 300-400k gas to complete.

Therefore it would be a reasonble idea for the protocol to allow users to use a maximum of 400,000 gas total between their before and after claim hooks, rather than specifically only allocating half of this to each. Doing so would not increase the damage of gas griefing by users (other than raising the maximum from 150,000 * 2 to 400,000), but give users a lot more freedom to make meaningful calls from their hooks.

Increasing input checks on PrizeVault creation

PrizeVault generation is trustless and any individual is free to create their own instance of a PrizeVault. However there are currently insufficient checks to help ensure the vault is generated safely (for both the individual deploying the prize vault and the users depositing into it). Making the contract deployment less error prone will increase the usability of the protocol and lower the bar to entry for potential users to deploy their own vault instances.

Potential Improvements:

• Add checks that the PrizePool contract the instance is linked to is a legitimate.
• Ensure the value passed to yieldBuffer_ is non zero.
• Add checks that shares ERC20 token name_ and symbol_ are not empty strings.

General Comments

Centralisation Risks

As PrizeVaults can be deployed trustlessly the main centralisation risks of Vaults are passed on to the deployer rather than the protocol itself. Overall the owner of the vault does not have significant powers to act maliciously after a protocol has been deployed, with the most damaging issue being the ability to raise the yieldFeePercentage before a liquidation without the users knowledge.

Code Complexity

Overall the codebase is relatively easy for an interested developer to pick up and understand, particularly the user facing deposit/withdraw aspects of the protocol. However it should be noted that process of converting yield generated to prize tokens becomes quite confusing and is unclear without concerted developer effort to ascertain the relationship between various contracts such as LiquidiationPair, Claimable, PrizeVault and PrizePool when it comes to deciding lottery winners, the liquidation of generated yield to prize tokens and payout of prize toens to winners. It would be valuable to both interested users and future security researchers if these steps could be made clearer.

Documentation

The codebase has complete NATSPEC comments as well as very detailed inline comments explaining any the more unique features of the codebase, such as the yield buffer. On top of this the explainer video provided in the contest overview was clear and gave researchers a good idea of the more complicated aspects of the protocol.

Test Coverage

The codebase has a comprehensive test suite which includes fuzzing, invariant testing and integration tests. Furthermroe, the test suite's layout is easy to understand making it easy for external code reviewers to plug in their own edge case tests where necessary. Overall the coverage of the test suite is comprehensive as shown below.

Time spent:

30 hours