[L-04] Insufficient Validation in Pool Operations

[softstackio]

Likelihood: Medium

Description:
In the recoverAbandonedStakes function of the StakingHbbft contract, there is insufficient validation of the stakingAddress before performing critical operations. The function iterates over a list of inactive pools and processes each one, but it does not verify if the stakingAddress actually exists or is valid before performing operations on it.

The relevant code snippet is:

if (isPoolEmpty(stakingAddress) ||
!validatorSetContract.isValidatorAbandoned(stakingAddress)) {
continue; }

This check only verifies if the pool is empty or if the validator is not abandoned. However, it does not ensure that the stakingAddress corresponds to a real, previously registered pool in the system.

Impact:

1. Processing of Invalid Pools: The function could potentially process pools that were never properly registered or no longer exist in the system.
2. Unexpected Behavior: Operations on invalid pools could lead to unexpected state changes or errors in other parts of the contract.
3. Potential for Exploitation: An attacker might be able to manipulate the list of inactive pools to include arbitrary addresses, potentially leading to unintended consequences.

Recommendation:

if (isPoolEmpty(stakingAddress) ||
!validatorSetContract.isValidatorAbandoned(stakingAddress)) {
continue; }

1. Add an explicit check to verify that the staking Address corresponds to a valid, registered pool before processing it. This could be done by maintaining a mapping of valid pool addresses or by adding a function to check pool validity.
2. Implement the check as follows:

if (!isValidPool(stakingAddress) || isPoolEmpty(stakingAddress) ||
!validatorSetContract.isValidatorAbandoned(stakingAddress)) {
continue; }

3. Implement the isValidPool function:

function isValidPool(address _stakingAddress) public view returns (bool) {
    // Check if the address is in the list of registered pools
    return _pools.contains(_stakingAddress) ||
_poolsInactive.contains(_stakingAddress);
}

4. Consider adding events to log any instances where an invalid pool address is encountered during the recovery process.

[axel-muller]

I think in this case there is some misunderstanding...

_pools array contains a list of current active pools, and the array _poolsInactive respectively the list of inactive pools. When a pool becomes inactive, it is removed from the _pools list and immediately added to _poolsInactive and vice versa.

function isValidPool(address _stakingAddress) public view returns (bool) {
    // Check if the address is in the list of registered pools
    return _pools.contains(_stakingAddress) || _poolsInactive.contains(_stakingAddress);
}

if (
  !isValidPool(stakingAddress)
  || isPoolEmpty(stakingAddress)
  || !validatorSetContract.isValidatorAbandoned(stakingAddress)
) {
    continue;
}

Suggested validation function isValidPool will always return true when used in recoverAbandonedStakes, because it iterates over inactive pools.

For the recoverAbandonedStakes function to have any effect on the staking pool, the corresponding validator must be marked as unavailable and not announce its availability for more than a year (current beta network value, may be different for mainnet). Otherwise, it will not be affected.

I think, given the existing checks, we won’t be able to harm the active well-behaved pools and validators.

[softstackio]

alright, double checked and agree.