Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect precision handling in Calc_Token_Amount() function can leads to unexpected Amount of Lp tokens received #98

Open
hats-bug-reporter bot opened this issue Oct 13, 2024 · 1 comment
Labels
bug Something isn't working invalid This doesn't seem right

Comments

@hats-bug-reporter
Copy link

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

Description:
Description
the calc_token_amount() function in StableSwapTwoPool.sol contract Calculates addition or reduction in token supply from a deposit or withdrawal and it returns the expected amount of LP tokens received.but here the function did not consider for the decimals of tokens(like lets say coin 1 with 18 decimals and token 2 with 6 decimals)here this function directly adds or sub the amount without considering for precision.i.e this function does not explicitly handle precision differences between tokens with varying decimals places.

Attack Scenario\

 /**
     * @notice Calculate addition or reduction in token supply from a deposit or withdrawal
     * Returns the expected amount of LP tokens received. 
     * This calculation accounts for slippage, but not fees.
     * @param amounts: Amount of each coin being deposited
     * @param deposit: Set True for deposits, False for withdrawals
     */
    function calc_token_amount(uint256[N_COINS] memory amounts, bool deposit) external view returns (uint256) {
        /**
        Simplified method to calculate addition or reduction in token supply at
        deposit or withdrawal without taking fees into account (but looking at
        slippage).
        Needed to prevent front-running, not for precise calculations!
        */
        uint256[N_COINS] memory _balances = balances;
        uint256 amp = get_A();
        uint256 D0 = get_D_mem(_balances, amp);
        for (uint256 i = 0; i < N_COINS; i++) {
            if (deposit) {
                _balances[i] += amounts[i];//@audit= does not scale for precision i.e to 1e18?
            } else {
                _balances[i] -= amounts[i];
            }
        }
        uint256 D1 = get_D_mem(_balances, amp);
        uint256 token_amount = token.totalSupply();
        uint256 difference;
        if (deposit) {
            difference = D1 - D0;
        } else {
            difference = D0 - D1;
        }
        return (difference * token_amount) / D0;
    }

lets understand the issue with example below

Attachments

  1. Proof of Concept (PoC) File
for (uint256 i = 0; i < N_COINS; i++) {
    if (deposit) {
        _balances[i] += amounts[i];
    } else {
        _balances[i] -= amounts[i];
    }
}

Scenario

  • Token A: 18 decimals (e.g., DAI)
  • Token B: 6 decimals (e.g., USDC)

Initial State

  • Balances:

  • Token A: 1,000 units (which is 1,000 * 10^18 in actual token units)

  • Token B: 2,000 units (which is 2,000 * 10^6 in actual token units)

  • Total Supply of LP Tokens: 3,000

User Action

A user wants to deposit:

  • 1 unit of Token A
  • 1 unit of Token B

Current Implementation (Without Precision Handling)

Balances Update:

  • For Token A: _balances[0] += 1 (no precision adjustment)
  • For Token B: _balances[1] += 1 (no precision adjustment)

Problem

The current implementation treats 1 unit of Token A and 1 unit of Token B as equivalent, which is incorrect due to their different decimal places. This results in an inaccurate calculation of the pool's invariant and, consequently, the LP tokens.

Expected Implementation (With Precision Handling)

  1. Precision Adjustment:
  • Adjust the deposit amounts to a common precision (18 decimals):
  • For Token A: adjustedAmountA = 1 * 10^18
  • For Token B: adjustedAmountB = 1 * 10^12 (since 10^18 / 10^6 = 10^12)
  1. Balances Update:
  • For Token A: _balances[0] += adjustedAmountA
  • For Token B: _balances[1] += adjustedAmountB
  1. New Balances:

Token A: 1,000 * 10^18 + 1 * 10^18 = 1,001 * 10^18
Token B: 2,000 * 10^6 + 1 * 10^12 = 2,001 * 10^6

  1. Revised Code File (Optional)
for (uint256 i = 0; i < N_COINS; i++) {
    uint256 adjustedAmount = (amounts[i] * RATES[i]) / PRECISION; // Adjust for precision
    if (deposit) {
        _balances[i] += adjustedAmount;
    } else {
        _balances[i] -= adjustedAmount;
    }
}
  • The fix adjusts the deposit or withdrawal amounts for each coin by scaling them to a common precision using the RATES array. This ensures that all calculations are performed with consistent precision, accommodating tokens with different decimal places. The adjusted amounts are then used to update the pool's balances, providing accurate calculations for deposits and withdrawals.
@hats-bug-reporter hats-bug-reporter bot added the bug Something isn't working label Oct 13, 2024
@omega-audits
Copy link

There is not concept of a "unit" in the contract, they operator on the native representation of the tokens.
Merely sayign that this is "incorrect" is not really a helpful description of the issue.

Can you provide a sceneario or PoC where a depositor recieves an "unexpected aount of lp tokens" as described in the title?

@Ghoulouis Ghoulouis added the invalid This doesn't seem right label Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

2 participants