`LiquidityPool.batchRegisterAsBnftHolder` function doesn't check if the caller is a registered BNFT holder

Github username: -- Submission hash (on-chain): 0x9fe23589dd836660bbf8dfa5fa57a050ce038d0196420d793aa59ca9c99aaa00 Severity: low

Description: Description\

LiquidityPool.batchRegisterAsBnftHolder function is designed to let BNFT players register validators they have deposited, and it will triggers a 1 ETH transaction to the beacon chain.
This function is supposed to be called only by a BNFT player on IDs that have been deposited.
But it was noticed that anyone can call batchRegisterAsBnftHolder without being a rgistered BNFT holder:

LiquidityPool.batchRegisterAsBnftHolder function

    function batchRegisterAsBnftHolder(
        bytes32 _depositRoot,
        uint256[] calldata _validatorIds,
        IStakingManager.DepositData[] calldata _registerValidatorDepositData,
        bytes32[] calldata _depositDataRootApproval,
        bytes[] calldata _signaturesForApprovalDeposit
    ) external whenNotPaused {
        require(_validatorIds.length == _registerValidatorDepositData.length && _validatorIds.length == _depositDataRootApproval.length && _validatorIds.length == _signaturesForApprovalDeposit.length, "lengths differ");

        stakingManager.batchRegisterValidators(_depositRoot, _validatorIds, msg.sender, address(this), _registerValidatorDepositData, msg.sender);

        //For each validator, we need to store the deposit data root of the 31 ETH transaction so it is accessible in the approve function
        for(uint256 i; i < _validatorIds.length; i++) {
            depositDataRootForApprovalDeposits[_validatorIds[i]] = _depositDataRootApproval[i];
            emit ValidatorRegistered(_validatorIds[i], _signaturesForApprovalDeposit[i], _registerValidatorDepositData[i].publicKey, _depositDataRootApproval[i]);
        }
    }

Attack Scenario

So if any registered BNFT holder adds a deposit to register validators (by calling batchDepositAsBnftHolder function) , then this BNFT holder got removed by the liquidity pool admin (by calling deRegisterBnftHolder function); this removed BNFT holder will still be able to register validators.

Attachments

Revised Code

Update LiquidityPool.batchRegisterAsBnftHolder function to check if the caller is a registered BNFT holder:

function batchRegisterAsBnftHolder(
     bytes32 _depositRoot,
     uint256[] calldata _validatorIds,
     IStakingManager.DepositData[] calldata _registerValidatorDepositData,
     bytes32[] calldata _depositDataRootApproval,
     bytes[] calldata _signaturesForApprovalDeposit
) external whenNotPaused {
     require(_validatorIds.length == _registerValidatorDepositData.length && _validatorIds.length == _depositDataRootApproval.length && _validatorIds.length == _signaturesForApprovalDeposit.length, "lengths differ");

+       require(bnftHoldersIndexes[msg.sender].registered, "Not registered");
     stakingManager.batchRegisterValidators(_depositRoot, _validatorIds, msg.sender, address(this), _registerValidatorDepositData, msg.sender);

     //For each validator, we need to store the deposit data root of the 31 ETH transaction so it is accessible in the approve function
     for(uint256 i; i < _validatorIds.length; i++) {
           depositDataRootForApprovalDeposits[_validatorIds[i]] = _depositDataRootApproval[i];
           emit ValidatorRegistered(_validatorIds[i], _signaturesForApprovalDeposit[i], _registerValidatorDepositData[i].publicKey, _depositDataRootApproval[i]);
     }
}

Add a mechanism to enable deregisterd BFT holders from claiming their deposited ethers if they haven't registered any validators.

hats-finance / ether-fi-0x36c3b77853dec9c4a237a692623293223d4b9bc4

`LiquidityPool.batchRegisterAsBnftHolder` function doesn't check if the caller is a registered BNFT holder #47