sammy - A validator cannot remove a delegator before joining the committee, which can lead to a constant DoS attack

[sherlock-admin4]

sammy

High

A validator cannot remove a delegator before joining the committee, which can lead to a constant DoS attack

Summary

A validator cannot remove a delegator bonded to them before joining a committee. This prevents a validator from controlling their jurisdiction beforehand and can lead to a DoS.

Vulnerability Detail

The withdraw[] mapping for the validator is set only when the validator joins the committee : credits.aleo#L245-L269

// if (new_validator)
    contains committee[r0] into r16;
    branch.eq r16 true to validator_in_committee;
    // {
        // Set the withdrawal address.
        // Note: This operation is only necessary on the first time for a validator entry, in order to initialize the value.
        set r4 into withdraw[r0];

        // Check if the initial bond amount is at least 100 credits.
        gte r2 100_000_000u64 into r17;
        // Ensure that the initial bond is at least 100 credits.
        assert.eq r17 true;

        // Get the committee size.
        get.or_use metadata[aleo1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq3ljyzc] 0u32 into r18;
        // Increment the committee size by one.
        add r18 1u32 into r19;
        // Set the new committee size.
        set r19 into metadata[aleo1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq3ljyzc];

        // Check if the validator exists in the unbonding state.
        contains unbonding[r0] into r20;
        // Ensure the validator currently is not unbonding.
        assert.eq r20 false;
    // }

Because of this, the validator cannot pass the following check in unbond_public, which causes a revert :

 // Check if the validator's withdrawal address has been set.
    // Note: This contains check must use `withdraw` and **not** `committee` to ensure the validator
    // can unbond delegators even when the validator is not in the committee. Of course, if the validator is
    // in the committee, then the validator must be able to unbond its delegators.
    contains withdraw[r5.validator] into r8;
    // Get the validator's withdrawal address from the bond state, using the zero address as the default.
    get.or_use withdraw[r5.validator] aleo1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq3ljyzc into r9;
    // Check if the validator's withdrawal address matches the caller.
    is.eq r0 r9 into r10;
    // AND the withdraw address has been set (to prevent an edge case where a validator uses the zero address as a withdrawal address).
    and r8 r10 into r11;

    // Either the caller is the staker's withdrawal address OR the validator's withdrawal address is set to the caller.
    or r7 r11 into r12;
    // Enforce the permission as `true`.
    assert.eq r12 true;

Every time a validator is unbonded, they need to wait 360 blocks before they can join the committee again. When they claim their unbonded assets, the withdrawal address is removed again, which means they cannot unbond the malicious delegators, which makes it possible for malicious delegators to front-run the validator and perform a constant DoS attack.

Impact

This can lead to a DoS when the validator self-bonds and joins the committee for the first time. Consider the following attack scenario :

1. Malicious delegators bond to the validator, increasing the total delegated amount beyond 10M credits
2. The validator cannot unbond the malicious delegators because withdrawal mapping has not been set
3. Validator self-bonds with 100 credits and joins the committee
4. Delegators pull their bonds through unbond_public and force the validator to leave the committee
5. The validator is restricted from joining the committee again for 360 blocks, and also no other delegators can bond to the validator during this time
6. As soon as the validator finishes unbonding, the malicious delegators can front-run the validator and repeat the same attack as their funds have finished unbonding as well.

This is also an issue for honest delegators because they will not earn rewards if the validator is not in the committee. Hence, they may be disincentivized from delegating their funds to the validator.

Coded POC

#[test]
fn test_poc() {
    let rng = &mut TestRng::default();

    // Construct the process.
    let process = Process::<CurrentNetwork>::load().unwrap();
    // Initialize a new finalize store.
    let (store, _temp_dir) = sample_finalize_store!();

    // Initialize the validator and delegator keys
    let validator_private_key = PrivateKey::<CurrentNetwork>::new(rng).unwrap();
    let validator_address = Address::try_from(&validator_private_key).unwrap();
    let withdrawal_private_key = PrivateKey::<CurrentNetwork>::new(rng).unwrap();
    let withdrawal_address = Address::try_from(&withdrawal_private_key).unwrap();
    let delegator_private_key = PrivateKey::<CurrentNetwork>::new(rng).unwrap();
    let delegator_address = Address::try_from(&delegator_private_key).unwrap();
    let malicious_delegator_private_key = PrivateKey::<CurrentNetwork>::new(rng).unwrap();
    let malicious_delegator_address = Address::try_from(&malicious_delegator_private_key).unwrap();

    // Initialize the account balances
    let validator_balance = 1_000_000_000u64; // 1,000 credits
    let delegator_balance = 100_000_000_000_000u64;
    initialize_account(&store, &validator_address, validator_balance).unwrap();
    initialize_account(&store, &delegator_address, delegator_balance).unwrap();
    initialize_account(&store, &malicious_delegator_address, delegator_balance).unwrap();

    let delegator_amount = MIN_VALIDATOR_STAKE - 20_000_000_000u64;
    let validator_amount = MIN_VALIDATOR_SELF_STAKE;
    let malicious_amount: u64 = 20_000_000_000u64;
    // Sanity check the state before finalizing.
    assert_eq!(committee_state(&store, &validator_address).unwrap(), None);
    assert_eq!(delegated_state(&store, &validator_address).unwrap(), None);
    assert_eq!(bond_state(&store, &validator_address).unwrap(), None);
    assert_eq!(unbond_state(&store, &validator_address).unwrap(), None);
    assert_eq!(withdraw_state(&store, &validator_address).unwrap(), None);
    assert_eq!(account_balance(&store, &validator_address).unwrap(), validator_balance);
    assert_eq!(account_balance(&store, &delegator_address).unwrap(), delegator_balance);

    /*
    1. Delegator bonds to validator before validator is in the committee
    2. Validator can then bond_validator to join the committee
    */
    test_atomic_finalize!(store, FinalizeMode::RealRun, {
        //honest delegators bond to the validator
        bond_public(
            &process,
            &store,
            &delegator_private_key,
            &validator_address,
            &delegator_address,
            delegator_amount,
            rng,
        )
        .unwrap();

        // maliious delegator bonds to the validator
        bond_public(
            &process,
            &store,
            &malicious_delegator_private_key,
            &validator_address,
            &malicious_delegator_address,
            malicious_amount,
            rng,
        )
        .unwrap();
        // Check that the committee, bond, unbond, and withdraw states are correct.
        assert_eq!(committee_state(&store, &validator_address).unwrap(), None);
        assert_eq!(delegated_state(&store, &validator_address).unwrap(), Some(delegator_amount + malicious_amount));

        // Perform the bond validator with the minimum self bond
        bond_validator(
            &process,
            &store,
            &validator_private_key,
            &withdrawal_address,
            validator_amount,
            TEST_COMMISSION,
            rng,
        )
        .unwrap();

        // malicious delegator unbonds all of their bond from the validator
        unbond_public(
            &process,
            &store,
            &malicious_delegator_private_key,
            &malicious_delegator_address,
            malicious_amount,
            1u32,
            rng,
        )
        .unwrap();

        // as a result, the validator is removed from the committee
        assert_eq!(committee_state(&store, &validator_address).unwrap(), None);

        // malicious delegator claims their unbonded amount after 360 blocks
        claim_unbond_public(
            &process,
            &store,
            &malicious_delegator_private_key,
            &malicious_delegator_address,
            361u32,
            rng,
        )
        .unwrap();

        //Note that new delegators will not be able to bond to the validator while it is in unbonding state

        // validator claims their unbonded amount after 360 blocks (this can also be done by the malicious delegator on behalf of the validator)
        claim_unbond_public(&process, &store, &validator_private_key, &validator_address, 361u32, rng).unwrap();

        // NOTE : Since the delegator force unbonded the validator by completely unbonding their stake, the validator cannot block the
        // malicious delegator from bonding to them again. This is a limitation of the current design.

        // malicious delegator front runs the validator and bonds to the validator
        bond_public(
            &process,
            &store,
            &malicious_delegator_private_key,
            &validator_address,
            &malicious_delegator_address,
            malicious_amount,
            rng,
        )
        .unwrap();

        // Validator detects malicious activity, but cannot unbond the malicious delegator because the validator's
        // withdrawal address was removed in the claim unbond call. Also, a validator cannot unbond a delegator
        // before having a withdrawal address (which requires joining the committee)

        // validator tries unbonding the malicious delegator

        let result =
            unbond_public(&process, &store, &validator_private_key, &malicious_delegator_address, 0, 362u32, rng);

        // the call fails
        assert!(result.is_err());

        // validator self bonds
        bond_validator(
            &process,
            &store,
            &validator_private_key,
            &withdrawal_address,
            validator_amount,
            TEST_COMMISSION,
            rng,
        )
        .unwrap();

        // malicious delegator repeats the attack
        unbond_public(
            &process,
            &store,
            &malicious_delegator_private_key,
            &malicious_delegator_address,
            malicious_amount,
            363u32,
            rng,
        )
        .unwrap();

        // validator is removed from the committee
        assert_eq!(committee_state(&store, &validator_address).unwrap(), None);

       // the malicious delegator can just keep repeating this attack

        Ok(())
    })
    .unwrap();
}

To run the test :

1. Copy the above code into test_credits.rs
2. Click on Run Test

   Code Snippet

Tool used

Manual Review

Recommendation

Do not remove the validator's withdrawal address when they claim their unbonded assets. This will give them enough time to remove any malicious delegators. Alternatively, allow validators to remove delegators when their withdrawal address is not set (withdrawal address is not set before they join the committee for the first time)

[evanmarshall]

This is not a real issue as the malicious delegators would have to be the reason the validator could call bond_validator in the first place. If malicious delegators were the difference to be above the 10M credits threshold and validators could unbond them before trying to call bond_validator then by removing them, the validator still couldn't call bond_validator as they wouldn't be above 10M credits.

[Haxatron]

Escalate

I still think this is a real issue because of the following comment:

https://github.com/sherlock-audit/2024-05-aleo/blob/main/snarkVM/synthesizer/program/src/resources/credits.aleo#L456-L458

    // Note: This contains check must use `withdraw` and **not** `committee` to ensure the validator
    // can unbond delegators even when the validator is not in the committee. Of course, if the validator is
    // in the committee, then the validator must be able to unbond its delegators.

So the core contract functionality is broken

[sherlock-admin3]

Escalate

I still think this is a real issue because of the following comment:

https://github.com/sherlock-audit/2024-05-aleo/blob/main/snarkVM/synthesizer/program/src/resources/credits.aleo#L456-L458

    // Note: This contains check must use `withdraw` and **not** `committee` to ensure the validator
    // can unbond delegators even when the validator is not in the committee. Of course, if the validator is
    // in the committee, then the validator must be able to unbond its delegators.

So the core contract functionality is broken

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Haxatron]

To elaborate: The DoS vector is invalid but the fact that you can't unbond the delegator while not in the committee is valid because of the code comment above.

Please check report #9

[evanmarshall]

That's fair. The comment seems incorrect or at least confusing.

Technically, you could still unbond by delegating to a different validator to create the withdrawal address without becoming a validator yourself -> you need a withdrawal address but don't want to be a validator so become a delegator. Then with 10K credits you could unbond people but then you have to wait 360 blocks to switch to a validator.

All in all, I agree the comment should be fixed even though technically may be considered correct.

[sammy-tm]

@evanmarshall

By the method you mentioned there is no guarantee that no one will call the claim_unbond function on behalf of the user (since it's permissionless) and remove the withdrawal address before they decide to unbond delegators.

This is a real issue and validators should be allowed to unbond delegators before joining committee for the first time.

This is essential to remove any 1 centralised entity from having control over the validator.

For example,

Any user that has a large number of credits (especially compared to a potential validator) can easily control a validator's availability. Check the given example in the report but assume that the delegator has a much larger amount of credits

[evanmarshall]

I don't agree that it's a real issue. The unbonding mapping check is only for validators so a validator calling bond_validator needs to have no unbounding state. If a validator is only able to reach the 10M credit threshold by a single entity, that single entity by definition controls when that validator gets to be part of the committee. That entity doesn't need to unbond to prevent the validator from joining the committee, they can simply not delegate and then the validator won't be able to join the committee.

Example 1:

1. Validator V only has 100 credits and Delegator D has 10M credits.
2. Delegator D never delegates, V can never join the committee

Example 2:

1. Validator V has 10M credits and Delegator D has 10M credits
2. Delegator D bond_public 10M to V
3. V bond_validator with 10M credits Nothing D does will remove V from the committee

In general, any delegator that can put the total delegation for a validator under 10M has the ability to remove the validator from the committee.

[Haxatron]

Considering that the comment clearly says the "validator can unbond delegators even when the validator is not in the committee" is an intended functionality, I still do think that this issue should be accepted.

Because otherwise, it would be a guessing game on whether a functionality should be intended or not. But I will leave this for HoJ to decide. 😊

[morbsel]

I also think it should be a valid medium issue because it's clearly stated in the comments that a validator should be able to unbond delegators even when the validator is not in the committee: https://github.com/sherlock-audit/2024-05-aleo/blob/55b2e4a02f27602a54c11f964f6f610fee6f4ab8/snarkVM/synthesizer/program/src/resources/credits.aleo#L455-L458

The code comment according to Sherlock's hierarchy of truth should stand above all judging rules: https://docs.sherlock.xyz/audits/judging/judging#iii.-sherlocks-standards

Note: my issue #26 is also affected by this, which should be valid in my opinion.

[infosecual]

I do not understand the "malicious delegators" verbiage. Delegators are in one of two categories->

1. They are needed for a validator to reach the 10M credit limit and be part of the committe. In this scenario the protocol wants them to have power over the validator (eg. they can remove their stake and thus remove the validator). The delegator is beneficial to the validator as it could not validate without them. This is a "you would be nothing without me" situation so I don't think you can say a delegator can be malicious here.
2. They are unneeded by the validator to reach the 10M credit limit. In this scenario they have no power over the validator. They are beneficial only in their commission. They are not a harm to the validator in any way. They cannot make the validator unbond or anything. Validators can close them out with set_validator_state if they feel inclined.

I do not see a scenario where a delegator could be malicious. I agree with Evan and Haxatron that the mentioned DOS scenario is not real.

It seems that Haxatron and Evan are on the same page that the issue here is the comment is misleading, just disagreeing a bit on the severity. I am not familiar enough with Sherlock judging rules but I am not sure that an issue which boils down to "the comment is incorrect and needs fixing" has enough impact to be a valid medium. The issue is the comment not the code so it seems the impact is not sufficient in my opinion.

[Haxatron]

That is fair. I am also not sure how "code comment states functionality ABC should be supported but functionality ABC is not supported" is judged under Sherlock's guidelines. Here is the relevant rule from the Sherlock guideline on comments:

https://docs.sherlock.xyz/audits/judging/judging#iii.-sherlocks-standards

If the protocol team provides specific information in the README or CODE COMMENTS, that information stands above all judging rules. In case of contradictions between the README and CODE COMMENTS, the README is the chosen source of truth.

The judge can decide that CODE COMMENTS are outdated based on contextual evidence. In that case, the judging guidelines are the chosen source of truth.

Example: The code comments state that "a swap can never fail" even though the code is built to support failing swaps.

In this case, I do not believe that there is contextual evidence that can be used to determine that the "validator shouldn't be able to unbond a delegator when not in the committee" comment is outdated. But I do understand that this is very subjective and will leave it to head of judging to decide.

[morbsel]

I do not see a scenario where a delegator could be malicious.

The issue isn't about if a delegator could be malicious (at least not in my issue #26), it's about that validators that are not in the committee can't unbond their delegators for whatever reason they want.

It is clearly intended and stated in the comments that validators should be able to unbond their delegators, even with an additional comment above the caller validation part where it is explicitly stated that validators that are not in the committee should be able to unbond there delegators, which makes it seam like it is important and part of the core functionality.

It tries to explicitly use "withdraw" instead of "committee" to get around the issue but in the end it still won't work how it was intended.

Based on that I will go with Haxatron that there isn't contextual evidence that the comment is outdated and therefore the comment should stand above all judging rules, according to Sherlock's rules and hierarchy of truth.

[cvetanovv]

Even if the NatSpec comment is confusing, does it meet the criteria for Medium or High severity? - reference Clearly, this is expected behavior, and the only fix the protocol will make is to rewrite the NatSpec comment to make it clearer.

You quote the documentation but skip the most important quote, which makes this issue invalid:

"The protocol team can use the README (and only the README) to define language that indicates the codebase's restrictions and/or expected functionality. Issues that break these statements, irrespective of whether the impact is low/unknown, will be assigned Medium severity."

When we look at such restrictions and expected behavior, we only look at Readme.

If the same NatSpec comment had it in the Readme, then this issue would be valid because we wouldn't be looking at an impact. In this case, we are looking at the NatSpec comment, and according to the sponsor, everything works as it should.

[Haxatron]

I do not want to argue based on literal interpretations of rules and I don't mind whether this issue is marked valid or not. However, I do believe clarification of rules is important, especially in Sherlock which is a rules-based platform, for future reference. I would like to clarify with the head of judging:

1) What is the purpose of the CODE COMMENTS paragraph then? In what scenario would the CODE COMMENTS matter, which would make the issue valid. It seems contradictory to say that the CODE COMMENTS don't matter at all and have a paragraph about it in the rule guidelines.

2) Related to point 1, especially for this particular audit, it was difficult to determine what the expected functionality of the codebase was. If we go by @cvetanovv interpretation, then it means only the README can be used to define what the expected functionality is. Valid issues such as #31 which demonstrates that the expected functionality is broken (without any loss of funds) would be marked as invalid because what the expected functionality (that a delegator should be able to partially unbond even when their withdrawal address is set to the same as the validator withdrawal address) is was not covered neither in the README nor CODE COMMENTS, and can only be determined by the sponsors input. Obviously, this shouldn't be the case and some clarification on the rules is required here.

3) Based on this, my interpretation of the rules differ from @cvetanovv, I interpret it as the following: for issues that do not show a medium / high impact (do not show loss of expected functionality, loss of funds, DoS of more than 1 week), if it still goes against what is stated in the README, then it can be a medium issue, and I think this is aligned with the spirit of the README rule which was recently introduced based on the rationale here: https://github.com/sherlock-protocol/governance-and-discussion/discussions/5. It does not mean that the CODE COMMENTS cannot be used to define the expected functionality of the protocol.

[morbsel]

Clearly, this is expected behavior, and the only fix the protocol will make is to rewrite the NatSpec comment to make it clearer.

Sorry but It's clearly not an expected behavior, they obviously want validators that aren't in the committee to unbond delegators, they explicitly use withdraw mapping instead of committee mapping, despite using the withdraw mapping the functionality still doesn't work as intended.

// Check if the validator's withdrawal address has been set. // Note: This contains check must use withdraw and not committee to ensure the validator // can unbond delegators even when the validator is not in the committee. Of course, if the validator is // in the committee, then the validator must be able to unbond its delegators. contains withdraw[r5.validator] into r8;

I find it remarkable that we are even having a discussion about whether it is expected or not. Also the protocol team has provided the information in the code comments that I still believe should be treated as source of truth.

If the protocol team provides specific information in the README or CODE COMMENTS, that information stands above all judging rules. In case of contradictions between the README and CODE COMMENTS, the README is the chosen source of truth.

[sammy-tm]

I can also point out an edge case in which removing a delegator before having a withdrawal address set will be important.

The max number of delegators allowed is 100,000

Now if the maximum limit for delegators is reached, there can be a scenario where validators are not able to join the committee because they cannot remove delegators before they have a withdrawal address.

Let's imagine this scenario: 1) Some of of those 100,000 delegators are bonded to validator A 2) The total delegated amount for A is 4M credits 3) A's balance is 3M credits 4) No new delegators can bond to A because the max limit is reached, also the validator cannot set the withdrawal address because they can't join the committee (as total delegated amount would be 7M after self bonding) 5) A cannot allow delegators that have higher stake to bond (which would push the delegated amount above 10M, hence not being able to become a validator

If this was mitigated, the validator would be able to remove delegators with lower stake and be able to replace them with delegators that can bond with a higher stake to it.

[WangSecurity]

@evanmarshall to clarify, are these comments in the escalation comment outdated and the protocol is not intended to follow them?

[sammy-tm]

I think the scenario I mentioned above supports that this is indeed intended. In that case, code should be treated as the source of truth.

TLDR; Validator cannot join the committee simply because it's not allowed to remove low bond delegators at max delegators case.

[WangSecurity]

Yep, I see, but still want to confirm if these comments are outdated or not, cause unsure about it.

[WangSecurity]

@evanmarshall could you check this comment?