minimum_stake doesn't check the user's total stake amount, which prevents the user's next stake if the stake amount < minimum_stake

[hats-bug-reporter[bot]]

Github username: @sonny2k Twitter username: 0xNGOC Submission hash (on-chain): 0xbbd69410245515d489ddc431d824447c4905ea245650f466b592f1fb9029c469 Severity: medium

Description: Description\ In vault/lib.rs, there is a validation that the stake amount must be larger than minimum_stake. However, when the user decides to stake an additional amount adding up to the previous stake amount, if the second stake amount is lesser than minimum_stake, the call will be reverted. Although the total stake the user wants to make is larger than the minimum_stake itself.

            // Verify minimum AZERO is being staked
            if azero < self.data.minimum_stake {
                return Err(VaultError::MinimumStake);
            }

Attack Scenario\ As the Aleph Zero's documents states:

Of course that doesn't mean you cannot change your staking choices - you can do it at any moment, but the changes you introduce (for example, adding more stake or changing a validator you nominate) will become effective only at the start of the next era. To be precise, you need to submit your changes at most 15 minutes (900 blocks) before the start of a new era for your changes to have an effect during that era.

Suppose Alice wants to stake 500 $AZERO and the minimum_stake value is 100, but for now, she only has 450 $AZERO remaining in the wallet and she decides to stake it anyway. After 5-10 minutes, she realizes that she also has like 50 $AZERO left on the CEX for example, and wants to stake that amount adding to the original amount. However, her tx is reverted as the second amount is < minimum_stake (50 < 100).

Impact\ The user has to wait for 14 days or more to stake the whole amount, although it should have been able to stake the additional amount within the era when the user first stakes.

Attachments

1. Proof of Concept (PoC) File Add this test case into drink_tests/lib.rs to see how the transaction panics when Alice stakes the second time:

   #[test]
   fn test_stake_panic_when_user_stake_more_but_the_second_amount_lesser_than_minimum_stake() {
       let ctx: TestContext = setup().unwrap();
       let sess = ctx.sess;
   
       // Fetch default minimum_stake
       let (minimum_stake, sess) = helpers::query_minimum_stake(sess, &ctx.vault).unwrap();
   
       // As default minimum_stake is 0, Alice can stake 10 gas tokens
       let (_, sess) = helpers::call_stake(sess, &ctx.vault, &ctx.share_token, &ctx.alice, minimum_stake + 10).unwrap();
   
       // Increase minimum_stake to 100
       let sess = helpers::call_function(
           sess,
           &ctx.vault,
           &ctx.bob, // owner
           String::from("adjust_minimum_stake"),
           Some(vec![(minimum_stake + 100).to_string()]),
           None,
           helpers::transcoder_vault(),
       )
           .unwrap();
   
       match helpers::call_stake(sess, &ctx.vault, &ctx.share_token, &ctx.alice, minimum_stake + 100 - 5) {
           Ok(_) => panic!("The additional stake action from Alice is panic as it's lesser than the new minimum_stake (95 < 100). 
           However, in reality, Alice should be able to stake more since her previous amount + this stake amount is larger than the new 
           minimum_stake(10 + 95 = 105 > 100)"),
           Err(_) => (),
       };
   }

2. Revised Code File (Optional)\ Consider adding a variable to store the user's total stake amount and comparing that with the minimum_stake to avoid preventing the user ability to add stake multiple times ( because the total stake amount satisfies the minimum_stake validation).

[0xmahdirostami]

Thank you for your submission. Just like issue #27 . The minimum stake requirement is not meant to force users to have more than that; it's meant for bonding more than that, and it works as intended.

[sonny2k]

Thanks for your reply, however, when staking into vault, the user must actually have the amount of gas token to be staked > minimum_stake. I'm talking about the scenario when the user's next stake getting blocked by minimum_stake validation. Although if you add the current stake amount with the previous stake amount, it would be larger than the minimum_stake. Can you check this again?

[0xmahdirostami]

@sonny2k The minimum stake that can go through a nomination pool is 10 AZERO. Because of this, the contract checks if azero < self.data.minimum_stake {return Err(VaultError::MinimumStake);}. It's not important how much is user's staked amount. Each time the user wants to stake, it must be bigger than 10 AZERO. each time a user stakes, this amount goes through a nomination pool, so it must be bigger than minimum_stake .

[sonny2k]

I remember that nominations pool in Aleph Zero allows bond_extra for example, what if user A 1st stake is 10 $AZERO, and 5 minutes later he wants to add 5 more $AZERO. But it fails since the 2nd stake amount is < minimum_stake which is 10. If this is not the case then why the protocol has a function which compounds rewards to the current stake amount, regardless of the fact that the rewards could be smaller than minimum_stake? It's because the current stake already satisfies the minimum_stake validation, just like my case, where user A's 1st stake has already satisfied the minimum_stake validation.

[0xmahdirostami]

@sonny2k

Consider the following scenario:

The owner added 1 nomination agent. User A stake 20 AZERO.

After some time, a new nomination agent was added. If this is your case and the user satisfies the minimum_stake, the user can stake below that, but the transaction reverts as the new pool hasn't joined yet.

So it's better to always check for this stake amount. Each time It must be bigger than a minimum_stake.

All users are considered the same as some agents, so it's not about each user's stake amount, it's about the agent's stake amount. As always we could have new agents, and we could withdraw and join again, it's better to check it each time.

However, if you think i missed something, please comment again.

[bgibers]

@sonny2k The minimum stake for each stake within our protocol is always 10 AZERO. The reason being, that the minimum bond for a nomination pool is 10 AZERO and we don't check balance on a nomination pool. Its easier to just enforce the minimum at all times to meet network limitations. @0xmahdirostami hit the nail right on the head with this explanation

@sonny2k

Consider the following scenario:

The owner added 1 nomination agent. User A stake 20 AZERO.

After some time, a new nomination agent was added. If this is your case and the user satisfies the minimum_stake, the user can stake below that, but the transaction reverts as the new pool hasn't joined yet.

So it's better to always check for this stake amount. Each time It must be bigger than a minimum_stake.

All users are considered the same as some agents, so it's not about each user's stake amount, it's about the agent's stake amount. As always we could have new agents, and we could withdraw and join again, it's better to check it each time.

However, if you think i missed something, please comment again.