`PoolSelector.computePoolAllocationForDeposit` could return an unfair value.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PoolSelector.sol#L50 https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderStakePoolsManager.sol#L199

Vulnerability details

Impact

When calling StaderStakePoolsManager.validatorBatchDeposit, it calls PoolSelector.computePoolAllocationForDeposit to get the validator count to deposit for the pool. It calculates the count based on the capacity and the weight of the pool. However, PoolSelector.computePoolAllocationForDeposit could return an unfair value due to the different poolDepositSize between the pools.

Proof of Concept

StaderStakePoolsManager.validatorBatchDeposit calls PoolSelector.computePoolAllocationForDeposit to get the validator count to deposit for the pool. https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderStakePoolsManager.sol#L199

    function validatorBatchDeposit(uint8 _poolId) external override nonReentrant whenNotPaused {
        …
        uint256 poolDepositSize = staderConfig.getStakedEthPerNode() - poolUtils.getCollateralETH(_poolId);

        if (availableETHForNewDeposit < poolDepositSize) {
            revert InsufficientBalance();
        }
        uint256 selectedPoolCapacity = IPoolSelector(staderConfig.getPoolSelector()).computePoolAllocationForDeposit(
            _poolId,
            (availableETHForNewDeposit / poolDepositSize)
        );

        …
    }

PoolSelector.computePoolAllocationForDeposit calculates the count based on the capacity and the weight of the pool. https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderStakePoolsManager.sol#L199

    function computePoolAllocationForDeposit(uint8 _poolId, uint256 _newValidatorToRegister)
        external
        view
        override
        returns (uint256 selectedPoolCapacity)
    {
        IPoolUtils poolUtils = IPoolUtils(staderConfig.getPoolUtils());
        uint256 totalActiveValidatorCount = poolUtils.getTotalActiveValidatorCount();
        uint256 totalValidatorsRequired = (totalActiveValidatorCount + _newValidatorToRegister);
        uint256 remainingPoolCapacity = poolUtils.getQueuedValidatorCountByPool(_poolId);
        uint256 currentActiveValidators = poolUtils.getActiveValidatorCountByPool(_poolId);
        uint256 poolTotalTarget = (poolWeights[_poolId] * totalValidatorsRequired) / POOL_WEIGHTS_SUM;
        (, uint256 remainingPoolTarget) = SafeMath.trySub(poolTotalTarget, currentActiveValidators);
        selectedPoolCapacity = Math.min(
            Math.min(poolAllocationMaxSize, _newValidatorToRegister),
            Math.min(remainingPoolCapacity, remainingPoolTarget)
        );
    }

Suppose that availableETHForNewDeposit is 1600 ETH and the weight of the permissioned pool and the weight of the permissionless pool are the same. And no ETH has been sent to the pools:

• validatorBatchDeposit(1) is called first // for permissionless pool

  availableETHForNewDeposit / poolDepositSize = 1600 / 28 = 57
  computePoolAllocationForDeposit(1, 57) is called
  totalValidatorsRequired = (totalActiveValidatorCount + _newValidatorToRegister) = 0 + 57 = 57
  currentActiveValidators = 0
  poolTotalTarget = (poolWeights[_poolId] * totalValidatorsRequired) / POOL_WEIGHTS_SUM = 57 / 2 = 28
  remainingPoolTarget = poolTotalTarget - currentActiveValidators = 28 - 0 = 28
  selectedPoolCapacity = 28

• validatorBatchDeposit(2) is called later // for permissioned pool

  availableETHForNewDeposit / poolDepositSize = 816 / 32 = 25
  computePoolAllocationForDeposit(1, 25) is called
  totalValidatorsRequired = (totalActiveValidatorCount + _newValidatorToRegister) = 28 + 25 = 53
  currentActiveValidators = 0
  poolTotalTarget = (poolWeights[_poolId] * totalValidatorsRequired) / POOL_WEIGHTS_SUM = 53 / 2 = 26
  remainingPoolTarget =  poolTotalTarget - currentActiveValidators = 26 - 0 = 26
  selectedPoolCapacity = Math.min(_newValidatorToRegister, remainingPoolTarget) = 25

• The permissioned pool activate 25 validators and the permissionless pool activate 28 validators

Let’s do it again in a different order:

• validatorBatchDeposit(2) is called first // for permissioned pool

  availableETHForNewDeposit / poolDepositSize = 1600 / 32 = 50
  computePoolAllocationForDeposit(2, 50) is called
  totalValidatorsRequired = (totalActiveValidatorCount + _newValidatorToRegister) = 0 + 50 = 50
  currentActiveValidators = 0
  poolTotalTarget = (poolWeights[_poolId] * totalValidatorsRequired) / POOL_WEIGHTS_SUM = 50 / 2 = 25
  remainingPoolTarget = poolTotalTarget - currentActiveValidators = 25 - 0 = 25
  selectedPoolCapacity = 25

• validatorBatchDeposit(1) is called later // for permissionless pool

  availableETHForNewDeposit / poolDepositSize = 800 / 28 = 28
  computePoolAllocationForDeposit(1, 28) is called
  totalValidatorsRequired = (totalActiveValidatorCount + _newValidatorToRegister) = 25 + 28 = 53
  currentActiveValidators = 0
  poolTotalTarget = (poolWeights[_poolId] * totalValidatorsRequired) / POOL_WEIGHTS_SUM = 53 / 2 = 26
  remainingPoolTarget =  poolTotalTarget - currentActiveValidators = 26 - 0 = 26
  selectedPoolCapacity = 26

• The permissioned pool activate 25 validators and the permissionless pool activate 26 validators

We can find out that the permissionless pool can only activate 26 validators if validatorBatchDeposit(2) is called first. It indicates that computePoolAllocationForDeposit could cause unfairness.

Tools Used

Manual Review

Recommended Mitigation Steps

The mitigation depends on how Stader wants to distribute the ETH based on the weight. If the weight should reflect the amount of ETH received by the pool. Then poolTotalTarget should be calculated based on the total received ETH amount instead of the totalValidatorsRequired since _newValidatorToRegister is calculated based on poolDepositSize. For instance, the calculation can be modified like:

function computePoolAllocationForDeposit(uint8 _poolId, uint256 availableETHForNewDeposit)
uint256 totalETH = totalETH + availableETHForNewDeposit
uint256 poolETHTraget = (poolWeights[_poolId] * totalETH) / POOL_WEIGHTS_SUM;
uint256 poolTotalTarget = poolETHTraget / poolDepositSize

Assessed type

Math

[c4-sponsor]

manoj9april marked the issue as sponsor acknowledged

[manoj9april]

Intention is to have 70% of total active validator in permissionless pool (assuming target is 70%). Total active validator count may change depending on the execution of validator batch deposit. We don't see any harm in that.

[c4-sponsor]

manoj9april marked the issue as disagree with severity

[Picodes]

The described impact doesn't qualify for Medium severity in my opinion. The fact that actions are not path independent is not an issue in itself.

[c4-judge]

Picodes marked the issue as unsatisfactory: Overinflated severity