PoolTogether - Aymen0909's results

Lines of code

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

Vulnerability details

Issue Description

The claimYieldFeeShares function in the contract is responsible for allowing the yield fee recipient to claim their shares of the yield fee. However, a critical issue has been identified in the deduction logic within the function:

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

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

    //@audit will decrease all fee balance regardless of amount of shares claimed
    yieldFeeBalance -= _yieldFeeBalance;

    _mint(msg.sender, _shares);

    emit ClaimYieldFeeShares(msg.sender, _shares);
}

As it can be seen the function decrease the yield fee balance (yieldFeeBalance) not based on the _shares parameter provided to the function, but instead the entire balance (_yieldFeeBalance) is deducted, which can result in a loss of funds for the fee recipient because the remaining yield shares (yieldFeeBalance - _shares) will be removed and the recipient will never be able to mint them again.

Impact

The incorrect deduction logic in the claimYieldFeeShares function can lead to a significant loss of funds for the yield fee recipient.

Tools Used

Manual review, VS Code

Recommended Mitigation

Review and correct the deduction logic within the claimYieldFeeShares function to ensure that only the appropriate amount of yield fee is deducted based on the _shares parameter provided.

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

Error

Lines of code

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L454-L472 https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L355-L366 https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L489-L508

Vulnerability details

Issue Description

When a user who has deposited assets into the PrizeVault wishes to withdraw (or redeem) them, they can do so by calling either the withdraw or redeem functions.

Under normal conditions in the vault, users expect to receive their full deposited asset amount back, as there is a 1:1 exchange ratio between the asset amount and shares.

However, if the underlying yield vault experiences a loss, this exchange rate will decrease. This is highlighted in the previewWithdraw function (or previewRedeem/convertToAssets function):

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

function convertToAssets(uint256 _shares) public view returns (uint256) {
    uint256 totalDebt_ = totalDebt();
    uint256 _totalAssets = totalAssets();
    if (_totalAssets >= totalDebt_) {
        return _shares;
    } else {
        // If the vault controls fewer assets than what has been deposited, a share will be worth a
        // proportional amount of the total assets. This can happen due to fees, slippage, or loss
        // of funds in the underlying yield vault.
        return _shares.mulDiv(_totalAssets, totalDebt_, Math.Rounding.Down);
    }
}

function totalAssets() public view returns (uint256) {
    return yieldVault.convertToAssets(yieldVault.balanceOf(address(this))) + _asset.balanceOf(address(this));
}

As shown, if the underlying yield vault experiences a loss, the total assets of the vault given by totalAssets() will decrease and might go below the total shares value totalDebt().

This will trigger the second if statement block, in which the calculated asset (or shares) amount will be converted using the ratio totalAssets/totalDebt (or its inverse for shares).

When a user redeems (or withdraws), if the yield vault experiences a loss while their transaction is pending (waiting in the mempool) and the total assets drop below the total vault debt, they will receive fewer assets after calling either the withdraw/redeem functions than they expected. Instead of a 1:1 ratio, they will use the current totalAssets/totalDebt ratio.

To illustrate this issue, consider the following scenario:

• In the vault, we have totalAssets() = 51000 and totalDebt() = 50000.

• Bob previously deposited 1000 tokens and received 1000 shares in return.

• Bob wants to redeem 500 shares and expects to get his 500 tokens back since currently totalAssets() > totalDebt() (so there's a 1:1 ratio).

• While Bob's transaction is pending in the mempool, the yield vault experiences a loss, and totalAssets() = 46000 drops below totalDebt() = 50000.

• When Bob's transaction goes through, he will receive: (500 * 46000) / 50000 = 460 < 500.

• Thus, instead of receiving 500 tokens, he only gets 460 back, resulting in a loss of 40 tokens.

If Bob had known that the yield vault had experienced a loss, he would have waited until the exchange rate increased again to withdraw his full amount.

This issue is also present in the withdraw function, but in that case, the user will be burning more shares to get the same amount and still incurring a loss.

Impact

Both withdraw/redeem functions lack slippage protection, which can lead to users losing funds in the event of a yield vault loss.

Tools Used

Manual review, VS Code

Recommended Mitigation

Both withdraw/redeem functions should include slippage protection parameters provided by the users (either minimum amount out for redeem function or maximum shares in for withdraw function).

Assessed type

Context