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