dtheo - `unbond_public` logic causes issues for some delegators preventing partial withdrawals

[sherlock-admin4]

dtheo

Medium

unbond_public logic causes issues for some delegators preventing partial withdrawals

Summary

Delegators that share the same withdrawal address as a validator will be unable to make partial withdrawal due to a logic error in unbond_public. In some cases this can accidentally trigger the complete unbonding of the valdiator even when the delegator does not intend this.

Vulnerability Detail

If a validator ever attempts to delegate to itself or for any reason a delegator shares a withdrawal address with the validator it is bonded to then the logic in unbond_public will prevent it from making partial withdrawals.

Imagine the case:

1. A validator AAA is validating with 100 credits and a withdrawal address of BBB
2. A delegator CCC is bonded to validator AAA with 10M credits and also uses withdrawal address BBB
3. Delegator CCC attempts to unbond ANY amount of credits from the validator using unbond_public, lets say 1 microcredit.eg. delegator calls unbond_public(stakers_addr = CCC, microcredits = 1)

The following logic to determine if the delegators bond should be entirely removed will always incorrectly report that it should:

// Check if the delegator will fall below 10,000 bonded credits.
lt r16 10_000_000_000u64 into r17;

// If the validator is forcing the delegator to unbond OR the delegator will fall below 10,000 bonded credits.
or r11 r17 into r18;

// Determine the amount to unbond: requested amount if >= 10,000 credits, otherwise the full bonded amount.
ternary r18 r5.microcredits r2 into r19;

This is due to the fact that the logic to determine if the delegator is being force unbonded by the validator will always incorrectly report that it is. This will cause both the deleagator and the validator to completely unbond and force their credits into the unbonding period (360 blocks). This will happen even if the delegator calls unbond_public with 0 microcredits.

Impact

This can incorrectly force unbonding for delegators and validators.

Code Snippet

snarkVM/synthesizer/program/src/resources/credits.aleo#L487-L494)

I have also created an annotated pseudo code version of unbond_public to make it easier to read.

Tool used

Manual Review

Recommendation

Add a a conditional to check r7 ( this is set to true if caller == stakers withdrawal address). If this is set and a smaller amount of bond reduction is supplied than the total (as the function argument microcredits then consider this a delegator withdrawal and handle the arithmetic correctly. This will allow validators to still force unbond and will prevent delegators from accidentally unbonding everything without meaning to.

[evanmarshall]

This is a real issue. I would categorize it as minor. Why it's minor is due to the factor that a single user (with both a validator and a delegator must set it up this way) and this single user has the ability to migrate to use separate withdrawal addresses.

[sherlock-admin3]

Escalate

The delegator setting the same withdraw address as the validator is essentially a self-rekt and therefore when this same withdraw address unbonds the delegator it is logically expected that the "validator unbond delegator" logic will apply here.

You've deleted an escalation for this issue.

[evanmarshall]

I'm not clear on the meaning of escalation here. Can you explain?

I think both interpretations are fair but I tend to side with the OP that you should be able to partial unbond. It is also a self rekt with very little consequence ie no funds lost or stolen. Only 360 blocks + one more transaction solve the issue.

[infosecual]

Only 360 blocks + one more transaction solve the issue.

This downplays the possible impact a bit much in my opinion. This could force-remove a validator and its delegator without the operator meaning to. You can clean up the mess by changing your withdrawal address but there is no functionality to "just change withdrawal address". Depending how the delegator and validator are set up they might have to withdrawal the validator and all of its delegators, wait the unbonding period, then re-enroll them with different withdrawal addresses. They could not re-bond the new validator until all of the previous delegators that helped it get to the 10M credit minimum have done this.

[infosecual]

I'm not clear on the meaning of escalation here. Can you explain?

Edit: Haxatron is escalating issues that he believes have been judged incorrectly. If he is right and can prove it he stands to be rewarded. https://docs.sherlock.xyz/audits/judging/escalation-period

[Haxatron]

Haxatron is attempting to escalate to get valid issues marked as invalid. If he is successful and he can get some of his invalids changed to valid he stands to make more of the pot.

Firstly, this comment is uncalled for and I apologise if you feel this way. But I believe I have been quite fair in my escalations thus far. If you believe I have made any incorrect points you are free to dispute it.

Secondly, I still do not think this is a real issue because the delegator must set the same withdrawal address as the validator, for which there is no legitimate reason to do so. And then, when this validator withdrawal address which is the same as the delegator withdrawal address unbonds the delegator it is expected that the "validator unbonds delegator" logic applies.

[morbsel]

I agree with Haxatron, if the withdrawal is set to the same withdrawal address as the validator it means only the validator withdrawal address can unbond the delegator's bond and when the validator unbonds a delegator's bond the whole amount should get unbonded, stated at: https://github.com/sherlock-audit/2024-05-aleo/blob/55b2e4a02f27602a54c11f964f6f610fee6f4ab8/snarkVM/synthesizer/program/src/resources/credits.aleo#L415-L417 So partial unbond shouldn't even be a point of discussion.

[infosecual]

Firstly, this comment is uncalled for and I apologise if you feel this way.

I apologize. I did not mean this in a condescending way or anything. I see how that came across now and I did not mean for it to sound this way. Please excuse me. This is better wording and is more in line with what I mean to say:

Haxatron is escalating issues that he believes have been judged incorrectly. If he is right and can prove it he stands to be rewarded.

[evanmarshall]

I still believe this issue as a whole remains an issue. In theory, a delegator should be able to partially unbond. It will be common for validator operators to handle large credit amounts by delegating to themselves. Validator keys are necessarily hot (why only the 100 credit self bond is required). Delegator keys and withdrawals addresses can be cold and use offline signing. It's possible that a validator tries to use the same withdrawal address for both validator and delegator. Through normal operation, a validator operator will want to withdraw rewards to fund operations. Partial unbonds would be expected in this case.

[Haxatron]

Since the sponsor has stated that there is a legitimate use case for a validator and a delegator to have the same withdrawal address, I do agree that this is a valid issue and will delete the escalation.