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

The entire yieldFeeBalance could be permanently lost due to wrong balance update

Proof Of Concept

The issue is in the logic of the claimYieldFeeShares() function which is intended to allow the yield fee recipient to transfer out yield fee shares. The caller is allowed to specify a non-zero amount of the fee shares to claim and checks that the amount specified is not more than the fee balance else reverts. However, after this check if updates the yield fee balance and then mint the shares for the recipient. The issue here is that the yield fee balance gets updated by the overall balance instead of the amount being claimed:

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 you can see, it minuses the entire balance regardless of the amount of shares to be claimed. This means even when a fee recipient attempts to claim any small amount of fee share, the entire yieldFeeBalance will be permanently lost.

Tools Used

Manual Review

Recommendation

I recommend adding this fix:

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

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

The withdraw and redeem operations in PrizeVault are exposed to high slippage returns and could result in a loss for LPs of PrizeVault.

Proof of Concept

When withdraw or redeem is called, they both work in similar ways in returning the amount to be burnt or received. When all assets are below the debt, the amount returned could vary. However, there is no slippage protection for these operations.

function withdraw(
        uint256 _assets,
        address _receiver,
        address _owner
    ) external returns (uint256) {
        uint256 _shares = previewWithdraw(_assets);
        _burnAndWithdraw(msg.sender, _receiver, _owner, _shares, _assets);
        return _shares;
    }

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

    function redeem(
        uint256 _shares,
        address _receiver,
        address _owner
    ) external returns (uint256) {
        uint256 _assets = previewRedeem(_shares);
        _burnAndWithdraw(msg.sender, _receiver, _owner, _shares, _assets);
        return _assets;
    }

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

However, there are no parameters for amountOutMin or amountInMax, which are used to prevent slippage. These parameters should be checked to create slippage protections.

https://docs.uniswap.org/contracts/v3/guides/providing-liquidity/decrease-liquidity

Tools Used

Visual Studio Code

Recommended Mitigation Steps

There are no parameters for amountOutMin or amountInMax, which are used to prevent slippage for varying return amount. These parameters should be checked to create slippage protections.

https://docs.uniswap.org/contracts/v3/guides/providing-liquidity/decrease-liquidity

Assessed type

MEV