Node Operators can `addValidatorKeys` with an invalid `_depositSignature`, which would lead to loss of users' funds.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PermissionlessNodeRegistry.sol#L128 https://github.com/code-423n4/2023-06-stader/blob/main/contracts/PermissionedNodeRegistry.sol#L143

Vulnerability details

Impact

Node Operators can addValidatorKeys with a valid _pubkey, a valid _preDepositSignature but an invalid _depositSignature, leading to loss of users' funds. This attack may/may not be intentional. Here is a good reason why someone would gladly want to do this. Although the Node Operator would lose his 4 eth bond + (0.4 -> 2) eth worth of SD, 28 eth of users' stake will also be lost, so the protocol would be affected far more than the malicious Node Operator. Considering the amount of users' funds that could potentially be lost by a single careless or rich malicious Node Operator, and the possibility of even draining the StaderStakePoolsManager contract, I believe this is of HIGH impact.

Proof of Concept

FUNCTIONALITY/WORKFLOW

• A node operator calls addValidatorKeys which assigns to the validator the nextValidatorId and then make a predeposit of 1 eth using the _preDepositSignature.

  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);
  }

• The node operator then waits for Operator(multisig) to markValidatorReadyForDeposit:

  function markValidatorReadyToDeposit(
      bytes[] calldata _readyToDepositPubkey,
      bytes[] calldata _frontRunPubkey,
      bytes[] calldata _invalidSignaturePubkey
  ) external override nonReentrant whenNotPaused {

• The Operator checks for frontrun pubkeys i.e. pubkeys that have their withdrawal credentials pointed to another address other than the one stader defined by querying this API. The Operator also checks if the predeposit signature is valid by querying another API. More info in Stader Blog
• Then, if the _pubkey passes these checks, operator adds the validator to a queue for its full deposit of 31 eth to be processed by calling markValidatorReadyForDeposit.
• Anyone can call validatorBatchDeposit or depositETHOverTargetWeight to loop through the validator queue and deposit 31 eth for each validator using their _depositSignature.

THE PROBLEM

The _depositSignature provided should have been signed with a private key. This private key would later be required to sign the withdrawal transaction. Info from this site.

The PROBLEM is that if a Node Operator addValidatorKeys with an invalid _depositSignature(a signature that was not signed with pubkey's private key), he would be successfully added to the validator queue, and when validatorBatchDeposit is called, 28 eth of users stake + 3 eth of Node Operator's bond would be deposited using the _depositSignature he provided. This 31 eth would be burned on eth1, but considered "invalid" on ETH2 due to the invalid signature provided.

Going through the code for the eth2.0 deposit contract, the actual signature is not verified there in the contract(only the length is checked) but rather on the Beacon Chain nodes. The reason for this is that EVM did not support BLS curve operations.

This would make StaderStakePoolManager#validatorBatchDeposit or StaderStakePoolManager#depositETHOverTargetWeight to successfully execute, but the funds will never be withdrawable.

Although Operators check that predeposit signature is valid by querying an API before calling markValidatorReadyForDeposit, there is no check to know if the _depositSignature is valid.

Here is what happened to eth of a person that attempted to deposit using an invalid signature

ATTACK SCENARIO

• Node Operator signs a valid _preDepositSignature using his validator private key
• Node Operator calls addValidatorKeys with his validator pubkey, _preDepositSignature he signed, and a _depositSignature which is a random signature with a length of 96 bytes.
• Node Operator's predeposit of 1 eth executes successfully.
• Operator multisig sees that predeposit executed successfully, and that the withdrawal credentials points to one created by stader. So Operator Node Operator validator's status to PRE_DEPOSIT
• Anyone calls validatorBatchDeposit or depositETHOverTargetWeight, and Node Operator's full deposit of 31 eth is executed by calling ETH2.0DepositContract#deposit(pubkey,withdrawal_credentials,_depositSignature) which will not revert because no signature validation is done.
• Now, out of the 32 eth that was burned in eth1, eth2 would consider only 1 eth as valid, but the remaining 31 eth would be considered invalid. Check this, this, and this
• Node Operator lost 4 eth while Stader lost 28 eth.

Tools Used

Manual Review

Recommended Mitigation Steps

EIP-2537 will offer the main curve operations that will enable the protocol to verify the actual signature and reject any attempts to use an invalid validator signature right on chain. This will enable innovations like decentralized staking pools based on distributed-key-generation. The protocol may have to work towards making EIP-2537 BLS curve libraries, or wait till one is created before this innovation would be feasible and risk free. More info in this blog post. When that is accomplished, both _preDepositSignature and _depositSignature should be verified and validated on chain before deposit calls to eth2.0 deposit contract are made.

Assessed type

Other

[manoj9april]

after first valid deposit on beacon chain, it does not verify signature of other deposit, it just increases the validator balance

[c4-sponsor]

manoj9april marked the issue as sponsor disputed

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[Picodes]

@manoj9april I am not sure to understand your remark. Although there are economic safeguards to prevent this, what prevents an invalid _depositSignature from being used and ultimately lead to a costly griefing attack?

[c4-judge]

Picodes marked the issue as satisfactory

[JGcarv]

As I understand, since an oracle will verify that the pre-deposit signature is a valid BLS signature, I don't see why it couldn't also verify the deposit sig, before marking it as ready to deposit, but I'm not that deep into ETH2.0, so I might be wrong.

[Picodes]

So from what I understand from what the sponsor sent me (thanks!) and what I've read, a validator's signature is checked only once as it is only a mitigation against rogue public keys attacks. Therefore, as in our case the validators already had a pre-deposit, an invalid signature isn't an issue.

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid