`accountsMap[ADMIN]` not set in `initialize` function of `StaderConfig` contract

[code423n4]

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

[c4-judge]

Picodes marked the issue as duplicate of #171

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to 2 (Med Risk)