Changing Validator to an Existing Delegator Might Skew Delegated Value

Summary

If a validator invoked the setValidatorAddress and set it to an existing delegator of that validator, the delegated variable will not be able to be changed, causing it to be inflated indefinitely.

Vulnerability Detail

When the _address variable is changed to an existing delegator, the delegated will not be able to be deducted because the the delegator becomes the validator.

As seen from the _unstake, it checked for whether the msg.sender is equal to v._address,

bool isValidator = msg.sender == v._address;

This line will never be called, as the delegator has now become the validator.

if (!isValidator) {
    v.delegated -= effectiveAmount;
}

Consequently, this will affect how much users can actually delegate to this particular validator. As seen from stake, it checks whether the current delegated amount with the addition of amount is lesser than the max cap. With the permanent initial delegated amount, the max delegated will be lesser than the true max cap. This is more significant if the new validator had a huge initial delegated before the transfer.

uint128 newDelegated = v.delegated + amount;
require(newDelegated <= delegationMaxCap, "Validator max delegation exceeded");

It also affects unstake. If the new validator had a large delegated amount before the change, the validator is unable to unstake

uint128 newValidatorMaxCap = newStaked * maxCapMultiplier;
require(v.delegated <= newValidatorMaxCap, "Cannot decrease delegation max-cap below current delegation while validator is enabled");

Impact

The inflated data results in users unable to delegate as much and the validator unable to unstake as much as the intended.

Code Snippet

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

function setValidatorAddress(uint128 validatorId, address newAddress) external whenNotPaused {
    Validator storage v = _validators[validatorId];
    require(msg.sender == v._address, "Sender is not the validator");
    require(v._address != newAddress, "The new address cannot be equal to the current validator address");
    require(newAddress != address(0), "Invalid validator address");
    require(!v.frozen, "Validator is frozen");

    v.stakings[newAddress].shares += v.stakings[msg.sender].shares;
    v.stakings[newAddress].staked += v.stakings[msg.sender].staked;
    delete v.stakings[msg.sender];

    Unstaking[] storage oldUnstakings = v.unstakings[msg.sender];
    uint256 length = oldUnstakings.length;
    require(length <= 300, "Cannot transfer more than 300 unstakings");
    Unstaking[] storage newUnstakings = v.unstakings[newAddress];
    for (uint128 i = 0; i < length; ++i) {
        newUnstakings.push(oldUnstakings[i]);
    }
    delete v.unstakings[msg.sender];

    v._address = newAddress;
    emit ValidatorAddressChanged(validatorId, newAddress);
}

PoC

it('Should be unable to remove old delegated number', async function() {
    const [
      opManager,
      contract,
      cqtContract,
      validator1,
      validator2,
      delegator1,
      delegator2,
    ] = await getAll();

    await contract.connect(opManager).setStakingManagerAddress(opManager.address);
    await addStakedValidator(0, contract, cqtContract, opManager, validator1, 10);
    let details = await contract.getValidatorMetadata(0);
    await expect(details._address).to.equal(VALIDATOR_1);

    // A delegator delegate to this address (10 token)
    await stake(oneToken.mul(10), delegator1, cqtContract, contract, 0);

    // Validator initial delegated supposed to be 10 ether
    details = await contract.getValidatorMetadata(0);
    await expect(details.delegated).to.equal(oneToken.mul(10));

    // setValidatorAddress to delegator
    await contract.connect(validator1).setValidatorAddress(0, delegator1.address);
    const oldDetails = await contract.getValidatorMetadata(0);
    await expect(oldDetails._address).to.equal(delegator1.address);

    // unstake 10
    await contract.connect(delegator1).unstake(0, oneToken.mul(10));
    const newDetails = await contract.getValidatorMetadata(0);

    // Now the delegates value that belonged to this address can no longer be changed.

    expect(newDetails.delegated).to.equal(oldDetails.delegated);
  })

Tool used

Manual Review

Recommendation

Implement additional checks to verify whether the newAddress has staked and shares, before transferring over.

function setValidatorAddress(uint128 validatorId, address newAddress) external whenNotPaused {
    Validator storage v = _validators[validatorId];
    require(msg.sender == v._address, "Sender is not the validator");
+   require(v.stakings[newAddress].shares == 0, "New address already has staked shares");
+   require(v.stakings[newAddress].staked == 0, "New address already has staked amount");
    require(v._address != newAddress, "The new address cannot be equal to the current validator address");
    require(newAddress != address(0), "Invalid validator address");
    require(!v.frozen, "Validator is frozen");

    v.stakings[newAddress].shares += v.stakings[msg.sender].shares;
    v.stakings[newAddress].staked += v.stakings[msg.sender].staked;
    delete v.stakings[msg.sender];

    Unstaking[] storage oldUnstakings = v.unstakings[msg.sender];
    uint256 length = oldUnstakings.length;
    require(length <= 300, "Cannot transfer more than 300 unstakings");
    Unstaking[] storage newUnstakings = v.unstakings[newAddress];
    for (uint128 i = 0; i < length; ++i) {
        newUnstakings.push(oldUnstakings[i]);
    }
    delete v.unstakings[msg.sender];

    v._address = newAddress;
    emit ValidatorAddressChanged(validatorId, newAddress);
}

Duplicate of #66

sherlock-audit / 2023-11-covalent-judging

SadBase - Changing Validator to an Existing Delegator Might Skew Delegated Value #82