search

sherlock-audit / 2023-11-covalent-judging

3 stars 2 forks source link

zach223 - When updating the validator address with the `setValidatorAddress` function, the delegated amount is not adjusted which may lead to the validator’s potential losses #89

Closed sherlock-admin2 closed 7 months ago

sherlock-admin2 commented 7 months ago

zach223

high

When updating the validator address with the setValidatorAddress function, the delegated amount is not adjusted which may lead to the validator’s potential losses

Summary

In the setValidatorAddress function of the OperationalStaking contract, the validator address self-updates the address information for the corresponding validatorId and invalidates the old validator address. However, if the new validator address was previously delegated tokens to the corresponding validatorId using the stake function, the delegation amount should be subtracted from _validators[validatorId].delegated after this address becomes the new validator address. Failure to do so may result in an excessive _validators[validatorId].delegated number for the new validator address, causing potential losses.

Vulnerability Detail

Considering the following scenario:

  1. Address A is _validators[0]._address, and Address B calls stake(0, delegationMaxCap).
  2. Address A calls setValidatorAddress(0, Address B), making Address B the new _validators[0]._address.
  3. Tokens staked by Address B before becoming the new validator address can no longer be unstaked because calling the unstake function would trigger the constraint v.delegated <= newValidatorMaxCap.

Impact

An inflation of _validators[validatorId].delegated beyond the actual value can lead to the following two impacts:

  1. Some tokens staked by the new validator address cannot be unstaked.
  2. The ability to stake the maximum token quantity into the corresponding validatorId is reduced due to the constraint v.delegated + amount <= delegationMaxCap.

Code Snippet

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

Tool used

Manual Review

Recommendation

It is recommended to add the additional code below the require(!v.frozen, "Validator is frozen”); line in the setValidatorAddress function:

v.delegated -= v.stakings[newAddress].staked;

This line ensures that the delegated value is adjusted by subtracting the staked amount associated with the new address in the setValidatorAddress function.

Duplicate of #66

sherlock-admin2 commented 7 months ago

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

takarez commented:

valid: duplicate of issue 088; although the recommendation here again does not take into account where the previous staked is going to; subtractting it means just lossing it; high(1)