sammy - Validator can block existing delegators from increasing their delegations and new delegators to bond at no cost

[sherlock-admin4]

sammy

Medium

Validator can block existing delegators from increasing their delegations and new delegators to bond at no cost

Summary

A validator can block delegators already bonded to the validator from increasing their delegations and new delegators from bonding to the delegator at no cost, because unbond_public allows unbonding of 0 credits

Vulnerability Detail

When a validator is in the committee, the self bond amount is > 100 credits and the total delegated amount is > 10M. The validator can use unbond_public to unbond a certain amount of credits or to force unbond a delegator. However, this function allows the validator to set the amount to unbond to 0 , which means that the validator can initialize the unbond[] mapping with 0 credits.

@=> // There is no previous check for r2, hence it can be zero
        /* Unbonding */

        // Retrieve or initialize the unbonding state.
        get.or_use unbonding[r5.validator] r4 into r38;
        // Increment the unbond amount.
        // Note: If the addition overflows, reject the transition.
        add r38.microcredits r2 into r39;
        // Set the updated unbond state.
        cast r39 r3 into r40 as unbond_state;
        // Store the new unbonding state.
        set r40 into unbonding[r5.validator];

When the unbond[] state is active for the validator, no new delegations can be made to it because of the following check in bond_public

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

This means that no new delegator will be able to bond to the validator, and existing delegators will be denied from increasing their bonds (in order to receive more staking awards).

After 360 blocks, the unbond[] state of the validator can be removed by calling claim_unbond_public

finalize claim_unbond_public:
    // Input the staker's address.
    input r0 as address.public;

    // Get the unbond state for the address, or fail if it does not exist.
    get unbonding[r0] into r1;
    // Determine if unbonding is complete.
    gte block.height r1.height into r2;
    // Enforce the unbonding is complete.
    assert.eq r2 true;

    /* Withdraw */

    // Get the withdrawal address for the address.
    get withdraw[r0] into r3;

    /* Account */

    // Add the unbonded amount to the withdrawal address public balance.
    // Increments `account[r3]` by `r1.microcredits`.
    // If `account[r3]` does not exist, 0u64 is used.
    // If `account[r3] + r1.microcredits` overflows, `claim_unbond_public` is reverted.
    get.or_use account[r3] 0u64 into r4;
    add r1.microcredits r4 into r5;
    set r5 into account[r3];

    /* Unbonding */

    // Remove the unbond state for the staker.
@=>    remove unbonding[r0];

    // Check if the staker is still bonded.
    contains bonded[r0] into r6;
    // Ends the `claim_unbond_public` logic.
    branch.eq r6 true to end;

    /* Withdraw */

    // If the caller is no longer bonded, remove the withdrawal address.
    remove withdraw[r0];

    // The terminus.
    position end;

However, the validator can just repeat the call to unbond_public with 0 as input and cause this DoS for the next 360 blocks.

Impact

DoS. Existing delegators should be able to increase their delegations as long as the validator is in the committee so that they can increase their staking rewards. The benefit that the validator gets from this is that their % share of stake does not fall due to new delegations. Hence, they retain higher staking rewards.

Coded PoC

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

    // Construct the process.
    let process = Process::<CurrentNetwork>::load().unwrap();

    // Initialize a new finalize store.
    let finalize_store = FinalizeStore::<CurrentNetwork, FinalizeMemory<_>>::open(None).unwrap();

    // Initialize the validators and delegators.
    let (validators, delegators) = initialize_stakers(&finalize_store, 2, 1, rng).unwrap();
    let mut validators = validators.into_iter();
    let (validator_private_key_1, (validator_address_1, _, withdrawal_private_key_1, withdrawal_address_1)) =
        validators.next().unwrap();
    let (delegator_private_key, (delegator_address, _)) = delegators.first().unwrap();

    // Bond the validator
    let validator_amount = MIN_VALIDATOR_STAKE;
    bond_validator(
        &process,
        &finalize_store,
        &validator_private_key_1,
        &withdrawal_address_1,
        validator_amount,
        TEST_COMMISSION,
        rng,
    )
    .unwrap();

    // Bond the delegator.
    let delegator_amount = MIN_DELEGATOR_STAKE;
    bond_public(
        &process,
        &finalize_store,
        delegator_private_key,
        &validator_address_1,
        delegator_address,
        delegator_amount,
        rng,
    )
    .unwrap();

    let block_height = rng.gen_range(1..100);

    // validator self-unbonds 0 credits, to set the unbond state
    unbond_public(&process, &finalize_store, &withdrawal_private_key_1, &validator_address_1, 0u64, block_height, rng)
        .unwrap();

    // existing delegator cannot bond to the validator, as unbond state exists
    assert!(bond_public(
        &process,
        &finalize_store,
        delegator_private_key,
        &validator_address_1,
        delegator_address,
        delegator_amount,
        rng,
    )
    .is_err());
}

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

Let the existing delegators increase their delegations even when the validator is in the unbonding state, as long as the validator is in the committee. Alternatively, check whether the amount in unbond_public call is at least 1 credit if the validator is self-unbonding

[evanmarshall]

The ability to unbond_public of 0 for a validator is certainly a real issue but I disagree on the impact. The real issue is that we should have some minimum to prevent holding on to more data than necessary in the unbonding mapping. As for DOS of delegators by validators, I disagree that this is unintended. Validators can already unbond any of their delegators by design and also prevent any new validators by changing their committee state .is_open to false.

There also is a cost to do this but it's the transaction fee associated with unbond_public. In any case, it's an issue we should fix but I would categorize it as minor

[Haxatron]

Escalate

I am sorry, but I also thought of this during the contest and figured out that this is a non-issue.

Why? Because there is no one stopping a validator from unbonding 1 microcredit each time to perform the same attack at which point this is the intended functionality for no one to bond while the validator is unbonding.

The sponsor comment of 0 unbonding being a real issue instead refers to saving on storage costs.

[sherlock-admin3]

Escalate

I am sorry, but I also thought of this during the contest and figured out that this is a non-issue.

Why? Because there is no one stopping a validator from unbonding 1 microcredit each time to perform the same attack at which point this is the intended functionality for no one to bond while the validator is unbonding.

The sponsor comment of 0 unbonding being a real issue instead refers to saving on storage costs.

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.

[evanmarshall]

It's more of a real issue to consider in the future ie informational. Right now, the total number of delegators is limited by the 10K minimum and 100K total maximum number of delegators. If Aleo ever chooses to lower the minimums & raise the maximums, setting a reasonable lower limit on the unbond amount makes sense.

[sammy-tm]

The problem with this is if a validator decides to turn malicious after becoming a validator, it can block existing validators too from bonding at pretty much no cost. And this is beneficial to the validator as it results in more rewards for them due to higher stake % .

There should atleast be some minimum penalty that would disincentivize the validator from doing so

[Haxatron]

The problem with this is if a validator decides to turn malicious after becoming a validator, it can block existing validators too from bonding at pretty much no cost. And this is beneficial to the validator as it results in more rewards for them due to higher stake % .

But the validator can just set the committee state to closed to prevent any new delegators from joining.

[sammy-tm]

@Haxatron

Key-word here is 'existing' delegators

[Haxatron]

Apologies, you are correct. I will just leave this up to the HoJ to decide, based on the points I and the sponsor have already mentioned.

[sammy-tm]

Clarification for ease of judgement:

• set_validator_state can only block new validators from bonding to the validator
• The report's method allows the validator to block both new validators from bonding and existing validators from increasing their bonds.

I believe the contrast between "existing" and "new" is extremely important here and seems to have been missed here, both by the sponsor and escalator.

[evanmarshall]

Validators have complete control over their delegators by design.

Through set_validator_state, they can block new delegators. Through unbond_public, they can forcibly unbond existing delegators.

Nothing has actually changed in the ability of validators to control their delegators with this issue. The reason I think it stands as an informational issue is that this is a somewhat unexpected mechanism to prevent delegators from bonding.

[sammy-tm]

Unbonding delegators completely is way different from blocking them from increasing their delegations.

With unbonding, the validator puts themselves at risk of getting removed from the committee and stop earning rewards. Also, you can only unbond 1 delegator at a time with 1 function call

With this method, the validator can very conveniently block delegators from increasing their delegations, all at the same time, at their benefit (and convenience) while also not risking removal from the committee and maintaining a healthy stake share %

Why this behaviour should be disincentivised is that a complete removal of a delegator is "bad-spirit" for the delegator and the delegator will not bond to the validator again. Whereas, with this method, the validator can exploit delegators that are bonded to them.

[IWildSniperI]

if i understand this correctly the mechanism of preventing any stakes during unbonding phase is so that a malicious validator cannot change the commision of their fee this works for the usual validator unStacking their bonds

but the problem here is that 0 unbonding is allowed,

what would be the motive of a validator preventing users from increasing their bond -> to increase their reward but in fact this should increase his turnAround rewards too from commision

and for the users, can they just un-bond from him and going to bond to another honest validator?

[evanmarshall]

I don't think it's a completely differen

Unbonding delegators completely is way different from blocking them from increasing their delegations.

With unbonding, the validator puts themselves at risk of getting removed from the committee and stop earning rewards. Also, you can only unbond 1 delegator at a time with 1 function call

With this method, the validator can very conveniently block delegators from increasing their delegations, all at the same time, at their benefit (and convenience) while also not risking removal from the committee and maintaining a healthy stake share %

Why this behaviour should be disincentivised is that a complete removal of a delegator is "bad-spirit" for the delegator and the delegator will not bond to the validator again. Whereas, with this method, the validator can exploit delegators that are bonded to them.

I'm not sure how you consider the impact of this as "validator can exploit delegators". There is no exploit here and the gained ability from this issue is already within the realm of power that validators should be considered to have. If a validator continuously unbonds and prevents new delegators / existing delegators from increasing their stake, then the delegator can always leave if they at all feel unhappy with the validator. Similarly, the validator should always have complete control of their delegators and there are good reasons for this: compliance with local laws preventing delegations from known sanctioned parties and to prevent getting too much stake that would result in 0 rewards.

I still consider this an informational issue as it's not an explicit mechanism of validator control but I don't see it as fundamentally different from existing validator controls.

[sammy-tm]

Is there any valid reason why a delegator should not be allowed to bond while validator is in unbonding state?

[WangSecurity]

I've got some questions to consider.

Firstly, as I understand correctly, there's no loss of funds, moreover, the existing delegators can unbond in the meantime and bond for another validator?

Secondly, I see that the DOS is for 360 blocks, how much is Aleo's block? As I understand the DOS in that case is <7 days?

[infosecual]

Firstly, as I understand correctly, there's no loss of funds, moreover, the existing delegators can unbond in the meantime and bond for another validator?

I think the only loss of funds would be the loss to the delegator that cannot make their staking yields while they are locked out. Regarding delegators bonding to another validator- they definitely have the option to delegate to another validator. One thing to note is not all validators are created equal. They can have different quality setups, different operators, different commissions, etc. In the future you could imagine different censorship and mev policies similar to what we see on Ethereum. I will leave it to the judges to determine if this length of time, amount of funds, etc. is sufficient to determine if sherlock's loss-of-funds criteria is met.

Secondly, I see that the DOS is for 360 blocks, how much is Aleo's block? As I understand the DOS in that case is <7 days?

This is from ap3056 (sponsor) on discord regarding block times in Aleo:

dtheo — 06/13/2024 7:41 PM how often are blocks produced in aleo currently? ... ap3056 — 06/13/2024 11:47 PM about once every 3-5 seconds, but as blocks start to get full-er this could slow down somewhat. We're anticipating 10-15 second blocktimes on mainnet

At 15 seconds per block (with the longest anticipated block time on mainnet) x 360 blocks the "can't bond" period is 90 minutes.

While I have stake in this (my #32 is a duplicate), I personally have not been defending this issue bc I do not believe the DOS part of this is particular submission is 100% defendable. I will say that the possibility of 0 credit unbonds is not intentional. I generally agree with everything that Evan says here:

The ability to unbond_public of 0 for a validator is certainly a real issue but I disagree on the impact. The real issue is that we should have some minimum to prevent holding on to more data than necessary in the unbonding mapping. As for DOS of delegators by validators, I disagree that this is unintended. Validators can already unbond any of their delegators by design and also prevent any new validators by changing their committee state .is_open to false.

TLDR: I think this is a "will fix". The impact is probably grey area w.r.t. loss of funds and thus up to the judge's discretion.

[Haxatron]

Let me go back to why I escalated this in the first place:

there is no one stopping a validator from unbonding 1 microcredit each time to perform the same attack at which point this is the intended functionality for no one to bond while the validator is unbonding.

Let me establish the key facts:

• When the validator unbonds, the delegator cannot bond to the validator. This is the intended design of the system and is therefore not considered a "DoS".

• This report is arguing that a 0 unbonding is fundamentally different from a non-zero unbonding and is therefore considered a "DoS"

The entire discussion on "DoS" is moot, the question is whether or not a 0 unbonding is fundamentally any different from a non-zero unbonding which would make it a "DoS".

I don't agree. No one has shown why the 0 value is so special which would make it differ from the intended behaviour and now considered a "DoS" of the system.

[WangSecurity]

I think I get the difference why non-zero unbonding is a design decision and not a DOS, while unbonding 0 is indeed a DOS, cause the validator doesn't get anything from it apart from preventing existing delegators delegating more and new delegators delegating. But, I believe it's a DOS of not time-sensitive functions and only lasts for 90 mins. Hence, I agree that the impact here is low severity. Planning to accept the escalation and invalidate the family.

[WangSecurity]

Result: Invalid Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• haxatron: accepted