code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The role of the updatePoolAddress function is to update the address of a pool.

However, when attempting to update an existing pool, the function does not behave correctly and consistently reverts with the error message ExistingOrMismatchingPoolId().

Proof of Concept

The issue lies within the verifyNewPool function that is part of the updatePoolAddress function. https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PoolUtils.sol#L49-L65

/**
 * @notice Update the address of a pool.
 * @dev This function should only be called by the `DEFAULT_ADMIN_ROLE` role
 * @param _poolId The Id of the pool to update.
 * @param _newPoolAddress The updated address of the pool.
 */
function updatePoolAddress(uint8 _poolId, address _newPoolAddress)
    external
    override
    onlyExistingPoolId(_poolId)
    onlyRole(DEFAULT_ADMIN_ROLE)
{
    UtilLib.checkNonZeroAddress(_newPoolAddress);
    verifyNewPool(_poolId, _newPoolAddress);
    poolAddressById[_poolId] = _newPoolAddress;
    emit PoolAddressUpdated(_poolId, _newPoolAddress);
}

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

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

This function checks the validity of the pool ID and pool address, ensuring that they do not already exist.

However, in the case of the updatePoolAddress function, it expects the pool ID to already exist, causing the isExistingPoolId(_poolId) check to always return true.

Consequently, the function consistently reverts with the error message ExistingOrMismatchingPoolId().

Tools Used

Recommended Mitigation Steps

To address this issue, you can modify the code by removing the usage of the verifyNewPool function and implementing the following mitigation code:

function updatePoolAddress(uint8 _poolId, address _newPoolAddress) 
    external 
    override
    onlyExistingPoolId(_poolId)
    onlyRole(DEFAULT_ADMIN_ROLE)
{
    UtilLib.checkNonZeroAddress(_newPoolAddress);

    require(poolAddressById[_poolId] != _newPoolAddress, "New address is the same as the current address");

    require(INodeRegistry(IStaderPoolBase(_newPoolAddress).getNodeRegistry()).POOL_ID() == _poolId, "Mismatching Pool ID")
    // Update the pool address
    poolAddressById[_poolId] = _newPoolAddress;

    emit PoolAddressUpdated(_poolId, _newPoolAddress);
}

Assessed type

Invalid Validation

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #341