sherlock-admin3 commented 2 weeks ago

sammy

Medium

`delegated[]` state is not removed after it reaches zero, potentially leading to higher computational costs and DoS

Summary

delegated[] mapping stores the amount of credits delegated to a validator. When this value reaches zero, it is not removed from storage, which can lead to higher computational costs.

Vulnerability Detail

When a validator is removed, the delegated[] mapping is not removed from storage : credits.aleo#L625-L663

    position remove_validator;
    // {
        /* Committee */

        // Remove the validator from the committee.
        remove committee[r5.validator];

        /* Metadata */

        // Retrieve the current committee size.
        get metadata[aleo1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq3ljyzc] into r42;
        // Decrement the committee size by one.
        sub r42 1u32 into r43;
        // Store the new committee size.
        set r43 into metadata[aleo1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq3ljyzc];

        /* Delegated */

        // Decrease the delegated total by the bonded amount.
        sub r31 r30.microcredits into r44;
        // Store the new delegated total.
        set r44 into delegated[r5.validator];

        /* Bonded */

        // Remove the bonded state.
        remove bonded[r5.validator];

        /* Unbonding */

        // Retrieve or initialize the unbonding state.
        get.or_use unbonding[r5.validator] r4 into r45;
        // Increment the unbond amount by the full bonded amount.
        // Note: If the addition overflows, reject the transition.
        add r30.microcredits r45.microcredits into r46;
        // Construct the updated unbond state.
        cast r46 r3 into r47 as unbond_state;
        // Store the new unbonding state.
        set r47 into unbonding[r5.validator];

This can cause problems in other areas of the code, where the entire delegated[] mapping is iterated over. For example, committee.rs#L73-L78

  let Some(microcredits) = delegated_map.iter().find_map(|(delegated_key, delegated_value)| {
                // Retrieve the delegated address.
                let delegated_address = match delegated_key {
                    Plaintext::Literal(Literal::Address(address), _) => Some(address),
                    _ => None,
                };
                // Check if the address matches.
                match delegated_address == Some(address) {
                    // Extract the microcredits from the value.
                    true => match delegated_value {
                        Value::Plaintext(Plaintext::Literal(Literal::U64(microcredits), _)) => Some(**microcredits),
                        _ => None,
                    },
                    false => None,
                }
            }) else {
                bail!("Missing microcredits for committee member - {address}");
            };

A malicious user can activate the delegated mapping for several addresses by repeatedly bonding and unbonding. This will make certain computations expensive and may even lead to a DoS if the computation becomes too expensive to execute.

Impact

DoS

Code Snippet

Tool used

Manual Review

Recommendation

Whenever the delegated[] mapping becomes 0, remove it from storage

sammy-tm commented 6 days ago

Escalate

I think this issue is valid because the cost to iterate the delegated mapping increases with each initialization. Delegated state should be removed if it touches 0. A malicious user can initialize as many delegated[] key-value pairs as they want by bonding and unbonding repeatedly.

sherlock-admin3 commented 6 days ago

Escalate

I think this issue is valid because the cost to iterate the delegated mapping increases with each initialization. Delegated state should be removed if it touches 0. A malicious user can initialize as many delegated[] pairs as they want by bonding and unbonding constantly.

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.

cvetanovv commented 4 days ago

I invalidated this issue because it's unclear what the actual impact would be. @evanmarshall @infosecual Can you give your opinion?

WangSecurity commented 2 days ago

A malicious user can activate the delegated mapping for several addresses by repeatedly bonding and unbonding. This will make certain computations expensive and may even lead to a DoS if the computation becomes too expensive to execute.

So it's at most 1-block DOS, correct? Secondly, how expensive the gas is at Aleo?

sammy-tm commented 2 days ago

No, once the delegated mapping is activated for an address, there is no way to remove it from storage. Also, all other mappings require a significant bond commitment to remain active and are removed from storage once they reach 0. Hence the impact is permanent.

sammy-tm commented 2 days ago

You can understand the transaction fees here

The problem with the delegated[] mapping is that anyone can add to it's storage as long as they have atleast 10_000 credits, they can bond and unbond to ANY address (even if it's not a real address in the network). The only cost to carry out the mentioned attack path is the gas fee. This will increase storage costs and can potentially slow down transactions/ increase fee, leading to DoS.

IWildSniperI commented 1 day ago

in what cases do we need to loop into that mapping?, is it in every txn? that will lead to chain halting? @sammy-tm

sammy-tm commented 1 day ago

Yes, the delegated map is iterated over in every single transaction in the network since it is used to calculate the staking rewards. Which is why it is a bigger concern than a normal smart contract having unbounded storage. Every transaction unrelated to credits.aleo will iterate over the mapping as well, so the impact is not just limited to credits.aleo.

sammy-tm commented 1 day ago


fn test_bond_delegator_to_multiple_random_validators() {
    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 validators and delegators.
    let (validators, delegators) = initialize_stakers(&store, 1000, 1000, rng).unwrap();
    let mut validators = validators.into_iter();

    test_atomic_finalize!(store, FinalizeMode::RealRun, {

        for _ in 0..1000{

            let (validator_private_key_1, (validator_address_1, _, _, withdrawal_address_1)) = validators.next().unwrap();
            let (delegator_private_key, (delegator_address, _)) = delegators.first().unwrap();

            let delegator_amount = MIN_DELEGATOR_STAKE;

        bond_public(
            &process,
            &store,
            delegator_private_key,
            &validator_address_1,
            delegator_address,
            delegator_amount,
            rng,
        )
        .unwrap();

        unbond_public(&process, &store, &delegator_private_key, delegator_address, delegator_amount, 2, rng).unwrap();

        claim_unbond_public(&process, &store, &delegator_private_key, delegator_address, 363, rng).unwrap();
}

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

This test shows that a delegator is able to bond and unbond to 1000 (example value) random addresses permissionlessly.

To run it, place the above in test_credits.aleo.

cvetanovv commented 1 day ago

The main problem is that the delegated[] mapping is not cleaned up and will get very large over time, leading to higher computational costs.

But we would consider it a future integration issue. According to the Readme:

Should potential issues, like broken assumptions about function behavior, be reported if they could pose risks in future integrations, even if they might not be an issue in the context of the scope? If yes, can you elaborate on properties/invariants that should hold?

No

There is also an example of this being done by a malicious user who intentionally filled the array. But here comes a lot of controversial stuff.

What does the malicious user gain from this? He will have to do many transactions to eventually fill the array. Each transaction will cost him money without taking anything in return.
Not everyone can do it.
If it was so easy to fill a map or an array, then almost every contest would have a similar valid issue because there is almost no contest that doesn't iterate through an array.
This is why the rules specify that it must be time-sensitive. Here are the DoS rules.
If this happens naturally over time then according to the readme it is invalid because the protocol does not concerned about future integration issues.

sammy-tm commented 1 day ago

How is this a future integration issue? By this logic every issue is a future integration issue. Can you point out what exactly is getting "integrated" here? The exploit is possible right off the bat and doesn't require any future update to be available.

This is not the same as other Solidity contests where storage is filled up through mappings, etc. This is a layer 1 blockchain and the mapping here affects every single transaction in the network.

What benefit does the attacker have in doing this? An entire network is getting affected by this and I don't think the attacker needs to have a "benefit" from carrying out this attack. If attacks were only carried out if the attacker got a benefit, this would invalidate #35 and #34, because the cost is high and the benefit is 0 for both these issues.

Please look at this issue from PoV of making the network DoS resilient.

sammy-tm commented 1 day ago

And no, the mapping will not be filled automatically over time. The actor in question needs to purposely craft transactions with malicious intent. Why? Because honest delegators will only delegate to addresses that are either in the committee or will join the committee.

WangSecurity commented 1 day ago

I agree that it's not a future integration, but still unsure on the DOS part and the cost of the attack.

Hence the impact is permanent.

So if this array is iterated in (almost) each transaction and it leads to a DOS, then if this attack is executed it would DOS the entire chain and the DOS would permanent. Hence, the only way to mitigate such attack if it were to happen is make a new network (yep, I'm exaggerating here, but want to understand how severe the impact here). Or it can be mitigated in any way if the attack were to happen?

And the cost would be just paying the fees for making the transaction to DOS the chain, correct?

sammy-tm commented 1 day ago

You're right, there is no way to mitigate this issue as it is as there is no way to clear the delegated[] mapping from storage. Hence impact is permanent.

Yes the cost is just gas fees. The credits can be reutilised.

sammy-tm commented 1 day ago

Not to be nit-picky but it is evident from the sponsors comment here that they don't want the storage to hold more data than it should.

https://github.com/sherlock-audit/2024-05-aleo-judging/issues/23#issuecomment-2199085149

However in that particular issue the unbonding[] mapping doesn't cause a storage issue as it requires a bond to remain active and is removed from storage once it's zero.

WangSecurity commented 1 day ago

Based on the above comments, I agree this issue is valid with medium severity. But, I'm planning to reject the escalation since the report doesn't clearly describe the permanent DOS and is a bit vague cause it says "potentially leading" and "may lead" to DOS, so I believe it wouldn't be unfair to penalise the Lead judge for classifying this report as low severity. Hope for the understanding on that decision.

sammy-tm commented 1 day ago

This is unfair because it's not possible to show the exact DoS because of test infrastructure limitation. The "vague" argument can be applied to #34 and #35 as well.

This is an edge case because in most contests the attack scenarios can be reproduced through tests, however it is not possible in this case as this is an audit of a layer 1 blockchain.

I request HoJ to value this edge case and reconsider their decision because this is definitely a valid issue and can cause permanent issues for the protocol. Invalidating this wouldn't be fair.

WangSecurity commented 1 day ago

I don't say the report is vague because it lacks POC, excuse me for the confusion. The problem is that with the report it's hard to verify the real world impact and how severe the DOS would be.

I believe I've said I'm planning to validate the issue with medium severity, but to reject the escalation. Excuse me if the comment was unclear and confusing. Are there additional duplicates of this issue or it's a unique?

sammy-tm commented 1 day ago

Yes, there are no duplicates.

As long as the issue turns valid, I am okay with the decision of rejecting the escalation.

infosecual commented 1 day ago

Good find Sammy

WangSecurity commented 5 hours ago

Result: Medium Unique

sherlock-admin4 commented 5 hours ago

Escalations have been resolved successfully!

Escalation status:

sammy-tm: accepted

sherlock-audit / 2024-05-aleo-judging

sammy - `delegated[]` state is not removed after it reaches zero, potentially leading to higher computational costs and DoS #25

`delegated[]` state is not removed after it reaches zero, potentially leading to higher computational costs and DoS

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

sherlock-audit / 2024-05-aleo-judging

sammy - `delegated[]` state is not removed after it reaches zero, potentially leading to higher computational costs and DoS #25

delegated[] state is not removed after it reaches zero, potentially leading to higher computational costs and DoS

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

`delegated[]` state is not removed after it reaches zero, potentially leading to higher computational costs and DoS