Funds locked due to missing transfer check

[c4-bot-7]

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

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #331

[raymondfam]

The majority of the ERC20 tokens would revert if the transfer of assets failed. Nevertheless, the exact use of _latentAssets could indeed be an issue if the assets withdrawn from the yield vault are deficient even by a single unit. Keeping a minimum asset buffer in the contract would probably be a more guaranteed way of circumventing the issue.

[c4-pre-sort]

raymondfam marked the issue as high quality report

[c4-pre-sort]

raymondfam marked the issue as not a duplicate

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-sponsor]

trmid (sponsor) confirmed

[trmid]

I would like to add that if a "compatible ERC4626 yield vault returns less assets than expected", then it is not actually ERC4626 compatible as these behaviors are required in the spec. That being said, there are likely to be some yield vaults that have errors like this and it is a good thing if we can protect against it without inhibiting the default experience!

The safeTransfer addition seems sufficient, while the other recommended mitigations are unnecessary and would break the "dust collector" strategy that the prize vault employs.

[trmid]

mitigation: GenerationSoftware/pt-v5-vault#86

[hansfriese]

The impact is critical if _asset.transfer() fails silently and it will be mitigated from this known issue.
So according to this criteria, this issue might be OOS if it's fully mitigated by adding safeTransfer.

But another impact is withdraw() might revert when yieldVault.redeem() returns fewer assets than requested and Medium is appropriate.

[c4-judge]

hansfriese changed the severity to 2 (Med Risk)

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as selected for report