Stader Labs - Aymen0909's results

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/SocializingPool.sol#L21 https://github.com/code-423n4/2023-06-stader/blob/main/contracts/Auction.sol#L14 https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderOracle.sol#L17 https://github.com/code-423n4/2023-06-stader/blob/main/contracts/OperatorRewardsCollector.sol#L16

Vulnerability details

Impact

The following contracts : SocializingPool, StaderOracle, OperatorRewardsCollector and Auction are supposed to be pausable (as they all inherit from PausableUpgradeable) but they don't implement the external pause/unpause functionalities which means it will never be possible to pause them.

Proof of Concept

All the following contracts SocializingPool, StaderOracle, OperatorRewardsCollector and Auction inherit from the openzeppelin PausableUpgradeable extension which means that they contain internal functions _pause and _unpause.

Because those function are internal, the contract must implement two other public/external pause and unpause functions to allow the manager to pause and unpause the contracts when necessary, but none of the aforementioned contracts implement those functions which means that even if those contracts are supposed to be pausable (have the pause/unpause functionalities) none of them can be paused.

Tools Used

Manual review

Recommended Mitigation Steps

Add public/external pause and unpause functions in the aforementioned contracts to allow them to be pausable, this can be done just as in the UserWithdrawalManager contract for example :

/**
 * @dev Triggers stopped state.
 * Contract must not be paused
 */
function pause() external {
    UtilLib.onlyManagerRole(msg.sender, staderConfig);
    _pause();
}

/**
 * @dev Returns to normal state.
 * Contract must be paused
 */
function unpause() external onlyRole(DEFAULT_ADMIN_ROLE) {
    _unpause();
}

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PoolUtils.sol#L55-L65

Vulnerability details

Impact

The purpose of the updatePoolAddress function is to update the pool address associated with an existing poolId. However, due to its internal invocation of the verifyNewPool function, the updatePoolAddress function always reverts, this occurs because the verifyNewPool function itself reverts when the specified poolId already exists. Consequently, it is not possible to update the pool address for an existing poolId.

Proof of Concept

The issue occurs in the updatePoolAddress function below :

File: PoolUtils.sol Line 55-65

function updatePoolAddress(
    uint8 _poolId,
    address _newPoolAddress
) external override onlyExistingPoolId(_poolId) onlyRole(DEFAULT_ADMIN_ROLE) {
    UtilLib.checkNonZeroAddress(_newPoolAddress);
    // @audit always revert on exsiting poolId
    verifyNewPool(_poolId, _newPoolAddress); 
    poolAddressById[_poolId] = _newPoolAddress;
    emit PoolAddressUpdated(_poolId, _newPoolAddress);
}

As it can be seen from the code above, the updatePoolAddress function contains the onlyExistingPoolId modifier which means it can only be called for updating the pool address of an already exiting poolId.

Before updating the pool address the updatePoolAddress function calls the verifyNewPool function below :

function verifyNewPool(uint8 _poolId, address _poolAddress) internal view {
    if (
        INodeRegistry(IStaderPoolBase(_poolAddress).getNodeRegistry()).POOL_ID() != _poolId ||
        isExistingPoolId(_poolId)
    ) {
        revert ExistingOrMismatchingPoolId();
    }
}

It's clear that The function reverts when the poolId already exists meaning isExistingPoolId(_poolId) == true.

So to summarize the updatePoolAddress function reverts when the poolId does not exists and the verifyNewPool function reverts when the poolId exists, the two functions work on opposite conditions which means that when the verifyNewPool function is called inside the updatePoolAddress function it will automatically revert and hence the pool address of already existing poolId can never be updated.

Tools Used

Manual review

Recommended Mitigation Steps

Remove the verifyNewPool call inside the updatePoolAddress function and replace it with the following :

function updatePoolAddress(
    uint8 _poolId,
    address _newPoolAddress
) external override onlyExistingPoolId(_poolId) onlyRole(DEFAULT_ADMIN_ROLE) {
    UtilLib.checkNonZeroAddress(_newPoolAddress);
    // @audit revert only when mismatch in poolId
    if (INodeRegistry(IStaderPoolBase(_poolAddress).getNodeRegistry()).POOL_ID() != _poolId) {
        revert MismatchingPoolId();
    }
    poolAddressById[_poolId] = _newPoolAddress;
    emit PoolAddressUpdated(_poolId, _newPoolAddress);
}

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderConfig.sol#L85-L103

Vulnerability details

Impact

When initializing the StaderConfig contract with the initialize function, the admin address is not set in accountsMap[ADMIN] variable, so the getAdmin() function will return address(0). This will cause the loss of the ownership of the VaultProxy contract as its initialize function sets the owner to be staderConfig.getAdmin() which will be address(0).

Proof of Concept

The issue occurs in the initialize function :

File: StaderConfig.sol Line 85-103

function initialize(address _admin, address _ethDepositContract) external initializer {
    UtilLib.checkNonZeroAddress(_admin);
    UtilLib.checkNonZeroAddress(_ethDepositContract);
    __AccessControl_init();
    setConstant(ETH_PER_NODE, 32 ether);
    setConstant(PRE_DEPOSIT_SIZE, 1 ether);
    setConstant(FULL_DEPOSIT_SIZE, 31 ether);
    setConstant(TOTAL_FEE, 10000);
    setConstant(DECIMALS, 10 ** 18);
    setConstant(OPERATOR_MAX_NAME_LENGTH, 255);
    setVariable(MIN_DEPOSIT_AMOUNT, 10 ** 14);
    setVariable(MAX_DEPOSIT_AMOUNT, 10000 ether);
    setVariable(MIN_WITHDRAW_AMOUNT, 10 ** 14);
    setVariable(MAX_WITHDRAW_AMOUNT, 10000 ether);
    setVariable(WITHDRAWN_KEYS_BATCH_SIZE, 50);
    setVariable(MIN_BLOCK_DELAY_TO_FINALIZE_WITHDRAW_REQUEST, 600);
    setContract(ETH_DEPOSIT_CONTRACT, _ethDepositContract);
    _grantRole(DEFAULT_ADMIN_ROLE, _admin);
    // @audit accountsMap[ADMIN] is not set
}

As it can be seen from the code above, the functions grant the admin role to the _admin address but is does not update accountsMap[ADMIN] variable which by default is equal to address(0).

The VaultProxy contract is normally owned by the admin address fetched from the StaderConfig contract as it's shown below :

function initialise(
    bool _isValidatorWithdrawalVault,
    uint8 _poolId,
    uint256 _id,
    address _staderConfig
) external {
    if (isInitialized) {
        revert AlreadyInitialized();
    }
    UtilLib.checkNonZeroAddress(_staderConfig);
    isValidatorWithdrawalVault = _isValidatorWithdrawalVault;
    isInitialized = true;
    poolId = _poolId;
    id = _id;
    staderConfig = IStaderConfig(_staderConfig);
    // @audit owner is set to be equal to staderConfig admin
    owner = staderConfig.getAdmin();
}

And the getAdmin() functions directly returns the accountsMap[ADMIN] value :

function getAdmin() external view returns (address) {
    return accountsMap[ADMIN];
}

So because the accountsMap[ADMIN] variable was not set in the initialize function of the StaderConfig contract, it will be equal to address(0) which will lead to the loss of the ownership of the VaultProxy contract when it is initialized (basically burning ownership).

Tools Used

Manual review

Recommended Mitigation Steps

Set the admin address into the accountsMap[ADMIN] variable When initializing the StaderConfig contract, the initialize function shold be modified with the following :

function initialize(address _admin, address _ethDepositContract) external initializer {
    UtilLib.checkNonZeroAddress(_admin);
    UtilLib.checkNonZeroAddress(_ethDepositContract);
    __AccessControl_init();
    setConstant(ETH_PER_NODE, 32 ether);
    setConstant(PRE_DEPOSIT_SIZE, 1 ether);
    setConstant(FULL_DEPOSIT_SIZE, 31 ether);
    setConstant(TOTAL_FEE, 10000);
    setConstant(DECIMALS, 10 ** 18);
    setConstant(OPERATOR_MAX_NAME_LENGTH, 255);
    setVariable(MIN_DEPOSIT_AMOUNT, 10 ** 14);
    setVariable(MAX_DEPOSIT_AMOUNT, 10000 ether);
    setVariable(MIN_WITHDRAW_AMOUNT, 10 ** 14);
    setVariable(MAX_WITHDRAW_AMOUNT, 10000 ether);
    setVariable(WITHDRAWN_KEYS_BATCH_SIZE, 50);
    setVariable(MIN_BLOCK_DELAY_TO_FINALIZE_WITHDRAW_REQUEST, 600);
    setContract(ETH_DEPOSIT_CONTRACT, _ethDepositContract);
    _grantRole(DEFAULT_ADMIN_ROLE, _admin);
    // @audit set accountsMap[ADMIN] 
    setAccount(ADMIN, _admin);
}

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderOracle.sol#L637-L651

Vulnerability details

Impact

The function getPORFeedData() inside the StaderOracle does make a call to the Chainlink latestRoundData() function but doesn't contain checks on the returned price (if its equal to zero or not), round completeness and update timestamp, which could lead to returning a stale or wrong price and thus the protocol functions that rely on accurate price feed might not work as expected and sometimes can lead to a loss of funds.

Proof of Concept

The issue occurs in the getPORFeedData() function below :

function getPORFeedData() internal view returns (uint256, uint256, uint256) {
    (, int256 totalETHBalanceInInt, , , ) = AggregatorV3Interface(staderConfig.getETHBalancePORFeedProxy())
        .latestRoundData();
    (, int256 totalETHXSupplyInInt, , , ) = AggregatorV3Interface(staderConfig.getETHXSupplyPORFeedProxy())
        .latestRoundData();
    return (uint256(totalETHBalanceInInt), uint256(totalETHXSupplyInInt), block.number);
}

As you can see the function getPORFeedData() calls the function latestRoundData() to get the prices of the (ETH, ETHX) tokens using chainlink price feeds, but the function does not implement the required checks for verifying that the returned prices from the latestRoundData() call are not equal to zero and there is no check on the round timestamp and completeness to avoid outdated results, this can lead to stale prices according to Chainlink's documentation.

As the protocol uses the oracle prices to update the exchange rate with the updateERFromPORFeed(), it must receive accurate price feed and in the case when an oracle return a stale/wrong price this will have a negative impact as the new exchange rate will be wrong and can potentialy lead to a to a loss of funds.

Tools Used

Manual review

Recommended Mitigation Steps

Add the required checks whenever the latestRoundData() function is called in the aforementioned instances, the function getPORFeedData() should be updated as follows :

function getPORFeedData() internal view returns (uint256, uint256, uint256) {
    (uint80 roundID, int256 totalETHBalanceInInt, uint256 timestamp, uint256 updatedAt, ) = AggregatorV3Interface(staderConfig.getETHBalancePORFeedProxy())
        .latestRoundData();
    // @audit Add the required checks
    require(totalETHBalanceInInt > 0,"Chainlink answer reporting 0");
    require(updatedAt >= roundID, "Stale price");
    require(timestamp != 0,"Round not complete");
    require(
        block.timestamp - updatedAt <= 3600,
        "ORACLE_HEARTBEAT_FAILED"
    );
    
    (roundID, int256 totalETHXSupplyInInt, timestamp, updatedAt, ) = AggregatorV3Interface(staderConfig.getETHXSupplyPORFeedProxy())
        .latestRoundData();
    // @audit Add the required checks
    require(totalETHXSupplyInInt > 0,"Chainlink answer reporting 0");
    require(updatedAt >= roundID, "Stale price");
    require(timestamp != 0,"Round not complete");
    require(
        block.timestamp - updatedAt <= 3600,
        "ORACLE_HEARTBEAT_FAILED"
    );
    
    return (uint256(totalETHBalanceInInt), uint256(totalETHXSupplyInInt), block.number);
}

Assessed type

Oracle