code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PermissionedNodeRegistry.sol#L562 https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PermissionlessNodeRegistry.sol#L468 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#L183 https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PermissionedPool.sol#L89 https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PermissionedNodeRegistry.sol#L258

Vulnerability details

Impact

StaderStakePoolsManager sends the staked ETH to the pools via StaderStakePoolsManager.validatorBatchDeposit and StaderStakePoolsManager.depositETHOverTargetWeight. It uses PoolSelector to select the pools. However, it doesn’t consider the pause status of the node registries. If the registry is paused, the pool should not receive the staked ETH. Or the staked ETH could be stuck in the pool until the registry is unpaused.

Proof of Concept

StaderStakePoolsManager sends the staked ETH to the pools via StaderStakePoolsManager.validatorBatchDeposit and StaderStakePoolsManager.depositETHOverTargetWeight. It would call IStaderPoolBase(poolAddress).stakeUserETHToBeaconChain() and send the staked ETH to the pool. https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderStakePoolsManager.sol#L183

    function validatorBatchDeposit(uint8 _poolId) external override nonReentrant whenNotPaused {
        …
        IStaderPoolBase(poolAddress).stakeUserETHToBeaconChain{value: selectedPoolCapacity * poolDepositSize}();
        …
    }

In PermissionedPool.stakeUserETHToBeaconChain, it receives the staked ETHs and performs the preDeposit. https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PermissionedPool.sol#L89

    function stakeUserETHToBeaconChain() external payable override nonReentrant {
        …
                uint256 validatorId = INodeRegistry(nodeRegistryAddress).validatorIdsByOperatorId(i, index);
                preDepositOnBeaconChain(nodeRegistryAddress, vaultFactory, ethDepositContract, validatorId);
            …
    }

And PermissionedNodeRegistry.markValidatorReadyToDeposit needs to be called to deposit the rest ETHs. https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PermissionedNodeRegistry.sol#L258

    function markValidatorReadyToDeposit(
        bytes[] calldata _readyToDepositPubkey,
        bytes[] calldata _frontRunPubkey,
        bytes[] calldata _invalidSignaturePubkey
    ) external override nonReentrant whenNotPaused {
        …
        IPermissionedPool(permissionedPool).fullDepositOnBeaconChain(_readyToDepositPubkey);
    }

There is a whenNotPaused modifier on PermissionedNodeRegistry.markValidatorReadyToDeposit. It cannot be called in when the registry is paused. That means the ETHs are stuck in the pool until the register is unpaused. https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PermissionedNodeRegistry.sol#L562

    function pause() external override {
        UtilLib.onlyManagerRole(msg.sender, staderConfig);
        _pause(); // @audit: PermissionedNodeRegistry can be paused by the manager.
    }

Tools Used

Manual Review

Recommended Mitigation Steps

StaderStakePoolsManager should not send the staked ETH to the pools with a paused registry. Add some checks in StaderStakePoolsManager or PoolSelector to prevent such issue.

Assessed type

Other

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #242

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)

sces60107 commented 1 year ago

This issue is identified as a dup of https://github.com/code-423n4/2022-06-stader-findings/issues/242, which is currently classified as a quality assurance (QA) issue

The decision to downgrade is based on the comment provided by the sponsor in https://github.com/code-423n4/2022-06-stader-findings/issues/242

Only Staked funds related flows are required to be pausable by protocol design.

This issue is staked funds related because: stakeUserETHToBeaconChain receives stake funds from the pool manager even the registry is paused. https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PermissionedPool.sol#L89

Therefore, it shouldn't be a dup of https://github.com/code-423n4/2022-06-stader-findings/issues/242. And the severity should be reconsidered

Picodes commented 1 year ago

Hi @sces60107, thanks for your comment. I still think QA is more appropriate:

no funds are lost in the scenario described here as the admin could unpause
validatorBatchDeposit has a whenNotPaused modifier and depositETHOverTargetWeight only manages the excesses of validatorBatchDeposit so overall if the admin behaves correctly there is no issue of medium severity here in my view