PoolTogether - n1punp's results

Lines of code

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

Vulnerability details

Impact

Yield fee amounts may not be fully collected by the recipient and missed out forever, especially in the case of the yield fee accrual between the transaction submission and tx execution.

Proof of Concept

The yield fee recipient can claim the accrued yield fee through claimYieldFeeShares function by specifying how many shares to claim. However, the function will clear all the yield fees, even when the specified shares is less than the accrued yield fee. This means that the supposedly remaining fee will be gone forever and can no longer be claimed (see the line of code: https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L617 )

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

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

        yieldFeeBalance -= _yieldFeeBalance; // this actually resets the yield fee balance to 0, even if the _shares is much less

        _mint(msg.sender, _shares);

        emit ClaimYieldFeeShares(msg.sender, _shares);
    }

Under normal circumstances, this may seem to be acceptable.

However, it is possible that there is a frontrunning yield fee accrual that happens before the yield fee claiming (the frontrunning yield fee accrual can happen upon liquidation event). If this happens, then the fee will be total discarded and can never be claimed since the increased yieldFeeBalance will be reset to 0 according to the problematic line.

Tools Used

Manual Review

Recommended Mitigation Steps

Only subtract the yieldFeeBalance by the minted _shares instead, for correct accounting, as follows:

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

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

        yieldFeeBalance -= _shares; // update this to reduce only by _shares

        _mint(msg.sender, _shares);

        emit ClaimYieldFeeShares(msg.sender, _shares);
    }

Assessed type

Other