Any fee claim lesser than the total yieldFeeBalance as unit of shares is lost and locked in the PrizeVault contract

[c4-bot-4]

Lines of code

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

Vulnerability details

Impact

Any fee claim by the fee recipient lesser than the accrued internal accounting of the yieldFeeBalance is lost and locked in the PrizeVault contract with no way to pull out the funds.

Proof of Concept

The claimYieldFeeShares allows the yieldFeeRecipient fee recipient to claim fees in yields from the PrizeVault contract. The claimer can claim up to the yieldFeeBalance internal accounting and no more. The issue with this function is it presents a vulnerable area of loss with the _shares argument in the sense that if the accrued yield fee shares is 1000 shares and the claimer claims only 10, 200 or even any amount less than 1000, they forfeit whatever is left of the yieldFeeBalance e.g if you claimed 200 and hence got minted 200 shares, you lose the remainder 800 because it wipes the yieldFeeBalance 1000 balance whereas only minted 200 shares.

Let's see a code breakdown of the vulnerable claimYieldFeeShares function:

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

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

        yieldFeeBalance -= _yieldFeeBalance; // @audit issue stems and realized next line of code

        _mint(msg.sender, _shares); // @audit the point where the claimant gets to lose

        emit ClaimYieldFeeShares(msg.sender, _shares);
    }

This line of the function caches the total yield fee balance accrued in the contract and hence, the fee recipient is entitled to e.g 100

uint256 _yieldFeeBalance = yieldFeeBalance;

This next line of code enforces a comparison check making sure the claimer cannot grief other depositors in the vault because the claimant could for example try to claim and mint 150 shares whereas they are only entitled to 100.

if (_shares > _yieldFeeBalance) revert SharesExceedsYieldFeeBalance(_shares, _yieldFeeBalance);

This line of code subtracts the cached total yield fee balance from the state yield fee balance e.g 100 - 100. So if say Bob the claimant tried to only mint 50 shares at this point in time with the _shares argument, the code wipes the entire balance of 100

yieldFeeBalance -= _yieldFeeBalance;

And this line of code then mints the specified _shares amount e.g 50 shares to Bob.

_mint(msg.sender, _shares);

So what essentially happens is:

• Total accrued fee is 100
• Bob claims 50 shares of the 100
• Bob gets minted 50 shares
• Bob loses the rest 50 shares

Here's a POC for this issue. Place the testUnclaimedFeesLostPOC function inside the PrizeVault.t.sol file and run the test.

function testUnclaimedFeesLostPOC() public {
        vault.setYieldFeePercentage(1e8); // 10%
        vault.setYieldFeeRecipient(bob); // fee recipient bob
        assertEq(vault.totalDebt(), 0); // no deposits in vault yet

        // alice makes an initial deposit of 100 WETH
        underlyingAsset.mint(alice, 100e18);
        vm.startPrank(alice);
        underlyingAsset.approve(address(vault), 100e18);
        vault.deposit(100e18, alice);
        vm.stopPrank();

        console.log("Shares balance of Alice post mint: ", vault.balanceOf(alice));

        assertEq(vault.totalAssets(), 100e18);
        assertEq(vault.totalSupply(), 100e18);
        assertEq(vault.totalDebt(), 100e18);

        // mint yield to the vault and liquidate
        underlyingAsset.mint(address(vault), 100e18);
        vault.setLiquidationPair(address(this));
        uint256 maxLiquidation = vault.liquidatableBalanceOf(address(underlyingAsset));
        uint256 amountOut = maxLiquidation / 2;
        uint256 yieldFee = (100e18 - vault.yieldBuffer()) / (2 * 10); // 10% yield fee + 90% amountOut = 100%
        vault.transferTokensOut(address(0), bob, address(underlyingAsset), amountOut);
        console.log("Accrued yield post in the contract to be claimed by Bob: ", vault.yieldFeeBalance());
        console.log("Yield fee: ", yieldFee);
        // yield fee: 4999999999999950000
        // alice mint: 100000000000000000000

        assertEq(vault.totalAssets(), 100e18 + 100e18 - amountOut); // existing balance + yield - amountOut
        assertEq(vault.totalSupply(), 100e18); // no change in supply since liquidation was for assets
        assertEq(vault.totalDebt(), 100e18 + yieldFee); // debt increased since we reserved shares for the yield fee

        vm.startPrank(bob);
        vault.claimYieldFeeShares(1e17);
        
        console.log("Accrued yield got reset to 0: ", vault.yieldFeeBalance());
        console.log("But the shares minted to Bob (yield fee recipient) should be 4.9e18 but he only has 1e17 and the rest is lost: ", vault.balanceOf(bob));

        // shares bob: 100000000000000000
        assertEq(vault.totalDebt(), vault.totalSupply());
        assertEq(vault.yieldFeeBalance(), 0);
        vm.stopPrank();
    }

Test logs and results:
Logs:
  Shares balance of Alice post mint:  100000000000000000000
  Accrued yield in the contract to be claimed by Bob:  4999999999999950000
  Yield fee:  4999999999999950000
  Accrued yield got reset to 0:  0
  But the shares minted to Bob (yield fee recipient) should be 4.9e18 but he only has 1e17 and the rest is lost:  100000000000000000

Tools Used

Manual review + foundry

Recommended Mitigation Steps

Adjust the claimYieldFeeShares to only deduct the amount claimed/minted

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

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

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

  _mint(msg.sender, _shares);

  emit ClaimYieldFeeShares(msg.sender, _shares);
}

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 high quality report

[c4-pre-sort]

raymondfam marked the issue as not a duplicate

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

It was deducting _shares in the previous vault though.

[c4-sponsor]

trmid (sponsor) confirmed

[trmid]

mitigation: GenerationSoftware/pt-v5-vault#85

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as selected for report