## Summary The get_deposit_amounts helper mis-evaluates the user-supplied minimum bound (min_a) by comparing it to desired_a instead of the computed amount_a. The function therefore always accepts deposits whose actual token-A amount is below the caller’s stated minimum, breaking the contract’s explicit slippage guarantee. ## Finding Description get_deposit_amounts calculates balanced deposit amounts for a constant-product pool. When the caller supplies too much token B, the routine derives amount_a that keeps the price in line: ``` let amount_a = desired_b.fixed_mul_floor(&e, &reserve_a, &reserve_b); if amount_a > desired_a || desired_a < min_a { // BUG panic_with_error!(…); } ``` The second predicate must validate amount_a < min_a. Instead, it tests desired_a < min_a, a value set by the user—not the actual amount to be transferred. Consequently: If desired_a ≥ min_a the check always passes, even when amount_a is far below min_a. The pool then mints LP shares against an underfunded token-A deposit, invalidating the caller’s slippage protection and the API’s documented guarantees. This is a pure logic flaw; no race, timing, or external prerequisites exist. ## Impact and Likelihood Explanation **Medium** – caller loses value proportional to slippage; no systemic risk. The vulnerability triggers whenever a caller supplies imbalanced desired_a / desired_b inputs—a common scenario for front-ends that calculate only one side of a two-token deposit. Deposits execute with lower-than-expected token-A contributions, silently defeating the user’s minimum-amount safeguard and causing an unavoidable value loss proportional to the slippage gap. Because deposit operations are routine, occurrence frequency is high. The issue affects only the initiating user and does not endanger pooled funds, yielding a Medium impact rating. ## Proof of Concept `cargo test -p integration-tests --lib bypasses_min_a_check -- --nocapture` A full end-to-end test (integration_tests/tests/test_bypasses_min_a_check.rs) reproduces the issue on a live pool; it is marked #[ignore] to avoid CI failures. ``` #[cfg(test)] use std::println; #[cfg(test)] use soroban_sdk::testutils::Address as _; #[cfg(test)] extern crate std; #[cfg(test)] use crate::testutils::{Setup, create_token_contract, get_token_admin_client}; #[cfg(test)] use soroban_sdk::{Env, Address, Vec}; #[cfg(test)] use soroban_liquidity_pool_contract::pool::get_deposit_amounts; #[test] #[ignore] fn test_bypasses_min_a_check() { let setup = Setup::default(); // Create tokens let token_a = create_token_contract(&setup.env, &setup.admin); let token_b = create_token_contract(&setup.env, &setup.admin); // Get admin clients for minting let token_a_admin = get_token_admin_client(&setup.env, &token_a.address); let token_b_admin = get_token_admin_client(&setup.env, &token_b.address); // Deploy a standard pool let (pool, _) = setup.deploy_standard_pool(&token_a.address, &token_b.address, 30); // Initialize the pool with initial liquidity token_a_admin.mint(&setup.admin, &1000_0000000); token_b_admin.mint(&setup.admin, &1000_0000000); pool.deposit( &setup.admin, &Vec::from_array(&setup.env, [1000_0000000, 1000_0000000]), &0, ); // Create a user let user = Address::generate(&setup.env); // Mint tokens to user with significantly imbalanced amounts token_a_admin.mint(&user, &1000_0000000); // User has 1000 of token A token_b_admin.mint(&user, &1_0000000); // User has only 1 of token B // Set a high min_a value that should reject the transaction if working correctly let min_shares = 500_0000000; // User expects at least 500 shares // The deposit should succeed despite the imbalance causing amount_a to be below min_shares // This demonstrates the bug: the check compares desired_a to min_a, not amount_a to min_a let (amounts, shares) = pool.deposit( &user, &Vec::from_array(&setup.env, [1000_0000000, 1_0000000]), &min_shares, ); // Assert that the actual amount of token A deposited is less than min_shares assert!(amounts.get(0).unwrap() < min_shares, "The deposit succeeded with token A amount below min_shares, demonstrating the bug"); // Print the values for verification println!( "Deposited token A: {}, min_shares: {}, actual shares: {}", amounts.get(0).unwrap(), min_shares, shares ); } #[test] fn bypasses_min_a_check() { // ── Arrange ─────────────────────────────────────────────────────────────── let e = Env::default(); // pool reserves let reserve_a: u128 = 1_000; let reserve_b: u128 = 1_000; // user-supplied parameters let desired_a: u128 = 1_000; // sender thinks they will deposit 1000 A let min_a: u128 = 500; // sender's explicit lower bound let desired_b: u128 = 1; // tiny amount of B let min_b: u128 = 0; // no bound on B // ── Act ─────────────────────────────────────────────────────────────────── // call the vulnerable helper let (actual_a, actual_b) = get_deposit_amounts( &e, desired_a, min_a, desired_b, min_b, reserve_a, reserve_b, ); // ── Assert ──────────────────────────────────────────────────────────────── // The bug allows deposit of only 1 A (≪ min_a) assert_eq!(actual_a, 1); assert_eq!(actual_b, 1); // Sanity check that the min-bound is violated assert!(actual_a < min_a, "slippage bound NOT enforced"); // Output the results println!("desired_a: {}, min_a: {}, actual_a: {}", desired_a, min_a, actual_a); println!("The test demonstrates that the deposit succeeds with actual_a ({}) < min_a ({})", actual_a, min_a); } ``` Result: ``` Finished `test` profile [unoptimized + debuginfo] target(s) in 1.42s Running unittests src/lib.rs (target/debug/deps/integration_tests-725b1e496c8cecaf) running 2 tests test test_bypasses_min_a_check::test_bypasses_min_a_check ... ignored desired_a: 1000, min_a: 500, actual_a: 1 The test demonstrates that the deposit succeeds with actual_a (1) < min_a (500) test test_bypasses_min_a_check::bypasses_min_a_check ... ok ``` ## Recommendation Replace the faulty predicate with a direct check against amount_a: ``` let amount_a = desired_b.fixed_mul_floor(&e, &reserve_a, &reserve_b); if amount_a > desired_a || amount_a < min_a { panic_with_error!(e, LiquidityPoolValidationError::InvalidDepositAmount); } ``` Add regression tests that assert a revert whenever amount_a < min_a or amount_b < min_b.

JohnLaw

Public reviews