PoolTogether - asui'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

When the yeildFeeRecipient tries to claim some amount of yeildFeeBalance by calling the claimYieldFeeShares function it will mint the given amount of shares but the remaining yeildFeeBalance will also be deducted. Which result in loss of yeildFees for the yeildFeeRecipient.

Proof of Concept

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);
    }

If we look at the function it takes an argument uint256 _shares so it is clear that the caller(yeildFeeRecipient) has the choice to specify the amount of yieldFeeBalance he wants to claim. However regardless of the given amount of _shares to claim, the function mints the given amount of _shares but will always deduct the full amount of yieldFeeBalance.

uint256 _yieldFeeBalance = yieldFeeBalance;

yieldFeeBalance -= _yieldFeeBalance;

        _mint(msg.sender, _shares);

It mints the given amount of shares but always deducts the full amount of yieldFeeBalance.

Since the underlying asset is not withdrawn and still in the yeildVault the assets are not directly lost. It goes back to the availableYeildBalance which will go to the pricePool. There might not be direct loss of assets but the yeildFeeRecipient clearly lost his collectible fees.

Tools Used

manual.

Recommended Mitigation Steps

Replace this line yieldFeeBalance -= _yieldFeeBalance; from the claimYieldFeeShares function with this : yieldFeeBalance -= _shares.

Or another approach would be to remove the parameter uint256 _shares and always mint the total amount of fees; remove if (_shares > _yieldFeeBalance) revert SharesExceedsYieldFeeBalance(_shares, _yieldFeeBalance); and change _mint(msg.sender, _shares); to _mint(msg.sender, _yieldFeeBalance);

Assessed type

Other