PoolTogether - CodeWasp's results

Lines of code

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

Vulnerability details

Impact

Impact: All of the user's funds are unretrievably locked in the PrizeVault contract.

A combination of issues allows for the following scenario:

1. Alice invokes _withdraw(receiver, assets) (via burn() or withdraw()).
2. The contract computes the number of shares to redeem, via previewWithdraw(assets).
3. The contract redeems as many shares, but the ERC 4626-compliant vault returns fewer shares than expected.
   At this point, the contract holds fewer than assets tokens.
4. The contract attempts to transfer assets to the receiver. This fails due to insufficient funds, but the ERC 20-compliant token does not revert (only returns false).
5. At this point, Alice's assets are locked in the PrizeVault contract. They cannot be withdrawn at a later point, because the corresponding prize vault and yield vault shares have been burned.

The exploit relies on insufficient handling of two corner cases of ERC-20 and ERC-4246:

• ERC-20 does not stipulate that transfer must throw if the message sender holds insufficient balance. Instead, returning false is compliant with ERC-20 and implemented by many tokens, including BAT, cUSDC, EURS, HuobiToken, ZRX and many more.
• ERC-4626 does not stipulate that redeem(previewWithdraw(assets)) transfers at least assets. In particular, redeem(shares, ...) only guarantees that exactly shares are burned. The only guaranteed way to gain a certain amount of assets is by calling withdraw(assets, ...).
  While this is the most standards-compliant scenario, a malicious vault could simply not transfer the required tokens on purpose, and still trigger the same effect as described above.

Proof of Concept

We provide a proof of concept that results in all of Alice's assets locked in the PrizeVault contract and all her shares burned.

Place the file below in test/unit/PrizeVault/PoCLockedFunds.t.sol and run the test with

$ forge test --mt test_poc_lockedFundsOnLossyWithdrawal

// Place in test/unit/PrizeVault/PoCLockedFunds.t.sol
pragma solidity ^0.8.24;

import { UnitBaseSetup } from "./UnitBaseSetup.t.sol";

import { IERC20, IERC4626 } from "openzeppelin/token/ERC20/extensions/ERC4626.sol";
import { ERC20PermitMock } from "../../contracts/mock/ERC20PermitMock.sol";
import { ERC4626Mock } from "openzeppelin/mocks/ERC4626Mock.sol";
import { Math } from "openzeppelin/utils/math/Math.sol";

// An ERC20-compliant token that does not throw on insufficient balance.
contract NoRevertToken is IERC20 {
    uint8   public decimals = 18;
    uint256 public totalSupply;

    mapping (address => uint)                      public balanceOf;
    mapping (address => mapping (address => uint)) public allowance;

    constructor(uint _totalSupply) {
        totalSupply = _totalSupply;
        balanceOf[msg.sender] = _totalSupply;
        emit Transfer(address(0), msg.sender, _totalSupply);
    }

    function transfer(address dst, uint wad) external returns (bool) {
        return transferFrom(msg.sender, dst, wad);
    }
    function transferFrom(address src, address dst, uint wad) virtual public returns (bool) {
        if (balanceOf[src] < wad) return false;                        // insufficient src bal

        if (src != msg.sender && allowance[src][msg.sender] != type(uint).max) {
            if (allowance[src][msg.sender] < wad) return false;        // insufficient allowance
            allowance[src][msg.sender] = allowance[src][msg.sender] - wad;
        }

        balanceOf[src] -= wad;
        balanceOf[dst] += wad;

        emit Transfer(src, dst, wad);
        return true;
    }
    function approve(address usr, uint wad) virtual external returns (bool) {
        allowance[msg.sender][usr] = wad;
        emit Approval(msg.sender, usr, wad);
        return true;
    }
}


// An ERC4626-compliant (yield) vault.
// `withdraw(assets)` burns `assets * totalSupply / (totalAssets + 1)` shares.
// `redeem(shares)` transfers `shares * (totalAssets + 1) / (totalSupply + 1)` assets.
contract YieldVault is ERC4626Mock {
    using Math for uint256;
    constructor(address _asset) ERC4626Mock(_asset) {}

    function previewWithdraw(uint256 assets) public view virtual override returns (uint256) {
        return assets.mulDiv(totalSupply(), totalAssets() + 1);
    }
}

// Demonstrate that all of Alice's funds are locked in the PrizeVault,
// with all corresponding shares burned.
contract PoCLockedFunds is UnitBaseSetup {
    NoRevertToken asset;

    function setUpUnderlyingAsset() public view override returns (ERC20PermitMock) {
        return ERC20PermitMock(address(asset));
    }

    function setUpYieldVault() public override returns (IERC4626) {
        return new YieldVault(address(underlyingAsset));
    }

    function setUp() public override {
        return;
    }

    function test_poc_lockedFundsOnLossyWithdrawal() public {
        uint256 deposited = 1e18;

        // Mint 10^18 tokens and transfer them to Alice.
        asset = new NoRevertToken(deposited);
        super.setUp();
        asset.transfer(alice, deposited);

        // Alice holds all tokens, the yield vault and the price vaults are empty.
        assertEq(underlyingAsset.balanceOf(alice), deposited);
        assertEq(underlyingAsset.balanceOf(address(vault)), 0);
        assertEq(underlyingAsset.balanceOf(address(yieldVault)), 0);
        assertEq(yieldVault.totalSupply(), 0);
        assertEq(yieldVault.balanceOf(address(vault)), 0);
        assertEq(vault.totalSupply(), 0);
        assertEq(vault.balanceOf(alice), 0);

        // Alice enters the vault.
        vm.startPrank(alice);
        underlyingAsset.approve(address(vault), deposited);
        vault.deposit(deposited, alice);

        // All assets were transferred into the yield vault,
        // as many yield vault shares were minted to the prize vault, and
        // as many prize vault shares were minted to Alice.
        assertEq(underlyingAsset.balanceOf(alice), 0);
        assertEq(underlyingAsset.balanceOf(address(vault)), 0);
        assertEq(underlyingAsset.balanceOf(address(yieldVault)), deposited);
        assertEq(yieldVault.totalSupply(), deposited);
        assertEq(yieldVault.balanceOf(address(vault)), deposited);
        assertEq(vault.totalSupply(), deposited);
        assertEq(vault.balanceOf(alice), deposited);

        // Perform the lossy withdraw.
        vault.withdraw(deposited, alice, alice);

        // At this point Alice should've received all her assets back,
        // and all prize/yield vault shares should've been burned.
        // In contrast, no assets were transferred to Alice,
        // but (almost) all shares have been burned.
        assertEq(underlyingAsset.balanceOf(alice), 0);
        assertEq(underlyingAsset.balanceOf(address(vault)), 999999999999999999);
        assertEq(underlyingAsset.balanceOf(address(yieldVault)), 1);
        assertEq(yieldVault.totalSupply(), 1);
        assertEq(yieldVault.balanceOf(address(vault)), 1);
        assertEq(vault.totalSupply(), 0);
        assertEq(vault.balanceOf(alice), 0);

        // As a result, Alice's funds are locked in the vault;
        // she cannot even withdraw a single asset.
        vm.expectRevert();
        vault.withdraw(1, alice, alice);
        vm.expectRevert();
        vault.redeem(1, alice, alice);
    }
}

Tools Used

manual code review

Recommended Mitigation Steps

We recommend to fix both the ERC-20 transfer and ERC-4626 withdrawal.

For the first, it is easiest to rely on OpenZeppelin's SafeERC20 safeTransfer function:

diff --git a/pt-v5-vault/src/PrizeVault.sol b/pt-v5-vault/src/PrizeVault.sol
index fafcff3..de69915 100644
--- a/pt-v5-vault/src/PrizeVault.sol
+++ b/pt-v5-vault/src/PrizeVault.sol
@@ -936,7 +936,7 @@ contract PrizeVault is TwabERC20, Claimable, IERC4626, ILiquidationSource, Ownab
             yieldVault.redeem(_yieldVaultShares, address(this), address(this));
         }
         if (_receiver != address(this)) {
-            _asset.transfer(_receiver, _assets);
+            _asset.safeTransfer(_receiver, _assets);
         }
     }

This already mitigates the erroneous locking of assets.

In addition, we recommend to ensure that at least the necessary amount of shares is withdrawn from the yield vault. In the simplest form, this can be ensured by invoking withdraw directly:

diff --git a/pt-v5-vault/src/PrizeVault.sol b/pt-v5-vault/src/PrizeVault.sol
index fafcff3..9bb0653 100644
--- a/pt-v5-vault/src/PrizeVault.sol
+++ b/pt-v5-vault/src/PrizeVault.sol
@@ -930,10 +930,7 @@ contract PrizeVault is TwabERC20, Claimable, IERC4626, ILiquidationSource, Ownab
         // 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));
+            yieldVault.withdraw(_assets - _latentAssets, address(this), address(this));
         }
         if (_receiver != address(this)) {
             _asset.transfer(_receiver, _assets);

If a tighter bound on redeemed shares is desired, the call to previewWithdraw/redeem should be followed by a withdraw of the outstanding assets:

diff --git a/pt-v5-vault/src/PrizeVault.sol b/pt-v5-vault/src/PrizeVault.sol
index fafcff3..622a7a6 100644
--- a/pt-v5-vault/src/PrizeVault.sol
+++ b/pt-v5-vault/src/PrizeVault.sol
@@ -934,6 +934,13 @@ contract PrizeVault is TwabERC20, Claimable, IERC4626, ILiquidationSource, Ownab
             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));
+            
+            // Redeeming `_yieldVaultShares` may have transferred fewer than the required assets.
+            // Ask for the outstanding assets directly.
+            _latentAssets = _asset.balanceOf(address(this));
+            if (_assets > _latentAssets) {
+                yieldVault.withdraw(_assets - _latentAssets);
+            }
         }
         if (_receiver != address(this)) {
             _asset.transfer(_receiver, _assets);

Assessed type

ERC20