code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderStakePoolsManager.sol#L215 https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PoolSelector.sol#L93 https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PoolSelector.sol#L99-L104

Vulnerability details

Impact

StaderStakePoolsManager.depositETHOverTargetWeight helps stake all deposited ETH to the capacity available across any pool without disabling deposits. This feature is designed to be fair to all pools over time and enables Stader to stake all deposited ETH as long as the validator supply exists on any pool.

However, if StaderStakePoolsManager.depositETHOverTargetWeight is called when availableETHForNewDeposit > 0 && availableETHForNewDeposit < poolDepositSize, poolIdArrayIndexForExcessDeposit move forward but no ETH would be deposited.

A malicious user can use it to move poolIdArrayIndexForExcessDeposit and causes unfairness

Proof of Concept

StaderStakePoolsManager.depositETHOverTargetWeight calls PoolSelector.poolAllocationForExcessETHDeposit(availableETHForNewDeposit) to get the selected pool capacity. https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderStakePoolsManager.sol#L215

    function depositETHOverTargetWeight() external override nonReentrant {
        if (block.number < lastExcessETHDepositBlock + excessETHDepositCoolDown) {
            revert CooldownNotComplete();
        }
        …
        if (availableETHForNewDeposit == 0) {
            revert InsufficientBalance();
        }
        (uint256[] memory selectedPoolCapacity, uint8[] memory poolIdArray) = IPoolSelector(
            staderConfig.getPoolSelector()
        ).poolAllocationForExcessETHDeposit(availableETHForNewDeposit);

        uint256 poolCount = poolIdArray.length;
        for (uint256 i = 0; i < poolCount; i++) {
            uint256 validatorToDeposit = selectedPoolCapacity[i];
            if (validatorToDeposit == 0) {
                continue;
            }
            …
        }
    }

In PoolSelector.poolAllocationForExcessETHDeposit, it calculate the selectedPoolCapacity and move poolIdArrayIndexForExcessDeposit. https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PoolSelector.sol#L93 https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PoolSelector.sol#L99-L104

    function poolAllocationForExcessETHDeposit(uint256 _excessETHAmount)
        external
        override
        returns (uint256[] memory selectedPoolCapacity, uint8[] memory poolIdArray)
    {
        UtilLib.onlyStaderContract(msg.sender, staderConfig, staderConfig.STAKE_POOL_MANAGER());
        uint256 ethToDeposit = _excessETHAmount;
        …
        uint256 ETH_PER_NODE = staderConfig.getStakedEthPerNode();
        …
        for (uint256 j; j < poolCount; ) {
            uint256 poolCapacity = poolUtils.getQueuedValidatorCountByPool(poolIdArray[i]);
            uint256 poolDepositSize = ETH_PER_NODE - poolUtils.getCollateralETH(poolIdArray[i]);
            uint256 remainingValidatorsToDeposit = ethToDeposit / poolDepositSize;
            selectedPoolCapacity[i] = Math.min(
                poolAllocationMaxSize - selectedValidatorCount,
                Math.min(poolCapacity, remainingValidatorsToDeposit)
            );
            selectedValidatorCount += selectedPoolCapacity[i];
            ethToDeposit -= selectedPoolCapacity[i] * poolDepositSize;
            i = (i + 1) % poolCount;
            //For ethToDeposit < ETH_PER_NODE, we will be able to at best deposit one more validator
            //but that will introduce complex logic, hence we are not solving that
            if (ethToDeposit < ETH_PER_NODE || selectedValidatorCount >= poolAllocationMaxSize) {
                poolIdArrayIndexForExcessDeposit = i;
                break;
            }
            unchecked {
                ++j;
            }
        }
    }

Suppose ethToDeposit is smaller than poolDepositSize. The following things happen.

remainingValidatorsToDeposit = 0 //  ethToDeposit / poolDepositSize = 0
selectedPoolCapacity[i] = 0 //  Math.min(poolCapacity, remainingValidatorsToDeposit)
ethToDeposit -= 0;
i = (i + 1) % poolCount;
poolIdArrayIndexForExcessDeposit = i; // ethToDeposit < poolDepositSize  <= ETH_PER_NODE
break;

We can find out that i moves forward when ethToDeposit is smaller than poolDepositSize. And poolIdArrayIndexForExcessDeposit is set to the new i. In conclusion, a malicious user can call StaderStakePoolsManager.depositETHOverTargetWeight to move poolIdArrayIndexForExcessDeposit when availableETHForNewDeposit > 0 && availableETHForNewDeposit < poolDepositSize. It could cause unfairness. It is said that depositETHOverTargetWeight is designed to be fair to all pools. https://blog.staderlabs.com/ethx-deposits-bda0f62d8ed8

Tools Used

Manual Review

Recommended Mitigation Steps

It is hard to get the poolDepositSize in StaderStakePoolsManager.depositETHOverTargetWeight. Uses ETH_PER_NODE as the lower bound of availableETHForNewDeposit

    function depositETHOverTargetWeight() external override nonReentrant {
        if (block.number < lastExcessETHDepositBlock + excessETHDepositCoolDown) {
            revert CooldownNotComplete();
        }
        …
+       uint256 ETH_PER_NODE = staderConfig.getStakedEthPerNode();
+       if (availableETHForNewDeposit < ETH_PER_NODE ) {
-       if (availableETHForNewDeposit == 0) {
            revert InsufficientBalance();
        }
    }

Assessed type

Other

manoj9april commented 1 year ago

This is a duplicate issue of #175

c4-sponsor commented 1 year ago

manoj9april marked the issue as sponsor disputed

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #175