Rhaydden - `liquidationPair` is able to add any number of `_yieldFee`

[sherlock-admin4]

Rhaydden

medium

liquidationPair is able to add any number of _yieldFee

Summary

The PrizeVault has ann issue where the liquidationPair can artificially inflate the yieldFeeBalance which can result in loss of funds.

Vulnerability Detail

First of all, let's take a look at this function:

The _withdraw function has two conditions that determine whether it performs any actions:

1. If _assets > _latentAssets, it redeems shares from the yieldVault. If _receiver != address(this), it transfers _assets to _receiver.

If neither condition is met, the function does nothing.

function _withdraw(address _receiver, uint256 _assets) internal {
        // The vault accumulates dust from rounding errors over time, so if we can fulfill the withdrawal from the
        // latent balance, we don't need to redeem any yield vault shares.
        uint256 _latentAssets = _asset.balanceOf(address(this));
        if (_assets > _latentAssets) {
            // The latent balance is subtracted from the withdrawal so we don't withdraw more than we need.
            uint256 _yieldVaultShares = yieldVault.previewWithdraw(_assets - _latentAssets);
            // Assets are sent to this contract so any leftover dust can be redeposited later.
            yieldVault.redeem(_yieldVaultShares, address(this), address(this));
        }
        if (_receiver != address(this)) {
            _asset.safeTransfer(_receiver, _assets);
        }
    }

The transferTokensOut function makes a call to the _withdraw function above:

function transferTokensOut(
        address /* sender */,
        address _receiver,
        address _tokenOut,
        uint256 _amountOut
    ) external virtual onlyLiquidationPair returns (bytes memory) {
        if (_amountOut == 0) revert LiquidationAmountOutZero();

        uint256 _totalDebtBefore = totalDebt();
        uint256 _availableYield = _availableYieldBalance(totalPreciseAssets(), _totalDebtBefore);
        uint32 _yieldFeePercentage = yieldFeePercentage;

        // Determine the proportional yield fee based on the amount being liquidated:
        uint256 _yieldFee;
        if (_yieldFeePercentage != 0) {
            // The yield fee is calculated as a portion of the total yield being consumed, such that 
            // `total = amountOut + yieldFee` and `yieldFee / total = yieldFeePercentage`. 
            _yieldFee = (_amountOut * FEE_PRECISION) / (FEE_PRECISION - _yieldFeePercentage) - _amountOut;
        }

        // Ensure total liquidation amount does not exceed the available yield balance:
        if (_amountOut + _yieldFee > _availableYield) {
            revert LiquidationExceedsAvailable(_amountOut + _yieldFee, _availableYield);
        }

        // Increase yield fee balance:
        if (_yieldFee > 0) {
----->            yieldFeeBalance = yieldFeeBalance + _yieldFee;
        }

        // Mint or withdraw amountOut to `_receiver`:
        if (_tokenOut == address(_asset)) {
            _enforceMintLimit(_totalDebtBefore, _yieldFee);
----->            _withdraw(_receiver, _amountOut);
        } else if (_tokenOut == address(this)) {
            _enforceMintLimit(_totalDebtBefore, _amountOut + _yieldFee);
            _mint(_receiver, _amountOut);
        } else {
            revert LiquidationTokenOutNotSupported(_tokenOut);
        }

        emit TransferYieldOut(msg.sender, _tokenOut, _receiver, _amountOut, _yieldFee);

        return "";
    }

Now, _receiver can be specified as any value. If _receiver is specified as address(this) :

The second if in the _withdraw function does not satisfy the condition, receiver == address(this) i.e (When calling transferTokensOut; specifying tokenOut = address(_asset), the _withdraw function will be called) The attacker can transfer a certain amount of _asset to address(this), and the value of _latentAssets in the _withdraw function can be controlled by the attacker.

The attacker passes _assets with a value less than _latentAssets: The second if in the _withdraw function does not satisfy the condition, _assets <= _latentAssets.

When _withdraw does nothing, calling transferTokensOut simply increases the value of the yieldFeeBalance:

 // Increase yield fee balance:
        if (_yieldFee > 0) {
            yieldFeeBalance = yieldFeeBalance + _yieldFee;
        }

This allows an attacker to repeatedly call transferTokensOut and increase the yieldFeeBalance

Impact

liquidationPair adds any amount of yield fees, resulting in the loss of funds.

Code Snippet

https://github.com/sherlock-audit/2024-05-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L1054-L1067

https://github.com/sherlock-audit/2024-05-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L714-L758

Tool used

Manual Review

Recommendation

Modify the transferTokensout like this:

      function transferTokensOut(
      .....  
-      if (_tokenOut == address(_asset)) {
+    if (_tokenOut == address(_asset) && _receiver != address(this)) {
            _enforceMintLimit(_totalDebtBefore, _yieldFee);
            _withdraw(_receiver, _amountOut);
        } else if (_tokenOut == address(this)) {
            _enforceMintLimit(_totalDebtBefore, _amountOut + _yieldFee);
            _mint(_receiver, _amountOut);
        } else {
            revert LiquidationTokenOutNotSupported(_tokenOut);
        }

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

infect3d commented:

lossy behavior for the attacker and no harm to the protocol

[nevillehuang]

Invalid, sponsor comments:

• invalid, the "attacker" in this scenario is just donating any assets that would have been profit to the prize vault instead at their own expense
• the yieldFeeBalance is still increasing with the expected amount based on the liquidtion value
• the _withdraw function does nothing in this case because it does not need to do anything; the funds are already where they need to be for the correct end result