Yield Fee Shares could be lost while claiming claimYieldFeeShares

[c4-bot-7]

Lines of code

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

Vulnerability details

Impact

• Whenever onlyYieldFeeRecipient calls claimYieldFeeShares(uint256 _shares), if _shares < total shares accrued from yield fee balance then there will be loss of yieldFeeBalance and that yieldFeeBalance could never be claimed back.

• There is no check in claimYieldFeeShares() that how many shares the claimer wants to claim so according to that yieldFeeBalance should get subtracted, but here whole yieldFeeBalance gets subtracted without checking how many shares the claimer wants to claim

Proof of Concept

• Assume vault has generated yield and we have liquidated it.
• A yield fee has been generated and it's in the form of shares.
• Now Bob who is YieldFeeReceipient claims the yield through claimYieldFeeShares(uint256 _shares) by passing it only half of the total yield fee shares .
• Bob get's minted half of the total shares.
• Bob can never claim back his other half of the shares as yieldFeeBalance becoming zero without checking that Bob is trying to claim total or partial amout of shares.

• Thus Bob can't claim his remaining half of the yield fee share.

  function testClaimFeeBalance_Bug() 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();
  
      assertEq(vault.totalAssets(), 1e18);
      assertEq(vault.totalSupply(), 1e18);
      assertEq(vault.totalDebt(), 1e18);
  
      // mint yield to the vault and liquidate
      underlyingAsset.mint(address(vault), 1e18);
      vault.setLiquidationPair(address(this));
  
      uint256 maxLiquidation = vault.liquidatableBalanceOf(address(underlyingAsset));
  
      uint256 amountOut = maxLiquidation / 2;
  
      uint256 yieldFee = (1e18 - vault.yieldBuffer()) / (2 * 10); // 10% yield fee + 90% amountOut = 100%
      console.log("the generated fee from liquidation ",yieldFee);
      vault.transferTokensOut(address(0), bob, address(underlyingAsset), amountOut);
  
      assertEq(vault.totalAssets(), 1e18 + 1e18 - amountOut); // existing balance + yield - amountOut
      assertEq(vault.totalSupply(), 1e18); // no change in supply since liquidation was for assets
      assertEq(vault.totalDebt(), 1e18 + yieldFee); // debt increased since we reserved shares for the yield fee
  
      // Claiming yield fee shares
      vm.startPrank(bob);
      console.log("Before claiming Yield fee balance  ",vault.yieldFeeBalance());
  
      vault.claimYieldFeeShares(yieldFee/2); // claiming only 1/2 of the yieldFee share and remaining half of the shares.
      // Half of the shares yet to be claimed
      assertEq(vault.totalDebt(), vault.totalSupply());
      // As we can see yieldFeeBalance became zero while claiming only 1/2 of the YieldFeeShare.
      // But in original  yieldFeeBalance should be vault.yieldFeeBalance()/2 but due to bug vault.yieldFeeBalance() becomes zero.
      // Thus losing 1/2 of the yieldFeeBalance.
      assertEq(vault.yieldFeeBalance(), 0);
      console.log("After claiming half of the shares , 1/2 of the yield fee balance should be remaining");
      console.log("the yield fee balance",vault.yieldFeeBalance());
      console.log("The yield fee balance got 0 instead of being half");
      vm.stopPrank();
  }

Tools Used

Manual Review

Recommended Mitigation Steps

While claiming claimYieldFeeShares() it should be checked how many amount of shares the claimer wants to claim rather than subtracting whole yieldFeeBalance.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #10

[c4-pre-sort]

raymondfam marked the issue as duplicate of #59

[c4-judge]

hansfriese marked the issue as satisfactory