alexbabits - Validator cannot set new address if more than 300 unstakes in it's array

[sherlock-admin2]

alexbabits

medium

Validator cannot set new address if more than 300 unstakes in it's array

Summary

If a validator has more than 300 accumulated unstakes associated with it, then it cannot set a new address for itself. The only way to decrease the length of the Unstaking array is through the setValidatorAddress() function, but will revert if it's array is longer than 300 entries. A malicious delegator could stake 1,000 tokens, and then unstake 301 times with small amounts to fill up the unstaking array, and there is no way to remove those entries from the array. Every time _unstake() is called, it pushes another entry to it's array for that validator.

A malicious validator could also set it's new address to another validator, forcing a merge of information to the victim validator. The malicious validator can do this even when it's disabled with 0 tokens. So it could get to 300 length, and then send all those unstakes into another victim validators unstaking array. This is the same kind of attack vector, but should be noted that validators can assign their address to other validators, effectively creating a forceful merging of them.

The README.md suggests there is a mechanism to counteract this: "In case if there are more than 300 unstakings, there is an option to transfer the address without unstakings." But there appears to be no function in scope that can transfer the address of the validator without unstakings, or any other function that can reduce the unstakings array at all.

Vulnerability Detail

See Summary

Impact

Validator can be permanently stuck with same address if there are too many entries in it's Unstaking array.

Code Snippet

Validator & Unstaking Structs: https://github.com/sherlock-audit/2023-11-covalent/blob/main/cqt-staking/contracts/OperationalStaking.sol#L35-#L51 setValidatorAddress() length check: https://github.com/sherlock-audit/2023-11-covalent/blob/main/cqt-staking/contracts/OperationalStaking.sol#L702 deletion of array inside setValidatorAddress(): https://github.com/sherlock-audit/2023-11-covalent/blob/main/cqt-staking/contracts/OperationalStaking.sol#L707

Tool used

Manual Review

Recommendation

Consider having a way to set a new address without unstakings, or allow for smaller batches to be transferred during an address change if there are too many. Consider disallowing validators from changing their address to other validators.

[sudeepdino008]

This is interesting. I do have few questions though

A malicious validator could also set it's new address to another validator, forcing a merge of information to the victim validator. The malicious validator can do this even when it's disabled with 0 tokens. So it could get to 300 length, and then send all those unstakes into another victim validators unstaking array. This is the same kind of attack vector, but should be noted that validators can assign their address to other validators, effectively creating a forceful merging of them.

How is setValidatorAddress operates within a Validator storage struct, which is retrieved from a query _validators[validatorId]. So each validatorId maintains its own unstakings etc. Even if a malicious validator sets its validator address to another validator, the new unstakings are maintained within the current validatorId's Validator instance (which is different from the victim validatorId and therefore victim's Validator instance and its unstakings).

Furthermore setting the newAddress to an existing validator address would typically mean that the attacker doesn't own private key of the victim validator. This would cause loss of funds for the attacker, as the validator needs the private key corresponding to its address to retrieve the funds via redeeum or transferUnstakedOut etc.

[sherlock-admin2]

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

takarez commented:

valid: there is no function for that unstaking if its more than 300 in the array; medium(1)

[noslav]

partially fixed by ignore 0 amount unstakings and require unstaking length < 300 - sa46,

[nevillehuang]

@sudeepdino008 @noslav

• What is the purpose of setValidatorAddress()? Is it simply to a feature to allow current validator to operate with another address (given they would have to call themselves, so I believe this does not cater to a private key loss scenario)
• Can the validator still perform regular functionalities such as redeeming rewards, commissions submit proofs with no issue?

If above scenarios are true, I believe this issue is low severity

[noslav]

@sudeepdino008 @noslav

• What is the purpose of setValidatorAddress()? Is it simply to a feature to allow current validator to operate with another address (given they would have to call themselves, so I believe this does not cater to a private key loss scenario)
• Can the validator still perform regular functionalities such as redeeming rewards, commissions submit proofs with no issue?

If above scenarios are true, I believe this issue is low severity

@nevillehuang This is indeed true, the function does not cater to the private key loss scenario but rather a private key leak scenario where an unknown entity has taken hold of the private key, the action required is for the original owner entity to quickly switch the validator staking address to another address they control to thereby save the stake also from being stolen. This function now has the check that it's not another existing address in the system that belongs to a delegator or a validator.

In the case of a complete loss of private key, the validator is responsible for any such full loss and has to deal with the "not your keys not your crypto" essentially having no recourse to _unstake _stake or _redeemRewards with the same address

[sherlock-admin]

Escalate

this issue should be Invalid.

This is already a known issue by the team, and was described in the readMe

When changing its address a validator cannot transfer unstakings if there are more than 300 of them. This is to ensure the contract does not revert from too much gas used. In case if there are more than 300 unstakings, there is an option to transfer the address without unstakings.

https://github.com/sherlock-audit/2023-11-covalent/blob/main/cqt-staking/README.md

scroll down to the Staking Explained section to find it.

You've deleted an escalation for this issue.

[midori-fuse]

I disagree with the escalation.

The first part (cannot transfer unstakings) is known, but the "In case if there are more than 300 unstakings..." part is not. Report #67 proves that it is not possible even for an admin to transfer the validator address without unstakings, so the quoted README part actually confirms that a core contract functionality is broken.

[ArnieSec]

@midori-fuse i agree with you removing escalation.

[ArnieSec]

@midori-fuse escalation removed thanks for pointing out my misjudgment.

[CergyK]

Fix LGTM

[MLON33]

The protocol team has acknowledged the issue.