dtheo - `bond_validator` and `bond_public` do not allow bonding even if unbonding window has passed or there are no credits unbonding

[github-actions[bot]]

dtheo

Medium

bond_validator and bond_public do not allow bonding even if unbonding window has passed or there are no credits unbonding

Summary

bond_validator and bond_public do not allow bonding if there currently exists an entry in the global unbonding[] map for the validator. This will fail even if the unbonding window is over, incorrectly preventing valid bonding in these cases.

Vulnerability Detail

bond_validator and unbond_validator do not allow bonding if there currently exists an entry in the global unbonding[] map for the validator. This will fail even if the unbonding window is over, incorrectly preventing valid bonding in these cases. The only way to re-allow bonding is to call claim_unbond_public on the validator. It is also possible for delegators and validators to have a 0 balance for their unbonding credits (see issue #3). This means that invalid unbonding logic can prevent valid bonding flows in multiple ways when this unbonding[] check method is used.

Impact

Delegators and validators attempting to make valid bonds will be unable to

Code Snippet

snarkVM/synthesizer/program/src/resources/credits.aleo#L391-L394

snarkVM/synthesizer/program/src/resources/credits.aleo#L265-L268

Tool used

Manual Review

Recommendation

Change these checks to check if the unbonding window has passed, not just check if an unbonding entry exists as it may not yet be claimed.

[evanmarshall]

Duplicate of: https://github.com/sherlock-audit/2024-05-aleo-judging/issues/23

[Haxatron]

Escalate

User can just call claim_unbond_public then, it is permissionless.

[sherlock-admin3]

Escalate

User can just call claim_unbond_public then, it is permissionless.

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]

In any case, this is either a non-issue or informational. It's an opinion about whether delegators should be able to bond to a validator who is decreasing their self bond.

I have the opinion that this is an informational level issue as we can normally expect validators to withdraw commissions (by calling unbond_public) on a semi regular base. It may be inconvenient that during this 360 blocks, existing delegators won't be able to increase their bond.

[infosecual]

It may be inconvenient that during this 360 blocks, existing delegators won't be able to increase their bond.

One thing to note- they will not be able to increase their bonds even after the the 360 block window until after they call claim_unbond_public, which is a key part of what this submission is calling out. This submission suggests the following fix as this does not seem like intended behavior:

Change these checks to check if the unbonding window has passed, not just check if an unbonding entry exists as it may not yet be claimed.

This will allow for validators to easily control topping up their bond without having to go through the extra step of calling claim_unbond_public. It seems odd to require a validator to call claim on their rewards before allowing an increase in bond as this has tax implications.

FWIW the suggested fix is only one more line of assembly to implement. I do not see good reason to leave these constraints as they are. I don't think the current status quo is the intended behavior.

[WangSecurity]

To better understand the issue. Firstly, if there's an existing entry in the unbonding map, then bonds are unavailable for 360 blocks. But, as this issue shows, the bonds are unavailable even if the 360-block window is passed until the delegator calls claim_unbond_public or the validator has to call claim_unbond_public to allow new bonds to go through?

[sammy-tm]

Anyone can call claim for anyone, It's permissionless

[infosecual]

Sammy is correct- anyone can call claim_unbond_public which is a bit odd. It can trigger taxable events for the validators and delegators even when 3rd parties call it.

It is also odd in the self-calling case for a validator to have to call it before they can top up their active bond. I think a check that the unbonding period is over as opposed to the current check that unbonding[] has an entry is warranted here. This would match up better with what I think the intended functionality is, evidenced by this comment:

// Ensure the validator currently is not unbonding.
assert.eq r20 false;

[infosecual]

Currently that line does not do what the comment says it does. The comments says "Ensure the validator currently is not unbonding." but the line is actually ensuring "there is no entry for the validator in unbonding[]". This is really the root issue and is a relatively simple fix. You can compare the unbonding[] entry unbond_state.height against block.height (the current height). A good example of how this is done is in claim_unbond_public link:

// Determine if unbonding is complete.
gte block.height r1.height into r2;

Changing bond_validator and bond_public to using this correct version of the "is unbonding" check (claim_unbond_publics implementation of the check) would mitigate the issue.

[WangSecurity]

So the impact here is only a DOS, correct? And it would be a one-block DOS, since anyone can call claim_unbond_public and the issue will be "resolved"? Is there a loss of funds involved? No time-sensitive functions are involved, correct?

[infosecual]

So the impact here is only a DOS, correct? And it would be a one-block DOS, since anyone can call claim_unbond_public and the issue will be "resolved"?

Yes. Validators and delegators trying to make valid bonds (eg. even after 360 blocks and even when the unbonding amount is 0 credits) will be blocked from doing so until claim_unbond_public has been called. Theoretically they will be able to do this the very next transaction after claim_unbond_public so I wouldn't say it is a "DOS for a block". I would actually not categorize it as a DOS in its traditional sense. I think the real issue is the process and financial downside of having to make the claim_unbond_public call before valid bonds can be accepted, for reasons mentioned below.

Is there a loss of funds involved? No time-sensitive functions are involved, correct?

I do not believe there are time sensitive functions. The loss of funds are due to:

1. Transaction fees incurred by either delegators or validators having to make calls to claim_unbond_public before being able to make bonds. I believe the intended functionality is that these transactions should not be needed (as evidenced by the code comments indicating otherwise - see my previous two comments).
2. Validators having to experience taxable events in the majority of jurisdictions before they are allowed to increase bonds. In the file's current form this is incorrectly required before making a bond that should be valid.

Not sure if it matters but the comments are not "outdated" or anything. You can see by the files commit history that the comments went in the same time as the incorrect checking of the unbonded[] mapping. I figured I would throw this out there since it might have been relevant in one of the other issues.

[WangSecurity]

Thank you for the clarification. I'll re-iterate how I see this issue (excuse me for repeating you, just to make sure I understand).

The issue here is not that the users will be DOSed and will have to call claim_unbond_public, but the problem is that they will have to spend fees on calling claim_unbond_public when they shouldn't call this function. The evidence here that it's not intended is the code comments here.

@evanmarshall can I get your confirmation the contracts are not outdated and indeed reflect the intended workflow?