Yield fee is permanently lost if fee claimer doesn't claim all the accumulated yield

[c4-bot-6]

Lines of code

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

Vulnerability details

Impact

The protocol provides the ability to set yield fee and yield fee recipient when registering an Yield Vault in the Prize Vault Factory. The yield fee is accumulated when a user decides to transfer tokens out of the contract as seen in PrizeVault::transferTokensOut

...
        uint32 _yieldFeePercentage = yieldFeePercentage;

        // Determine the proportional yield fee based on the amount being liquidated:
        uint256 _yieldFee;
        if (_yieldFeePercentage != 0) {
            // The yield fee is calculated as a portion of the total yield being consumed, such that
            // `total = amountOut + yieldFee` and `yieldFee / total = yieldFeePercentage`.
@            _yieldFee = (_amountOut * FEE_PRECISION) / (FEE_PRECISION - _yieldFeePercentage) - _amountOut;
        }

        // Ensure total liquidation amount does not exceed the available yield balance:
        if (_amountOut + _yieldFee > _availableYield) {
            revert LiquidationExceedsAvailable(_amountOut + _yieldFee, _availableYield);
        }

        // Increase yield fee balance:
        if (_yieldFee > 0) {
@            yieldFeeBalance += _yieldFee;
        }
...

The accumulated yield can then be withdrawn by the yield fee recipient via PrizeVault::claimYieldFeeShares. The function takes in one parameter specifying the amount of accumulated yield shares to mint.
However instead of subtracting the amount of shares from the yield fee balance, the balance is subtracted from itself, effectively setting it to zero loosing the yildFeeBalance - _shares amount.

Note that this issue is not present if _shares == _yieldFeeBalance.

Proof of Concept

Add the following test to Liquidate.t.sol and run with forge test --mt testClaimYieldFeeShares_YieldDeleted -vvv

    function testClaimYieldFeeShares_YieldDeleted() public {
        vault.setYieldFeePercentage(1e8); // 10% fee
        vault.setYieldFeeRecipient(bob);
        vault.setLiquidationPair(address(this));

        // liquidate some yield
        underlyingAsset.mint(address(vault), 1e18);
        uint256 amountOut = vault.liquidatableBalanceOf(address(underlyingAsset));
        assertGt(amountOut, 0);

        vault.transferTokensOut(address(0), alice, address(underlyingAsset), amountOut);
        uint256 yieldFeeBalance = vault.yieldFeeBalance();

        console.log("yieldFeeBalance before: ", yieldFeeBalance);

        vm.prank(bob);
        vault.claimYieldFeeShares(yieldFeeBalance / 3);

        assertEq(vault.balanceOf(bob), yieldFeeBalance / 3);

        console.log("yieldFeeBalance after: ", vault.yieldFeeBalance());
        console.log("Yieeld Fee Bob balance: ", vault.balanceOf(bob));
    }

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

    function claimYieldFeeShares(uint256 _shares) external onlyYieldFeeRecipient {
        if (_shares == 0) revert MintZeroShares();

        uint256 _yieldFeeBalance = yieldFeeBalance;
        if (_shares > _yieldFeeBalance) revert SharesExceedsYieldFeeBalance(_shares, _yieldFeeBalance);

-        yieldFeeBalance -= _yieldFeeBalance;
+        yieldFeeBalance -= _shares;

        _mint(msg.sender, _shares);

        emit ClaimYieldFeeShares(msg.sender, _shares);
    }

Assessed type

Math

[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