Feature/hotfixes and tests

[yurii-ssv]

No description provided.

[andrew-blox]

in validateClusterOnRegistration you have

    // todo owner can override ssv cluster here, refactor this check
    if (clusterData == bytes32(0) && clusterDataSSV!= bytes32(0)) {
        revert ISSVNetworkCore.IncorrectClusterVersion();
    }

I can technically migrate without calling migrate function. I can liquidate myself / remove validators (clusterData == bytes32(0)) then register a validator and i will migrate. BUt since i didnt officially migrate im not sure we will pay the operators correctly i would assume we pay the operators ssv but with amounts of .00x as in the accounting would pay eth balances but in ssv.

This needs to be reviewed to make sure its true, tight unit testing here would be great

[andrew-blox]

i see we have isLiquidatable and isLiquidatableWithEB
when registering a validator we call isLiquidatable should we instead be calling isLiquidatableWithEB?
we call isLiquidatable as of now in the register validator funct (updateClusterOnRegistration)

Also isLiquidatable get call should prob call isLiquidatableWithEB internally
I didnt check but also reactivate and withdraw should be affected and migrate

[andrew-blox]

in updateClusterOperators we have
if (increaseValidatorCount && !isClusterLiquidated) {
operator.validatorCount -= deltaValidatorCount;
}

shouldnt it be += as increaseValidatorCount = true??

[andrew-blox]

in _updateOperatorVUnits you have
if (deltaPositive) seb.operatorEthVUnits[operatorId] += deltaAbs;
else seb.operatorEthVUnits[operatorId] -= deltaAbs

im not 100% understanding but just want to make sure deltaAbs can never be higher than seb.operatorEthVUnits[operatorId] right?

[andrew-blox]

in getBalanceSSV i see you have
cluster.updateBalanceWithEB(hashedCluster, clusterIndex, sp.currentNetworkFeeIndexSSV());

shouldnt it be cluster.updateBalance? Lots of other places have this logic too i guess just make sure they also call updateBalance instead of updateBalanceWithEB as i didnt check all places that should have this

[andrew-blox]

I think maybe theres some very edge case race condition, in updateBalanceWithEB you have
uint64 vUnits = getVUnits(clusterId, cluster.validatorCount);

Now if the oracle calls to update some cluster by accident calls twice same update func on same cluster really fast in same block, i dont know if the getVunits will give old or updated vunits and how will it apply to the balance.

Maybe theres nothing wrong here i think you guys should verify though

[andrew-blox]

migrateClusterToETH has 2 major issues first is already stated before using isLiquidatable instead of the eth one

Second is you get the burn rate at the beginning
(uint64 clusterIndex, uint64 burnRate) = OperatorLib.updateClusterOperators(

then when you go to see if its liquidatable you are not using the eth amounts!!! you are using the old ssv burn rate yikes :)

[andrew-blox]

in migrateClusterToETH there can be small enhancement

basically no matter what you call updateClusterOperators

now when it comes to dao you have
if (!isLiquidated) {
sp.updateDAOSSV(false, cluster.validatorCount);
}

you shouldnt need to call updateClusterOperators if isLiquidated = true , just like the dao the operators should already be updated with correct accounting

I know you might say updateClusterOperators returns burnRate and we need that but refer to bug above you dont need the ssv burn rate for any reasons moving forward.

[andrew-blox]

i see you have 2 storages for clusterData (ssv and eth)
bytes32 clusterData = s.ethClusters[hashedCluster];
bytes32 clusterDataSSV = s.clusters[hashedCluster];

I thought that eth cluster data just overrides ssv data, this way brings unnecessary gas costs IMO. Maybe i am not seeing full picture. (i put this comment before but deleted it by accident)

[mtabasco]

in validateClusterOnRegistration you have

    // todo owner can override ssv cluster here, refactor this check
    if (clusterData == bytes32(0) && clusterDataSSV!= bytes32(0)) {
        revert ISSVNetworkCore.IncorrectClusterVersion();
    }

I can technically migrate without calling migrate function. I can liquidate myself / remove validators (clusterData == bytes32(0)) then register a validator and i will migrate. BUt since i didnt officially migrate im not sure we will pay the operators correctly i would assume we pay the operators ssv but with amounts of .00x as in the accounting would pay eth balances but in ssv.

This needs to be reviewed to make sure its true, tight unit testing here would be great

@andrew-blox This flow can not be performed because of these reasons:

• Validator removal is not possible for SSV clusters. No matter the order of the action (before/after liquidation).
• Even with all cluster values set to 0 and active = false, the hashed version is not equal to bytes32(0). So keccak256(abi.encodePacked(0,0,0,0,false)) != bytes32(0)