PoolTogether - Fitro's results

Lines of code

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

Vulnerability details

Impact

claimYieldFeeShares() is used for the collection of yield fees to a specific recipient.

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

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

@>       yieldFeeBalance -= _yieldFeeBalance;
        
        _mint(msg.sender, _shares);

        emit ClaimYieldFeeShares(msg.sender, _shares);
    }

As observed, _shares is used to specify the amount of fees to collect, the issue arises when _shares != yieldFeeBalance. The issue lies in the fact that only _shares are minted, but the entire yieldFeeBalance is subtracted. Consequently, the remaining fees are lost, specifically yieldFeeBalance - _shares.

Proof of Concept

To execute the POC copy the provided code into the PrizeVault.t.sol file.

function testYieldFeeLost() 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%
        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

        vm.startPrank(bob);
        vault.claimYieldFeeShares(yieldFee / 2); //claim yieldFee / 2
        assertEq(vault.totalDebt(), vault.totalSupply());
        assertEq(vault.yieldFeeBalance(), 0); //the other yieldFee / 2 is lost
        vm.stopPrank();
    }

As observed, only half of the fees are claimed, but the vault.yieldFeeBalance() is 0.

Tools Used

Manual review.

Recommended Mitigation Steps

Change yieldFeeBalance -= _yieldFeeBalance for yieldFeeBalance -= _shares.

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

Other