Closed code423n4 closed 1 year ago
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.
manoj9april marked the issue as sponsor disputed
Picodes marked the issue as unsatisfactory: Invalid
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
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.
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
)
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.
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:
addValidatorKeys
, and gets assigned that validator on stader.StaderStakePoolsManager#validatorBatchDeposit
will call PermissionedPool#stakeUserETHToBeaconChain
which will make a preDepositOnBeaconChain
, using users' funds which are staked in StaderStakePoolsManager
contractStaderStakePoolsManager
each time.markValidatorRadyForDeposit
is called, attacker is still not deactivated because attack will be reported under invalidSignature
which does not deactivate attacker from being a NodeOperator
.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.
Hi @Emedudu, major thing that you are missing is permissioned operator are very few selected one, no one can be permissioned operator, these are the operator with long history of running nodes. We dont expect these operator to perform wrongly. even if they do user fund are never at stake, as this is also pointed by @Picodes insurance fund take care of such cases of lost 1ETH.
@sanjay-staderlabs thanks for your reply, but read this:
major thing that you are missing is permissioned operator are very few selected one, no one can be permissioned operator, these are the operator with long history of running nodes. We dont expect these operator to perform wrongly.
Sure, it is not expected that they perform wrongly, but if they do, through validator theft, there is no mechanism in place to punish them(Unlike frontrunPubkey
that punishes by deactivating validators). So, nothing actually stops or restricts them from performing this attack(Please correct me if I'm wrong)
even if they do user fund are never at stake, as this is also pointed by @Picodes insurance fund take care of such cases of lost 1ETH
I'm not expecting any rewards for this because this issue was found after the contest, but I want to point out once again that since preDepositOnBeaconChain
is called in PermissionedPool#stakeUserETHToBeaconChain
, which is called by StaderStakePoolsManager#validatorBatchDeposit
, the lost 1 eth would be from StaderStakePoolsManager, so when insurance calls reimburseUserFund
, it would send the lost amount from insurance to PermissionedPool.
NOTE that StaderInsuranceFund
does not have unlimited amount of ether, so since attacker can replay attack, it will get to a point when StaderInsuranceFund
will not have any more ether to reimburse PermissionedPool
, so at this point, if attacker continues attack, StaderStakePoolsManager
will keep on wasting 1 eth each time, until it becomes bankrupt.
By the way, I don't think it is a good thing for someone to deliberately force StaderInsuranceFunds
to pay ether that they lost.
Hi @Emedudu, if you check this piece of code, you can find that we can deactivate permissionedNode operator if they do such thing.
@sanjay-staderlabs Supposing there is a lot of staked funds(say 1000ETH),
attacker will call addValidatorKeys
with a very long array of pubkey and invalid preDepositSignatures, which would cause transaction to be executed within a block, before manager deactivates malicious operator.
Even though stader would deactivate operator, a lot of funds would have already been lost
@Emedudu the debate has deviate from the original report which focuses on "validator stealing", which doesn't work as shown above.
Now, on the possibility of front running to grief the system:
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
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.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
_pubkeyA
signs_preDepositSignatureA
and_depositSignatureA
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