Yield fee is not accounted for during liquidation and therefore lost to the receiver in the `PrizeVault` contract

[c4-bot-10]

Lines of code

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

Vulnerability details

Impact

Yield fee is not accounted for during liquidation in the PrizeVault::transferTokensOut function during such liquidations. Therefore, the yield fee percentage relative to the amountOut is lost.

Proof of Concept

The function below executes liquidations in the PrizeVault contract. As intended by the PoolTogether protocol, the _yieldFee is to be accounted for in the amount being liquidated but in the current implementation, it does not ensure such liquidations get deducted the relative fee while allocating the supposed fee to the yieldFeeBalance.

/// @inheritdoc ILiquidationSource
    /// @dev Emits a `TransferYieldOut` event
    /// @dev Supports the liquidation of either assets or prize vault shares.
    function transferTokensOut(
        address,
        address _receiver,
        address _tokenOut,
        uint256 _amountOut
    ) public virtual onlyLiquidationPair returns (bytes memory) {
        if (_amountOut == 0) revert LiquidationAmountOutZero();

        uint256 _availableYield = availableYieldBalance();
        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 += _yieldFee; // @note yieldFeeBalance accounts for the _yieldFee this transaction will incur
        }

        // Mint or withdraw amountOut to `_receiver`:
        if (_tokenOut == address(_asset)) {
            _withdraw(_receiver, _amountOut);  // @audit _yieldFee is not accounted for         
        } else if (_tokenOut == address(this)) {
            _mint(_receiver, _amountOut); // // @audit _yieldFee is not accounted for
        } else {
            revert LiquidationTokenOutNotSupported(_tokenOut);
        }

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

        return "";
    }

Breaking down the interesting part of the function logic, we can see that:

• yieldFeeBalance increments by the _yieldFee this liquidation will incur.

if (_yieldFee > 0) {
            yieldFeeBalance += _yieldFee;
        }

• But the amount being withdrawn to the receiver is still the full _amountOut. Hence, the yieldFeeBalance has just incurred a loss that wasn't accounted for during the final transaction.

if (_tokenOut == address(_asset)) {
            _withdraw(_receiver, _amountOut);         
        }

• Also, the shares being minted via _mint will happen with the full _amountOut unit of shares instead of _amountOut - _yieldFee

_mint(_receiver, _amountOut);

In the POC below, the test is run with the code as is without the suggested mitigation in the recommended mitigation section of this report:

Place the test in the PrizeVault.t.sol file and run with forge test --mt testliquidationDoesntAccountYieldFeePOC -vv

function testliquidationDoesntAccountYieldFeePOC() public {
        vault.setYieldFeePercentage(1e8); // 10%
        vault.setYieldFeeRecipient(bob); // fee recipient bob
        assertEq(vault.totalDebt(), 0); // no deposits in vault yet

        // alice makes an initial deposit of 100 WETH
        underlyingAsset.mint(alice, 100e18);
        vm.startPrank(alice);
        underlyingAsset.approve(address(vault), 100e18);
        vault.deposit(100e18, alice);
        vm.stopPrank();

        assertEq(vault.totalAssets(), 100e18);
        assertEq(vault.totalSupply(), 100e18);
        assertEq(vault.totalDebt(), 100e18);

        // mint yield to the vault and liquidate
        underlyingAsset.mint(address(vault), 100e18);
        vault.setLiquidationPair(address(this));

        uint256 maxLiquidation = vault.liquidatableBalanceOf(address(underlyingAsset));
        uint256 amountOut = maxLiquidation / 2;

        vault.transferTokensOut(address(0), bob, address(underlyingAsset), amountOut);

        console.log("Accrued yield in the contract: ", vault.yieldFeeBalance());

        console.log("Underlying asset balance of Bob: ", underlyingAsset.balanceOf(bob));
    }

[PASS] testliquidationDoesntAccountYieldFeePOC() (gas: 451042)
Logs:
  Accrued yield in the contract:  4999999999999950000
  Underlying asset balance of Bob:  44999999999999550000

Now, this other POC is run with the recommended mitigation applied to the PrizeVault contract code:

Place the test in the PrizeVault.t.sol file and run with forge test --mt testliquidationAccountsYieldFeePOC -vv

function testliquidationAccountsYieldFeePOC() public {
        vault.setYieldFeePercentage(1e8); // 10%
        vault.setYieldFeeRecipient(bob); // fee recipient bob
        assertEq(vault.totalDebt(), 0); // no deposits in vault yet

        // alice makes an initial deposit of 100 WETH
        underlyingAsset.mint(alice, 100e18);
        vm.startPrank(alice);
        underlyingAsset.approve(address(vault), 100e18);
        vault.deposit(100e18, alice);
        vm.stopPrank();

        assertEq(vault.totalAssets(), 100e18);
        assertEq(vault.totalSupply(), 100e18);
        assertEq(vault.totalDebt(), 100e18);

        // mint yield to the vault and liquidate
        underlyingAsset.mint(address(vault), 100e18);
        vault.setLiquidationPair(address(this));

        uint256 maxLiquidation = vault.liquidatableBalanceOf(address(underlyingAsset));
        uint256 amountOut = maxLiquidation / 2;

        vault.transferTokensOut(address(0), bob, address(underlyingAsset), amountOut);

        console.log("Accrued yield in the contract: ", vault.yieldFeeBalance());

        console.log("Underlying asset balance of Bob: ", underlyingAsset.balanceOf(bob));
    }

[PASS] testliquidationAccountsYieldFeePOC() (gas: 451093)
Logs:
  Accrued yield in the contract:  4999999999999950000
  Underlying asset balance of Bob:  39999999999999600000

Tools Used

Manual review + foundry

Recommended Mitigation Steps

Adjust the logic to account for the _yieldFee in the final _amountOut:

  function transferTokensOut(
      address,
      address _receiver,
      address _tokenOut,
      uint256 _amountOut
  ) public virtual onlyLiquidationPair returns (bytes memory) {

      ...

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

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

      ...
  }

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

Incorrect assumption. _amountOut is typically _liquidYield or _maxAmountOut as determined liquidatableBalanceOf() with _yieldFee already taken care of. (_amountOut + _yieldFee) is then checked again ensuring they do not exceed _availableYield. If you deduct _yieldFee from _amountOut again, it's going to cause under liquidation.

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid