AquaNetwork Aquarius
KOSEC participated in a code review with Cantina to audit the AquaNetwork Aquarius AMM.
Summary
The deposit function in /liquidity_pool/src/contract.rs is vulnerable to a Denial-of-Service (DoS) attack due to an unchecked conversion from u128 to i128. A user can provide an extremely large u128 value for the desired token amount to trigger an integer overflow when it is cast to i128. One of the conversions with the missing check can be seen at line 272 of /liquidity_pool/src/contract.rs.
Finding Description
The deposit function in the LiquidityPool contract allows users to provide desired amounts of tokens to add liquidity. When the contract attempts to transfer tokens from the user, it uses the transfer in the SorobanTokenClient, which expects an i128 for the amount. The conversion from u128 to i128 for desired_a and desired_b is performed without any bounds checking at lines 272 and 273 in /liquidity_pool/src/contract.rs.
token_a_client.transfer(&user, &e.current_contract_address(), &(desired_a as i128));token_b_client.transfer(&user, &e.current_contract_address(), &(desired_b as i128));
The u128 type can hold values from 0 to (2**(n)) - 1 while i128 can hold values from -2**(n-1) to (2**(n-1)) - 1. If a u128 value greater than (2**(n-1)) - 1 is cast to i128, it results in an integer overflow.
Impact Explanation
The bug has a medium impact considering exploitation results in a DoS. Since any user can trigger this overflow, the only transaction that fails is their own and the ultimate result is a reverted transaction. Despite this, the LiquidityPool contract does still is pass an invalid argument to an external call due to an internal type conversion flaw. While the behavior of Soroban does have safeguards preventing the issue from escalating further, relying on an external contract for error handling on an internal integrity issue is not ideal.
Likelihood Explanation
The likelihood of this issue occurring is a medium since the value of u128::MAX is unrealistically high, however, the ability to trigger this overflow can be directly done via direct user input or potentially the Aquarius Web UI.
Proof of Concept
The following test demonstrates the vulnerability by attempting to deposit u128::MAX - 1 as one of the desired token amounts. This causes the transaction to fail due to a negative amount being passed to the token transfer function.
#[test]fn test_integration_poc() { let setup = Setup::default();
// create tokens let mut tokens = std::vec![ create_token_contract(&setup.env, &setup.admin).address, create_token_contract(&setup.env, &setup.admin).address, create_token_contract(&setup.env, &setup.admin).address, ]; tokens.sort(); let xlm = TokenClient::new(&setup.env, &tokens[0]); let usdc = TokenClient::new(&setup.env, &tokens[1]); let usdt = TokenClient::new(&setup.env, &tokens[2]);
let xlm_admin = get_token_admin_client(&setup.env, &xlm.address); let usdc_admin = get_token_admin_client(&setup.env, &usdc.address); let usdt_admin = get_token_admin_client(&setup.env, &usdt.address);
// deploy pools let (standard_pool, standard_pool_hash) = setup.deploy_standard_pool(&xlm.address, &usdc.address, 30); xlm_admin.mint(&setup.admin, &344_000_0000000); usdc_admin.mint(&setup.admin, &100_000_0000000);
let test_deposit: u128 = u128::MAX - 1;
standard_pool.deposit( &setup.admin, &Vec::from_array(&setup.env, [344_000_0000000, test_deposit]), &0, );}
Below is the CLI Output demonstrating the negative amount is rejected in Event 5:
---- tests::test_integration_poc stdout ----
thread 'tests::test_integration_poc' panicked at /Users/v0ldemort/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/soroban-env-host-22.1.3/src/host.rs:847:9:HostError: Error(Contract, #8)
Event log (newest first): 0: [Diagnostic Event] topics:[error, Error(Contract, #8)], data:"escalating error to panic" 1: [Diagnostic Event] topics:[error, Error(Contract, #8)], data:["contract call failed", deposit, [CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM, [3440000000000, 340282366920938463463374607431768211454], 0]] 2: [Failed Diagnostic Event (not emitted)] contract:CCL76SC6HJMM34D7MYKIMGJYMODAQYDNAEL7BZB4PVOIHCOXIHBPXUBZ, topics:[log], data:["VM call trapped with HostError", deposit, Error(Contract, #8)] 3: [Failed Diagnostic Event (not emitted)] contract:CCL76SC6HJMM34D7MYKIMGJYMODAQYDNAEL7BZB4PVOIHCOXIHBPXUBZ, topics:[error, Error(Contract, #8)], data:"escalating error to VM trap from failed host function call: call" 4: [Failed Diagnostic Event (not emitted)] contract:CCL76SC6HJMM34D7MYKIMGJYMODAQYDNAEL7BZB4PVOIHCOXIHBPXUBZ, topics:[error, Error(Contract, #8)], data:["contract call failed", transfer, [CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM, CCL76SC6HJMM34D7MYKIMGJYMODAQYDNAEL7BZB4PVOIHCOXIHBPXUBZ, -2]] 5: [Failed Diagnostic Event (not emitted)] contract:CCF7HWZ6PBT2MQ6AUBKXM4CPWAR2PO6ND34WXLB32NDAYD7KIKBAAS7I, topics:[error, Error(Contract, #8)], data:["negative amount is not allowed", -2]
(...)
Recommendation
There should be an explicit bounds check on the u128 input amounts before they are cast to i128. This will prevent overflows and transaction failures. Below is a re-write of the lines 272 and 273 in /liquidity_pool/src/contract.rs with the suggested fix.
let desired_a_i128: i128 = match desired_a.try_into() { Ok(val) => val, Err(_) => panic_with_error!(&e, LiquidityPoolValidationError::InvalidDepositAmount),};
let desired_b_i128: i128 = match desired_b.try_into() { Ok(val) => val, Err(_) => panic_with_error!(&e, LiquidityPoolValidationError::InvalidDepositAmount),};
token_a_client.transfer(&user, &e.current_contract_address(), &desired_a_i128);token_b_client.transfer(&user, &e.current_contract_address(), &desired_b_i128);