Storage can be bloated with low liquidity positions

[hats-bug-reporter[bot]]

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

Description: Description\ When using the substrate framework, it is one of the main goals of developers to prevent storage bloat. If storage can easily be bloated by users, this can lead to high costs for the maintainers of the chain and a potential DOS. A more in detail explanation can be found here

In lib.add_liquidity, the function calls math::rated_compute_lp_amount_for_deposit to calcualte the share and fee_part. Please note that in mod.compute_lp_amount_for_deposit, if the pool_token_supply is not zero, the function will not check if deposit_amounts contains 0, and the function doesn't check if the return value share and fee_part are zero

Next back to lib.add_liquidity, after calculating the share and fee_part, token are transferred in lib.rs#L615-L622, there is also no check if the amounts[i] is zero.

Afterward, the function calls mint to mint LP token for the caller, and in the mint function, the function checks if the amount is zero.

    /// Mints a `value` of new tokens to `to` account.
    pub fn mint(&mut self, to: AccountId, value: u128) -> Result<Vec<PSP22Event>, PSP22Error> {
        if value == 0 {
            return Ok(vec![]);
        }
        let new_supply = self
            .total_supply
            .checked_add(value)
            .ok_or(PSP22Error::Custom(String::from(
                "Max PSP22 supply exceeded. Max supply limited to 2^128-1.",
            )))?;
        self.total_supply = new_supply;
        let new_balance = self.balance_of(to).saturating_add(value);
        self.balances.insert(to, &new_balance);
        Ok(vec![PSP22Event::Transfer {
            from: None,
            to: Some(to),
            value,
        }])
    }

Which means that if a malicious user can mint 1 share of LP token

Attack Scenario\ Please consider a case that a malicious user can create lots of accounts, and for each account, he calls add_liquidity to deposit 1 share of LP token, which will lead to high maintenance costs for the chain and a potential DOS

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

[JanKuczma]

Thank you for your submission.

The explanation you have referenced explains how to prevent it on the chain (transaction) level. Because any contract interaction is a transaction, the issue is already mitigated by the blockchain design (weights system).

[web3layman000]

Hi @JanKuczma ,thanks for your judging. Please check https://github.com/code-423n4/2024-02-hydradx-findings/issues/54#issuecomment-1996571933 and https://github.com/code-423n4/2024-02-hydradx-findings/issues/54#issuecomment-1996595161 . I think those links provide a better explaination

[JanKuczma]

Please check https://github.com/code-423n4/2024-02-hydradx-findings/issues/54#issuecomment-1996571933 and https://github.com/code-423n4/2024-02-hydradx-findings/issues/54#issuecomment-1996595161 .

This protocol is working on the pallet level. StablePool is a smart contract, which implements PSP22. This means that the described Attack Scenario could be done on any PSP22 token - the attacker sends 1 token to lots of newly created accounts.

However, the attacker would have to pay for every transaction gas (computation cost + storage deposit), which makes this attack unfeasible.

[web3layman000]

However, the attacker would have to pay for every transaction gas (computation cost + storage deposit), which makes this attack unfeasible.

Even this makes no profit for the attacker, but this will cost normal users more gas. And this should fit the Gas griefing attacks (make users overpay for gas) in Medium severity levels

[DamianStraszak]

Hi @web3layman000 -- please provide a unit test demontrating the gas griefing attack.

[web3layman000]

Hi @DamianStraszak I can't make test case that can prove the issue directly. But by the following test case, it can prove part of the issue. please run with cargo test --manifest-path ./drink-tests/Cargo.toml stable_swap_tests::tests_add_remove_lp::test_dos -- --nocapture

The following test case does:

1. create lots of accounts by create_account_id_vec function
2. mint little share of lpToken to different accounts created by create_account_id_vec function

As shown by the results, it takes 48.494944975s to run the first 1000 tx, and with the number of txs increase, the longer it takes to run 1000 txs. Which should consistent with Storing data unnecessarily can lead to slow network performance and resources running out.

diff --git a/amm/drink-tests/src/stable_swap_tests/tests_add_remove_lp.rs b/amm/drink-tests/src/stable_swap_tests/tests_add_remove_lp.rs
index 2606cad..81358c3 100644
--- a/amm/drink-tests/src/stable_swap_tests/tests_add_remove_lp.rs
+++ b/amm/drink-tests/src/stable_swap_tests/tests_add_remove_lp.rs
@@ -2,6 +2,7 @@ use drink::{self, session::Session};
 use stable_pool_contract::MathError;

 use super::*;
+use std::time::{Duration, Instant};

 // ref https://github.com/ref-finance/ref-contracts/blob/d241d7aeaa6250937b160d56e5c4b5b48d9d97f7/ref-exchange/tests/test_stable_pool.rs#L123
 #[drink::test]
@@ -1175,6 +1176,7 @@ fn test_lp_withdraw_all_but_no_more() {
         BOB,
     );

+    println!("get_gas_limit: {}", session.get_gas_limit());
     _ = stable_swap::add_liquidity(
         &mut session,
         stable_swap,
@@ -1224,4 +1226,64 @@ fn test_lp_withdraw_all_but_no_more() {
         dave(),
     )
     .expect_err("Should not successfully remove liquidity");
+    println!("get_gas_limit: {}", session.get_gas_limit());
 }
+
+fn create_account_id_vec(cnt: usize) -> Vec<AccountId32> {
+    (0..cnt).map(|i| {
+            let mut bytes = [0u8; 32];
+            let i_bytes = i.to_le_bytes();
+            bytes[0..8].copy_from_slice(&i_bytes);
+            AccountId32::new(bytes)
+        })
+        .collect()
+}
+
+
+#[drink::test]
+fn test_dos(mut session: Session) {
+    seed_account(&mut session, CHARLIE);
+    seed_account(&mut session, DAVE);
+    seed_account(&mut session, EVA);
+
+    let initial_reserves = vec![100000 * ONE_USDT, 100000 * ONE_USDC];
+    let initial_supply = initial_reserves
+        .iter()
+        .map(|amount| amount * 100_000_000_000)
+        .collect::<Vec<u128>>();
+    let (stable_swap, tokens) = setup_stable_swap_with_tokens(
+        &mut session,
+        vec![6, 6],
+        initial_supply.clone(),
+        10_000,
+        2_500_000,
+        200_000_000,
+        BOB,
+        vec![],
+    );
+
+
+    println!("blanceOf: {}", psp22_utils::balance_of(&mut session, stable_swap, charlie()));
+    let cnt: usize = 1_000_000;
+    let accounts = create_account_id_vec(cnt);
+    let start = Instant::now();
+    for i in 0..cnt {
+        if i % 1_000 == 0 {
+            let duration = start.elapsed();
+            println!("xxx_xxx i:{}, duration:{:?}", i, duration);
+        }
+        _ = stable_swap::add_liquidity(
+            &mut session,
+            stable_swap,
+            BOB,
+            1,
+            vec![1, 1],
+            AsRef::<[u8; 32]>::as_ref(&accounts[i]).clone().into(),
+            )
+        .expect("Should successfully add liquidity");
+    }
+
+    let duration = start.elapsed();
+    println!("xxx_xxx i:{}, duration:{:?}", cnt, duration);
+}
+

[DamianStraszak]

Thank you. However, without a valid PoC in the form of a unit test demonstrating a vulnerability, this submission is classified as invalid.

[web3layman000]

Hi @DamianStraszak and @JanKuczma The reason I can't provide a full POC is that I don't know any api that can calculate the cost for executing a tx, if you can help me find those api, I would like to continue the POC

Even though I can't provide the full POC, the poc above can prove: the more add_liquidity is called, the longer it takes to finish a tx, and quoting from A closer look at the inclusion fee

inclusion_fee = base_fee + length_fee + [targeted_fee_adjustment * weight_fee]; final_fee = inclusion_fee + tip;

weight_fee is defined as:

weight fee: A fee proportional to the execution time (input and output and computation) that a transaction consumes.

So my POC can prove that the execution time can be increased by keeping call add_liquidity, which should mean that weight_fee will increase

[DamianStraszak]

weight_fee does not increase in your example

[web3layman000]

can you please tell me how to calculate weight_fee? I can't find the api