NodeOperator will steal other NodeOperators' validators through frontrunning

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PermissionlessNodeRegistry.sol#L125-L172

Vulnerability details

Impact

People that want to earn staking rewards, but do not have the resources to run validators, will see Stader as an avenue where they can easily steal and use others validators and they will actually earn Operator rewards from Stader.
This could lead to two things

• Original validator owner would continue performing his duties, but he would be laboring for another person. And if he decides to withdraw, the validator thief would lose nothing
• Original validator owner would want to behave maliciously to punish the validator thief. But, this would affect Stader because when the validator gets slashed below 28 eth, stakers' funds would be at risk

Proof of Concept

If a NodeOperator calls addValidatorKeys to use validators that he operates, or that he trusts, another NodeOperator that sees the pending transaction in the mempool, could frontrun the original NodeOperator by calling addValidatorKeys with the same parameters, and the validator would be registered under him, while the original NodeOperator's transaction would get reverted.

function addValidatorKeys(
        bytes[] calldata _pubkey,
        bytes[] calldata _preDepositSignature,
        bytes[] calldata _depositSignature
    ) external payable override nonReentrant whenNotPaused {
    ...
    IPermissionlessPool(staderConfig.getPermissionlessPool()).preDepositOnBeaconChain{
            value: staderConfig.getPreDepositSize() * keyCount
        }(_pubkey, _preDepositSignature, operatorId, operatorTotalKeys);
}

Since the addValidatorKeys function does not perform any sanity checks on the caller, and the eth2.0 deposit contract's caller is Stader contracts, there is no way to know the true owner of a pubkey.

ATTACK SCENARIO

• Node Operator Alice with _pubkeyA signs _preDepositSignatureA and _depositSignatureA
• Alice calls addValidatorKeys(_pubkeyA,_preDepositSignatureA, _depositSignatureA)
• Node Operator Bob sees the transaction in mempool, and calls addValidatorKeys(_pubkeyA,_preDepositSignatureA, _depositSignatureA) with higher gas
• Bob's transaction gets executed before Alice's so even though Alice is the true owner of _pubkeyA, it get registered for Bob
• Since the signatures are valid, it would be considered valid in eth2.0
• If Alice's Validator continues performing its duties, or decides to exit, Bob gains his eth back plus rewards
• If Alice decides to perform maliciously to punish Bob, and gets slashed to 16 eth on Beacon Chain, Bob loses his 4 eth while Stader loses 12 eth.

Tools Used

Manual Review

Mitigation Steps

Consider creating a library for BLS12-381 curves using EIP 2537. With that, you may require the Node Operator to sign a confirmation signature using the private key he used to sign the _preDepositSignature. If the pubkey extracted from the confirmation signature is equal to the pubkey he provided, then he is the true owner of the validator and should be activated.

Assessed type

Access Control

[manoj9april]

As mentioned in the ATTACK_SCENARIO, 5th point is not valid as stader computes withdrawVaultAddress at validator level, which is dependent on operatorID.
WithdrawVault will be different for both attacker and victim NO.
Hence the signature will not be valid as they are signed on different withdrawalCredential.

[c4-sponsor]

manoj9april marked the issue as sponsor disputed

[c4-judge]

Picodes marked the issue as unsatisfactory:
Invalid

[Emedudu]

Hello. I'm not sure this issue was fully understood.
There is no way to trace any of the addValidatorKeys parameters to msg.sender and because of that, if a Node Operator calls addValidatorKeys function with pubKey, preDepositSignature, and depositSignature, an attacker who sees the transaction in mempool can call the function with same parameters, 4 eth stake(not required in PermissionedNodeRegistry), but with higher gas, and that would cause the attacker's transaction to be executed before the legitimate Node Operator's, while legitimate Node Operator's transaction gets reverted.
Stader and Eth Beacon contract only checks if those parameters are valid, but do not know who the owner is and this is why the transaction will pass.
This is the reason why no staking solution platform uses this method to add validators. But with BLS12-381 curves, this would be possible as Validator signatures can be verified right on chain. You may read this short blog to gain more understanding of this issue
The consequence of this is that anyone can EASILY steal other users' validators through front running and since validators are important assets in liquid staking, and victims of the attack cannot recover them(they can't change withdrawalCredentials), and bug occurs in both PermissionedNodeRegistry and PermissionlessNodeRegistry, I believe this qualifies as HIGH impact as per code4rena#Estimating risk

[Picodes]

Hi @Emedudu, thanks for reaching out. This is what I originally understood, and I still think the attack vector you are describing cannot be used, but feel free to add more context if I am missing something.

The withdrawal credentials depends on the original msg.sender because the withdrawalVaultAddress depends on the salt which depends on the operatorId.

Therefore, the withdrawalCredentials depends on the msg.sender, so any front-running would invalidate the pre-deposit signature (which is over the public key, the withdrawal credentials, and the deposit amount). Therefore a front-runner would just loose the predeposit amount.

[Emedudu]

If this attack happens in PermissionlessNodeRegistry, call won't revert, attacker loses 1 eth on Beacon chain, and the original Node Operator won't also be able to use that validator(because it has been registered for attacker on Stader).

And if it happens in PermissionedNodeRegistry, attacker would cause same effect to original Node Operator, and lose nothing(instead, waste 1 eth of users' stake).
When Stader Operator markValidatorReadyToDeposit, it would report invalidSignature for that NodeOperator. The result of this is that NodeOperator will be able to replay this attack, and drain users' stake(by wasting 1 eth of staked funds each time), because for invalidSignatures, NodeOperators are not deactivated(unlike for frontrunDeposits)

[Picodes]

Thanks a lot for detailing these additional scenarios @Emedudu. Note that this would be another issue as the original scenario is still invalid. Furthermore, this would not drain user funds directly as the invalid signature case is paid by the insurance fund.

Tagging @sanjay-staderlabs for visibility.

[Emedudu]

this would not drain user funds directly as the invalid signature case is paid by the insurance fund.

This does not stop users' staked funds from being drained because this is the execution flow:

• attacker frontruns Node Operator in calling addValidatorKeys, and gets assigned that validator on stader.
• calling StaderStakePoolsManager#validatorBatchDeposit will call PermissionedPool#stakeUserETHToBeaconChain which will make a preDepositOnBeaconChain, using users' funds which are staked in StaderStakePoolsManager contract
• At this point(to the best of my understanding), nothing stops attacker from doing this again, which will waste 1 eth of staked funds in StaderStakePoolsManager each time.
• Assuming attacker waits until markValidatorRadyForDeposit is called, attacker is still not deactivated because attack will be reported under invalidSignature which does not deactivate attacker from being a NodeOperator.
• This means that attacker is FREE to replay this attack many times, wasting 1 eth of staked funds each time.

I don't know if I'm missing something, but from what I can see, losses due to invalid signatures getting insured by StaderInsuranceFund does not stop anyone from performing this attack (which steals funds directly from StaderStakePoolsManager)

Note that this would be another issue as the original scenario is still invalid

Indeed most of this explanation is a different valid CRITICAL issue which could have been titled: "Malicious Node Operator can drain users' stake by calling addValidatorKeys with an invalid preDepositSignature many times"(I hope sponsor @sanjay-staderlabs confirms and appreciates this🙂).
But I also want to point out that the main issue:"NodeOperator will steal other NodeOperators' validators through frontrunning" is also valid because as explained above, the attacker will actually deny other Node Operators from using their validators on stader, risking 1 eth for the attack in PermissionlessNodeRegistry, and risking nothing in PermissionedNodeRegistry. Note also that attacker won't be penalized in any way in PermissionedNodeRegistry based on the current implementations, as reporting invalidSignatures will not deactivate NodeOperator.