PrizeVault.claimYieldFeeShares() resets yieldFeeBalance on every call

[c4-bot-1]

Lines of code

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

Vulnerability details

Summary

Calling PrizeVault.claimYieldFeeShares(uint256 _shares) with any amount of _shares (that passes function conditions) will result in loss of yield (loss of yieldFeeBalance inside vault) because it resets yieldFeeBalance. When yieldFeeRecipient calls this function with _shares that is less then _yieldFeeBalance and not equal to 0, it will lead to _yieldFeeBalance - shares amount of yield being stuck in the PrizeVault contact.

Impact

Fees are stuck in PrizeVault contract.

Proof of Concept

Add this test to PrizeVault.t.sol and run with

    forge test --match-contract PrizeVaultTest --match-test testClaimYieldFeeShares_LossOfFundsIfClaimNotAllYieldFeeBalance

    function testClaimYieldFeeShares_LossOfFundsIfClaimNotAllYieldFeeBalance() 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();

        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);
        vault.transferTokensOut(address(0), bob, address(underlyingAsset), amountOut);

        vm.prank(bob);
        // claim half of yieldFee
        vault.claimYieldFeeShares(yieldFee / 2);

        // because claimYieldFeeShares resets yieldFeeBalance it will always be 0 not depending on the amount of _shares
        assertEq(vault.yieldFeeBalance(), 0);
    }

Recommended Mitigation Steps

Substruct _shares instead of _yieldFeeBalance in PrizeVault.claimYieldFeeShares().

    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