code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/pods/EigenPod.sol#L175-L226

Vulnerability details

Impact

Attacker would get shares in StrategyManager without staking any real funds. This would allow him to earn rewards or act maliciously without fear of getting slashed.

Proof of Concept

Here is the verifyWithdrawalCredentialsAndBalance function: L175-L226

EigenPod's verifyWithdrawalCredentialsAndBalance function checks if the validator's current balance, which he can withdraw from BeaconChain, is greater or equal to the REQUIRED_BALANCE_GWEI, then verifies that the withdrawal credentials of the validator at the oracleBlockNumber points to the EigenPod, then calls eigenPodManager.restakeBeaconChainETH which increments the podOwner's shares in StrategyManager contract.

The problem is that the function only checks that the validator's withdrawalCredentials at an arbitrary oracleBlockNumber points to the eigenPod(L189). So the validator is only required to have

at least REQUIRED_BALANCE_GWEI BeaconChainETH currently(with withdrawal credentials set to any address),
should have staked some ETH, at least once in the past, with withdrawal credentials set to the EigenPod. It does not matter if the eth has been slashed or not.

A validator can create a pod using EigenManager's createPod function, and if he has enough eth(>=REQUIRED_BALANCE_GWEI) at the current block with withdrawalCredentials pointed to any address he wants, he can input a stale oracleBlockNumber at which his withdrawalCredentials was pointing to the EigenPod, with corresponding proofs and validatorFields from that block, and the function will pass and shares will be minted for the podOwner. The function still passes when the validator's stake at oracleBlockNumber with withdrawal credentials pointing to the EigenPod has already been slashed at a later block(oracleBlockNumber+x). As a result, the validator is not risking any funds because the withdrawal credentials for his currently staked eth is not set to the EigenPod, and the one he staked at oracleBlockNumber has already been slashed in the past.

Since the user now has shares without staking his ETH, He would earn "free" rewards from EigenLayer, or would act maliciously because he did not stake any real value.

Tools Used

Manual Review

Recommended Mitigation Steps

For this issue, I would recommend two fixes to verifyWithdrawalCredentialsAndBalance:

It is safer to verify against the current beaconStateRoot, instead of allowing user to input an arbitrary oracleBlockNumber. An alternative is to make sure oracleBlockNumber is not stale, as it was done in verifyOvercommittedStake:
```
require(oracleBlockNumber + VERIFY_OVERCOMMITTED_WINDOW_BLOCKS >= block.number,
        "EigenPod.verifyOvercommittedStake: specified blockNumber is too far in past");
```


- I would also recommend not minting shares immediately `verifyWithdrawalCredentialsAndBalance` is called, but there should be a function to manually mint the shares once the verifyOvercommittedStake function gets inactive i.e. 50400 blocks(7 days) has passed. This would allow the WithdrawalCredentials to be verified before user participates in EigenLayer.

## Assessed type

Invalid Validation

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as primary issue

Sidu28 commented 1 year ago

A modifier was added to address this specific kind of concern. It is present here

c4-sponsor commented 1 year ago

Sidu28 marked the issue as sponsor disputed

c4-judge commented 1 year ago

GalloDaSballo marked the issue as unsatisfactory: Invalid

GalloDaSballo commented 1 year ago

Agree with the Sponsor

Emedudu commented 1 year ago

Hello, I don't think the modifier that was added is adequate. Here is the proofIsForValidBlockNumber modifier:

modifier proofIsForValidBlockNumber(uint64 blockNumber) {
        require(blockNumber > mostRecentWithdrawalBlockNumber,
            "EigenPod.proofIsForValidBlockNumber: beacon chain proof must be for block number after mostRecentWithdrawalBlockNumber");
        _;
    }

mostRecentWithdrawalBlockNumber is only updated within the withdrawBeforeRestaking function. Consequently, if the podOwner refrains from calling the withdrawBeforeRestaking function, proofIsForValidBlockNumber will always succeed since the default value of mostRecentWithdrawalBlockNumber is 0. As a result, proofIsForValidBlockNumber will generally have no practical significance.

Consider adding a check in proofIsForValidBlockNumber that mostRecentWithdrawalBlockNumber is not 0.

GalloDaSballo commented 1 year ago

Hello, I don't think the modifier that was added is adequate. Here is the proofIsForValidBlockNumber modifier:
modifier proofIsForValidBlockNumber(uint64 blockNumber) {
        require(blockNumber > mostRecentWithdrawalBlockNumber,
            "EigenPod.proofIsForValidBlockNumber: beacon chain proof must be for block number after mostRecentWithdrawalBlockNumber");
        _;
    }
mostRecentWithdrawalBlockNumber is only updated within the withdrawBeforeRestaking function. Consequently, if the podOwner refrains from calling the withdrawBeforeRestaking function, proofIsForValidBlockNumber will always succeed since the default value of mostRecentWithdrawalBlockNumber is 0. As a result, proofIsForValidBlockNumber will generally have no practical significance.

Consider adding a check in proofIsForValidBlockNumber that mostRecentWithdrawalBlockNumber is not 0.

@ChaoticWalrus sorry to keep bothering you, what do you think about this? Do you think this is realistic?

(Btw this should be the last issue standing)

ChaoticWalrus commented 1 year ago

@ChaoticWalrus sorry to keep bothering you, what do you think about this? Do you think this is realistic?

The withdrawBeforeRestaking() function has its own modifier that prevents it being called after any restaking proofs are verified. It is the hasNeverRestaked modifier, which is defined here https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/pods/EigenPod.sol#L114-L117 and applied here https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/pods/EigenPod.sol#L454

So the cases are: 1) withdrawBeforeRestaking is called first, in which case mostRecentWithdrawalBlockNumber is non-zero 2) verifyWithdrawalCredentialsAndBalance is called first, in which case the ETH cannot have been withdrawn out of the EigenPod, and withdrawBeforeRestaking cannot subsequently be called, ever.

So I'm not seeing any need for a modification here.

(Btw this should be the last issue standing)

Thank you, really glad to hear this.

ChaoticWalrus commented 1 year ago

For additional context, the code that prevents withdrawBeforeRestaking being called after verifyWithdrawalCredentialsAndBalance also includes these lines https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/pods/EigenPod.sol#L217-L220 which set hasRestaked = true inside of the verifyWithdrawalCredentialsAndBalance function.

Emedudu commented 1 year ago

Hello If the user calls verifyWithdrawalCredential without calling withdrawBeforeRestaking, there would be no restrictions

Emedudu commented 1 year ago

This is withdrawBeforeRestaking:

function withdrawBeforeRestaking() external onlyEigenPodOwner hasNeverRestaked {
        mostRecentWithdrawalBlockNumber = uint32(block.number);
        _sendETH(podOwner, address(this).balance);
    }

If there is no eth in the eigenpod, there would be no reason to call withdrawBeforeRestaking.

ChaoticWalrus commented 1 year ago

Hello If the user calls verifyWithdrawalCredential without calling withdrawBeforeRestaking, there would be no restrictions

There is no restriction on the oracleBlockNumber in this case, as you originally stated. I think we agree on that.

If there is no eth in the eigenpod, there would be no reason to call withdrawBeforeRestaking.

Yes, also agree on this.

I suspect we are talking past each other, at least to some degree.

It seems like the general critique falls on the existence of a concept we term 'overcommitments', for which we have an honest watcher assumption and have chosen to not restrict who can call the verifyOvercommittedStake or verifyAndProcessWithdrawal. Information on overcommitments is in our documentation, particularly here https://github.com/code-423n4/2023-04-eigenlayer/blob/main/docs/EigenPods.md#merkle-proofs-for-overcommitted-balances

Emedudu commented 1 year ago

It seems like the general critique falls on the existence of a concept we term 'overcommitments', for which we have an honest watcher assumption and have chosen to not restrict who can call the verifyOvercommittedStake or verifyAndProcessWithdrawal. Information on overcommitments is in our documentation, particularly here https://github.com/code-423n4/2023-04-eigenlayer/blob/main/docs/EigenPods.md#merkle-proofs-for-overcommitted-balances

I agree with this. But a user would get 32 eth worth of shares for as little as 0 staked eth, and even though there are watchers, there will surely be a delay between when user verifyWithdrawalCredentialsAndBalance, and when the watcher verifyOvercommitment. During this delay period, user could be getting rewards, or choose to act maliciously without fear of getting slashed. I really recommend delaying eigenPodManager.restakeBeaconChainETH(podOwner, REQUIRED_BALANCE_WEI); by probably 50400 blocks to allow watchers to do their job and to eliminate chances of this occuring You may also modify proofIsForValidBlockNumber because I'm not sure that is the intended behaviour.

ChaoticWalrus commented 1 year ago

I agree with this. But a user would get 32 eth worth of shares for as little as 0 staked eth, and even though there are watchers, there will surely be a delay between when user verifyWithdrawalCredentialsAndBalance, and when the watcher verifyOvercommitment. During this delay period, user could be getting rewards, or choose to act maliciously without fear of getting slashed. I really recommend delaying eigenPodManager.restakeBeaconChainETH(podOwner, REQUIRED_BALANCE_WEI); by probably 50400 blocks to allow watchers to do their job and to eliminate chances of this occuring You may also modify proofIsForValidBlockNumber because I'm not sure that is the intended behaviour.

Super. Personally am viewing this more as a difference of opinion at this point, but your suggestion is something we could consider; really appreciate you taking the time to discuss and dig in a bit more.

The one key point that I want to emphasize here -- which contributed significantly to the decisions we've made in the present design -- is that the user you are describing would necessarily have had 32 ETH at some time and gotten slashed down below 31 ETH. Importantly, this means the user has already incurred an ETH loss strictly greater in magnitude than the amount by which they would be briefly 'overcommitted' in EigenLayer. I describe this as there being no 'free lunch' in the system 😄

Emedudu commented 1 year ago

In most cases, the current design is okay. But I'll suggest that external factors, like a user getting slashed on Beacon chain should have as little negative impact as possible on the system.

Importantly, this means the user has already incurred an ETH loss strictly greater in magnitude than the amount by which they would be briefly 'overcommitted' in EigenLayer. I describe this as there being no 'free lunch' in the system 😄

Yes there may be no 'free lunch' for the user, and most times there is no incentive to do this, but the system could still be negatively impacted.

GalloDaSballo commented 1 year ago

Per the Sponsors comment: Caller calls verifyWithdrawalCredentialsAndBalance first -> They are participating as validator and have a valid balance They will not be able to call withdrawBeforeRestaking The flag validatorStatus[validatorIndex] prevents replay

Caller calls withdrawBeforeRestaking first -> The block number is increased so older block proofs cannot be used

At this point I would recommend you investigate the finding further and follow up with me or the Sponsor privately if you find something

But with the info I have available, I believe the finding is invalid

Emedudu commented 1 year ago

Caller calls verifyWithdrawalCredentialsAndBalance first -> They are participating as validator and have a valid balance They will not be able to call withdrawBeforeRestaking.

The caller can actually call verifyWithdrawalCredentialsAndBalance with an invalid balance.

GalloDaSballo commented 1 year ago

Agree with no free lunch

Also I must put a stop to this dynamic because the submitted finding doesn't have the same information as in the comments

You cannot submit a new finding after the contest is over, the additional info must be contained in the original submission

At this point I would recommend you follow up after the contest if you can write a POC, but I cannot accept the finding as valid due to lack of proof

GalloDaSballo commented 1 year ago

I have been messaged by the warden privately, and believe that:

The submission is incorrect in some ways which lead me to forcefully ignore the rest of the info in the comments
Even if we used the most generous possible interpretation of all findings, the worst case scenario is the shares of a slashed operator, that self-rekt, being credited until an overcommitment is registered (theoretically this can happen instantly, just as much as it can be delayed), meaning that the operator may, for a brief moment have a balance that is not synched with their actual balance, but as a whole, the system is resilient to this desynch

For the above am maintaining the finding as invalid

code-423n4 / 2023-04-eigenlayer-findings

Attacker can operate as a staker/operator on eigenLayer without risking any funds #374

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps