PUSH0 - Frontrunning validator freeze to withdraw tokens

[sherlock-admin]

PUSH0

medium

Frontrunning validator freeze to withdraw tokens

Summary

Covalent implements a freeze mechanism to disable malicious Validators, this allows the protocol to block all interactions with a validator when he behaves maliciously. Covalent also implements a timelock to ensure tokens are only withdraw after a certain amount of time. After the cooldown ends, tokens can always be withdrawn.

Following problem arise now: because the tokens can always be withdrawn, a malicious Validator can listen for a potential "freeze" transaction in the mempool, front run this transaction to unstake his tokens and withdraw them after the cooldown end.

Vulnerability Detail

Almost every action on the Operational Staking contract checks if the validator is frozen or not:

 require(!v.frozen, "Validator is frozen");

The methods transferUnstakedOut() and recoverUnstaking() are both not checking for this, making the unstake transaction front runnable. Here are the only checks of transferUnstakedOut():

require(validatorId < validatorsN, "Invalid validator");
        require(_validators[validatorId].unstakings[msg.sender].length > unstakingId, "Unstaking does not exist");
        Unstaking storage us = _validators[validatorId].unstakings[msg.sender][unstakingId];
        require(us.amount >= amount, "Unstaking has less tokens");

https://github.com/sherlock-audit/2023-11-covalent/blob/main/cqt-staking/contracts/OperationalStaking.sol#L559-L572

This makes following attack possible:

1. Validator cheats and gets rewarded fees.
2. Protocol notices the misbehavior and initiates a Freeze transaction
3. Validator sees the transaction and starts a unstake() transaction with higher gas.
4. Validator gets frozen, but the unstaking is already done
5. Validator waits for cooldown and withdraws tokens.

Now the validator has gained unfairly obtained tokens and withdrawn his stake.

Impact

Malicious validators can front run freeze to withdraw tokens.

Code Snippet

https://github.com/sherlock-audit/2023-11-covalent/blob/main/cqt-staking/contracts/OperationalStaking.sol#L559-L572

Tool used

Manual Review

Recommendation

Implement a check if validator is frozen on transferUnstakedOut() and recoverUnstaking(), and revert transaction if true.

If freezing all unstakings is undesirable (e.g. not freezing honest unstakes), the sponsor may consider storing the unstake timestamp as well:

• Store the unstaking block number for each unstake.
• Freeze the validator from a certain past block only, only unstakings that occur from that block onwards will get frozen.

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid: this is theoretically not possile due to the cooldown time; the time will allow the governance to pause the contract/function

[noslav]

fixed by check validator not frozen for recoverUnstaking & transferUnstakedOut

[nevillehuang]

Invalid, both are user facing functions, not validators.

[Oot2k]

Escalate

I believe this issue was mistakenly excluded, the frontrunning of freeze transaction is indeed a problem like described in the Report.

The impact described is clearly medium, because this attack makes the freeze function almost useless. Also it generates clear loss of funds for the protocol, because in most cases a malicious validator might accrue rewards which do not belong to him.

This issue can not really be fixed by governance pausing the contract, this would pause the contract for everyone else aswell and cause even more damage.

The fix by protocol teams looks good.

To summarize: Issue is fixed, impact is High, issue should be open and valid.

[sherlock-admin]

Escalate

I believe this issue was mistakenly excluded, the frontrunning of freeze transaction is indeed a problem like described in the Report.

The impact described is clearly medium, because this attack makes the freeze function almost useless. Also it generates clear loss of funds for the protocol, because in most cases a malicious validator might accrue rewards which do not belong to him.

This issue can not really be fixed by governance pausing the contract, this would pause the contract for everyone else aswell and cause even more damage.

The fix by protocol teams looks good.

To summarize: Issue is fixed, impact is High, issue should be open and valid.

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.

[midori-fuse]

Adding to the escalation point, there is no way for governance to forcefully claim an unstaking (or any rewards that has been distributed). Therefore eventually the contract must be unpaused to avoid locking of existing funds, and the malicious actor fully gets away.

[Oot2k]

Additionally I think this issue should be judged as HIGH severity based on following facts:

• it create a clear loss of funds for the protocol (The main reason to freeze a validator is to penalize him for malicious behavior, this can include stealing funds / rewards)
• there is no way to prevent this behavior without causing more damage
• attack cost is really low -> just transaction fee

[nevillehuang]

@Oot2k @midori-fuse @noslav Could you shed more details on how the validator can cheat and get rewarded fees and in what scenarios is a freeze initiated. It seems to me like a hypothetical scenario given my understanding is validator is still unstaking amounts that belongs to him, but could be significant.

1. Validator cheats and gets rewarded fees.

Additionally, this issue seems to be dependent on a front-running attack vector, so:

• If flashbots are considered similar to issue here to mitigate front-running, I believe this could be low severity
• If not, if it is true that the freeze mechanism can be bypassed, then I believe this is medium severity, since it is dependent on a hypothetical scenario that validators turn malicious.

Additionally, is there any mechanisms in place to mitigate malicious validators?

[midori-fuse]

Providing evidence for the bypassing of freeze mechanism. Search the following phrase within the contract:

require(!v.frozen, "Validator is frozen");

It appears 6 times throughout the contract, and covers all entry points except transferUnstakedOut() (except admin and reward manager functions). Eyeballing all other external functions (except the ones mentioned) will show that they all go through _stake() or _unstake(), which has the appropriate check.

For transferUnstakedOut(), there is no check for whether the validator corresponding to that unstake has been frozen or not, neither is there in _transferFromContract().

The flow for an unstaking to happen (for delegators or validators alike) is that:

• The user calls unstake() or unstakeAll().
• Wait for the cooldown.
• Call transferUnstakedOut() to actually receive those tokens.

Then the freeze is bypassed if the user is able to call unstake before the validator is frozen. Front-running is only required to maximize the getaway amounts by squeezing some extra rewards, you can just unstake before getting freeze and you bypass the freeze already. Therefore this is just a bypassing of freeze, and not dependent on front-running. We simply show the scenario which has the maximum impact.

[midori-fuse]

For the scenario where a validator cheats, there are certain ways for it to happen:

• Two or more validators collude and are able to force quorum on certain sessions, earning them rewards on an incorrect block specimen.
• A validator finds a systemic exploit and/or simply not doing work correctly (e.g. admin determined them to repeatedly copy-paste other validators' works by watching the mempool or copying existing submissions, despite it being wrong or not). Note that a disabled validator can still unstake and get away with funds, unlike the frozen scenario (without the current bypassing issue).

The freeze is there to protect against these kind of situations.

[nevillehuang]

I personally am not convinced of this issue because the admins can always perform a system wide contract pause before freezing validators in separate transactions via flashbots (which pauses all actions, including transferUnstakedOut), which possibly mitigates this issue. This is in addition to the fact that there is a 28 day unstaking cooldown period which is more than sufficient time to react to malicious validators by admins (which in itself is a mitigation).

So I will leave it up to @Czar102 and sponsors to decide validity.

[midori-fuse]

28 day unstaking cooldown does not mitigate this. As soon as the unstaking is done, the amount can be transferred out after 28 days (and the admin unpauses the system). Even if the system is paused, there are no admin actions that can revoke an unstaking that's on cooldown.

Keeping the system paused equates to locking all funds, including other validators' funds and their respective delegators, and the admin still cannot take over the stolen funds.

Furthermore the issue shows that freezing can be bypassed, and front-running is not a condition. The validator can just unstake right after the exploit, and the admin is already powerless before noticing the issue.

Just adding some extra points. As part of the team making the escalation, we have the responsibility to provide extra information and any context the judges' might have missed, but we respect the judges' decision in any case.

[Oot2k]

Agree with @midori-fuse here. Cooldown -> does not do anything because the malicious user still transfers tokens out (this is the root cause of this issue) Admin can pause protocol -> this will pause all actions for other users as well, as soon as the protocol is unpaused funds can be withdrawn again Malicious funds -> yes this report assumes there is a way to get funds maliciously and for this reason the team implemented the freeze mechanism

I think this summarizes the issue pretty well and should be enough for Sherlock to validate.

[nevillehuang]

If the hypothetical scenario of a way to cheat funds/validators being malicious is considered as a valid reason that break admin initiated pause mechanism, I can agree this is a valid medium since I believe the only way to resolve the issue is for the owner to perform an upgrade to the contract.

Although I must say, the whole original submission is only presenting a front-running scenario, and the watsons only realized after that front-running is not necessary but did not include it in the original submission, and hence my arguments.

[Czar102]

Great points made, the frontrunning argument is also not convincing to me since it's quite clear that this race condition is by design and it's admin's responsibility to keep the information about the freeze private until confirmation.

But, this issue can also be considered a loss of functionality (freezing stakes) due to the existence of a beneficial optimal game-theoretic behavior of the attacker.

I'm currently inclined to accept this as a Medium severity issue.

[Czar102]

Result: Medium Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• Oot2k: accepted

[sherlock-admin]

The protocol team fixed this issue in PR/commit https://github.com/covalenthq/cqt-staking/pull/125/commits/de86308999d093a3f4553aa7094ed4d29be8beb0.

[CergyK]

Fix LGTM

[sherlock-admin]

The Lead Senior Watson signed off on the fix.