sammy - `bond_validator` uses `self.signer` as validator's address, which will make `set_validator_state` uncallable if the validator uses account abstraction

[sherlock-admin2]

sammy

High

bond_validator uses self.signer as validator's address, which will make set_validator_state uncallable if the validator uses account abstraction

Summary

bond_validator uses self.signer to identify the validator, while set_validator_state uses self.caller to identify the validator. This makes credits.aleo incompatible with account abstraction and can cause loss of functionality when the validator is using an account abstraction method.

self.signer is the address of the transaction signer, while self.caller is the address of the intermediate contract (if present) that makes the call.

Vulnerability Detail

The bond_validator function uses self.signer as the validator's address in order to self-bond.

function bond_validator:
    // Input the withdrawal address.
    input r0 as address.public;
    // Input the amount of microcredits to bond.
    input r1 as u64.public;
    // Input the commission percentage.
    input r2 as u8.public;

    // Ensure the withdrawal address is not the validator address.
    assert.neq self.signer r0;

    // Determine if the amount is at least 1 credit.
    gte r1 1_000_000u64 into r3;
    // Enforce the amount is at least 1 credit.
    assert.eq r3 true;

    // Ensure the commission percentage does not exceed 100%.
    gt r2 100u8 into r4;
    assert.neq r4 true;

    // Bond the specified amount of microcredits to the specified validator.
@=> async bond_validator self.signer r0 r1 r2 into r5;
    // Output the finalize future.
    output r5 as credits.aleo/bond_validator.future;

Once the validator joins the committee, they can use the set_validator_state function to close themselves to new delegators. This function serves two purposes :

1. Allow a validator to leave the committee, by closing themselves to stakers and then unbonding all of their stakers.
2. Allow a validator to maintain their % of stake, by closing themselves to allowing more stakers to bond to them.

However, this function uses self.caller as the validator's address :

function set_validator_state:
    // Input the 'is_open' state.
    input r0 as boolean.public;
    // Set the validator to be either open or closed to new stakers.
@=> async set_validator_state self.caller r0 into r1;
    // Output the finalize future.
    output r1 as credits.aleo/set_validator_state.future;

finalize set_validator_state:
    // Input the validator's address.
    input r0 as address.public;
    // Input the 'is_open' state.
    input r1 as boolean.public;

    // Get the committee state for the specified validator.
    // If the validator does not exist, this finalize scope will fail.
@=> get committee[r0] into r2;

    // Construct the updated committee state.
    cast r1 r2.commission into r3 as committee_state;
    // Update the committee state for the specified validator.
    set r3 into committee[r0];

This means that if the validator uses an intermediate contract (like a smart wallet) to bond as a validator, set_validator_state would revert as it uses self.caller to identify the validator, which is the intermediate contract's address and not the validator's.

Impact

By not being able to close delegations by new stakers, validators will lose stake %, which will lead to lesser rewards for the validator. Also, it would make the process of unbending stakers to leave the committee cumbersome, as new delegators constantly bond to the validator

Code Snippet

Tool used

Manual Review

Recommendation

Use self.caller in bond_validator, Alternatively use self.signer in set_validator_state

[evanmarshall]

bond_validator uses self.signer on purpose as to actually participate in consensus, you need to have a private key that can generate signatures for certification creation. We don't allow AA for validators as we couldn't guarantee there's a private key capable of signing. self.signer guarantees that us that the validator address is the actual signer of the transaction.

[sammy-tm]

Escalate

Then why use self.caller in set_validator_state ?

Nowhere in the documentation does it state that validators are not allowed to have AA. I looked up several Leo wallets and a lot of them seem AA friendly. In the ARC-41 docs it is stated that not only Aleo, but also other organisations will have enough credits to become validators at genesis. What's stopping them from using a multi-sig or an AA wallet?

This is certainly an issue if you want to use self.signer in bond_validator for the reason @evanmarshall gave then you must use self.signer in set_validator_state as well.

Even if you decide to disallow AA, the consistency in using self.signer in both functions is necessary. Any intermediate contract would cease the functionality of the validator

[sherlock-admin3]

Escalate

Then why use self.caller in set_validator_state ?

Nowhere in the documentation does it state that validators are not allowed to have AA. I looked up several Leo wallets and a lot of them seem AA friendly. In the ARC-41 docs it is stated that not only Aleo, but also other organisations will have enough credits to become validators at genesis. What's stopping them from using a multi-sig or an AA wallet?

This is certainly an issue if you want to use self.signer in bond_validator for the reason @evanmarshall gave then you must use self.signer in set_validator_state as well.

Even if you decide to disallow AA, the consistency in using self.signer in both functions is necessary. Any intermediate contract would cease the functionality of the validator

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]

That's a fair callout, it would make it more consistent to use self.caller in set_validator_state.

[sammy-tm]

Given that the likelihood of a potential validator using an intermediary (smart walet/multisig/ etc) is medium, and loss of rewards as the validator won't be able to close itself to new validators ( therefore also risking crossing the 25% stake-per-validator limit after which rewards are not earned by the validator or anyone bonded to it ) , I think High severity, as mentioned in the report, is appropriate.

The fix would be to use self.signer in set_validator_state as well

[evanmarshall]

I mispoke, yes it should be self.signer in set_validator_state.

It's certainly not a high severity issue. An EOA is required to actually be a validator because you need to generated signed certificates as part of consensus. Given that a validator address will always represent an EOA as bond_validator uses self.signer, you guarantee that for every validator there exists a private key such that it can always call set_validator_state directly without going through another program. There is no possible loss of functionality.

[infosecual]

Even if you decide to disallow AA, the consistency in using self.signer in both functions is necessary. Any intermediate contract would cease the functionality of the validator

I don't agree with this. For all things OCD and in the name of consistency I can see the argument for wanting set_validator_state to use self.signer but it is not "necessary" and nothing breaks.

Given that the likelihood of a potential validator using an intermediary (smart walet/multisig/ etc) is medium, and loss of rewards as the validator won't be able to close itself to new validators ( therefore also risking crossing the 25% stake-per-validator limit after which rewards are not earned by the validator or anyone bonded to it ) , I think High severity, as mentioned in the report, is appropriate.

This is incorrect. An AA wallet will never be able to become a validator so it will never be able to call set_validator_state in the first place. Therefore it is not possible that to have a "loss of rewards as the validator won't be able to close itself to new validators."

In order to call set_validator_state you have to be part of the committee already to pass this check:

// Get the committee state for the specified validator.
// If the validator does not exist, this finalize scope will fail.
get committee[r0] into r2;

In order to be part of the committee you would have to use a real, non-AA key for self.signer in bond_validator. bond_validator is the first place that any functionality can happen in a validator's life cycle. You cannot use set_validator_state without having passed the self.signer check in bond_validator previously.

Therefore:

1. The self.signer check must exist in bond_validator (as it does)
2. bond_validator is the only place that it is necessary to exist
3. There is no loss of functionality in the code's current form related to this in any way

This is a safety mechanism to prevent loss of funds and does not cause any loss of functionality in the process. The Consensus layer requires validators to make cryptographic signatures over blocks and could not support AA without an additional ledger to hold all of the smart contract wallets and other dependencies of AA.

I can definitely appreciate the eye for detail that Sammy had in this submission not to mention the creativity to consider the AA corner case for validators. Unfortunately I believe the submission to be invalid regardless.

[sammy-tm]

I'm sorry if I'm missing something, but I don't understand why the following scenario is not possible; please feel free to correct me further.

EOA -> Smart Contract -> credits.aleo

Here, self.signer is the validator's EOA, self.caller is a smart contract.

If the validator calls the bond_validator function in credits.aleo, since self.signer is an EOA, everything works correctly and the credits.aleo contract in particular does not care if there is an intermediary contract. The validator can still generate signatures with his EOA (which is the validators actual address)

Now, if the validator tries to call set_validator_state through the same contract, this time, the contract's address would be used, so it would revert.

[evanmarshall]

You're right that the validator couldn't call set_validator_state through the same contract but they could always call it directly on the credits.aleo contract.

[infosecual]

I stand corrected.

In the mentioned scenario Sammy is correct that bond_validator is callable with the EOA -> Smart Contract -> credits.aleo setup and set_validator_state is not. I also agree with Evan that the EOA can still be used to call set_validator_state. The EOA will be required as the validator key in the consensus layer as well (as it should be), so functionality is not broken in anyway in its curent form. Having said that, I think it would be a minor improvement to change set_validator_state to use self.signer instead of self.caller.

[sammy-tm]

I believe this can cause issues in certain use cases wherein some organizations decide to have intermediary contracts to add extra functionality/integrate with credits.aleo. Since already stated in the docs, organizations other than Aleo will have enough credits to become validators at genesis, and they may not be aware of this limitation.

Or even with AA wallets, validator's may choose to use AA, as it's not mentioned anywhere that it's specifically disallowed. In that case, they could bond with the actual EOA, through the smart wallet's interface, and that EOA would be able to generate signatures and everything would work normally. However, the set_validator_state function through the same wallet interface would not work.

[cvetanovv]

I believe this can cause issues in certain use cases wherein some organizations decide to have intermediary contracts to add extra functionality/integrate with credits.aleo. Since already stated in the docs, organizations other than Aleo will have enough credits to become validators at genesis, and they may not be aware of this limitation.

If an organization uses Aleo and adds some new functionality or doesn't check what problems may arise from it, it will be their fault.

[sammy-tm]

What about AA wallets? They are a common use case.

[infosecual]

I would argue that using AA wallets for a validator are not a common use case.

Regardless of this argument the validator will still be able to call set_validator_state with their EOA address. They will need the EOA address to sign blocks anyways. I could see arguments that it would be better to lock down set_validator_state to using the EOA address than to allow a third party interface to call it. It could even be argued that this is a security feature. This construction definitely isn't causing a loss of funds or anything that would qualify it for a medium severity issue.

[sammy-tm]

In that case, I agree that the impact is minor. This issue is low severity and hence invalid. Thanks for the clarification!

[WangSecurity]

Since the escalator agrees issue is low, planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• sammy-tm: rejected