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

In claimYieldFeeShares the fee recipient can choose the amount of shares that they want to claim from the yieldFeeBalance. The issue is that currently, even if the fee recipient chooses a number less than the entire yieldFeeBalance, yieldFeeBalance is reset to 0, preventing the fee recipient from claiming any more of the fees they were initially granted. For example, if the fee recipient chooses to claim only 50 of 100 from the yieldFeeBalance, yieldFeeBalance will be reset to 0, and the fee recipient will not be able to claim the remaining 50 yield fees.

Proof of Concept

As we can see yieldFeeBalance is decremented by _yieldFeeBalance instead of _shares.

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

   yieldFeeBalance -= _yieldFeeBalance;

Tools Used

Manual review

Recommended Mitigation Steps

Update from:

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

   yieldFeeBalance -= _yieldFeeBalance;

To:

   yieldFeeBalance -= _shares;

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The withdraw function of PrizeVault.sol allows a user to specify the amount of assets they want to withdraw. Furthermore, when the total assets are less than the total debt, one asset becomes more expensive than one share, causing more shares to need to be burned than the assets that are received. For example, if a user withdraws 100 assets they may have to burn 120 shares.

The issue is that there is no slippage protection on the amount of shares that the withdrawer is going to have to burn, which will likely cause an unexpected loss of funds. This problem will be further worsened during periods of high market volatility.

Proof of Concept

As we can see in the previewWithdraw function, used in withdraw when there are fewer assets than debt, the shares that will be burned are more than the withdrawn assets. Furthermore, there is no limit on the amount of shares that the caller may need to burn. The lack of slippage protection can also be observed in the redeem function, where the redeemer may receive fewer assets than expected for the amount of shares that they have burned.

    function previewWithdraw(uint256 _assets) public view returns (uint256) {
        uint256 _totalAssets = totalAssets();


        // No withdrawals can occur if the vault controls no assets.
        if (_totalAssets == 0) revert ZeroTotalAssets();


        uint256 totalDebt_ = totalDebt();
        if (_totalAssets >= totalDebt_) {
            return _assets;
        } else {
            // Follows the inverse conversion of `convertToAssets`
            return _assets.mulDiv(totalDebt_, _totalAssets, Math.Rounding.Up);
        }
    }

Tools Used

Manual review

Recommended Mitigation Steps

Include slippage protection on the number of shares burned in withdraw and the number of assets received in redeem.

Assessed type

Other