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

1 stars 1 forks source link

verifyWithdrawalCredentialsAndBalance does not verify that oracleBlockNumber is the latest block number. #409

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

After participating in Ethereum Staking, you may receive shares based on your balance when it was 32 ETH, even though you may have suffered a certain level of slashing at 32 ETH.

Until some conscientious watcher proves the slashing in the Consensus Layer in the EigenPod Contract, it seems that the middleware could be temporarily negatively impacted by using the shares issued when the balance was 32 ETH.

This can be blocked by the slashQueuedWithdrawal function and slashShares function of the StrategyManager Contract, so it does not permanently negatively affect the middleware, but it can negatively affect the middleware until these functions are executed.

Proof of Concept

function _testDeployAndVerifyNewEigenPod(address _podOwner, bytes memory _signature, bytes32 _depositDataRoot)
        internal returns (IEigenPod)
    {
        // (beaconStateRoot, beaconStateMerkleProofForValidators, validatorContainerFields, validatorMerkleProof, validatorTreeRoot, validatorRoot) =
        //     getInitialDepositProof(validatorIndex);

        BeaconChainProofs.ValidatorFieldsAndBalanceProofs memory proofs = _getValidatorFieldsAndBalanceProof();
        validatorFields = getValidatorFields();
        bytes32 newBeaconStateRoot = getBeaconStateRoot();
        uint40 validatorIndex = uint40(getValidatorIndex());
        BeaconChainOracleMock(address(beaconChainOracle)).setBeaconChainStateRoot(newBeaconStateRoot);

        cheats.startPrank(_podOwner);
        eigenPodManager.stake{value: stakeAmount}(pubkey, _signature, _depositDataRoot);
        cheats.stopPrank();

        IEigenPod newPod;
        newPod = eigenPodManager.getPod(_podOwner);

        uint64 blockNumber = 1;
        vm.roll(10000);
        newPod.verifyWithdrawalCredentialsAndBalance(blockNumber, validatorIndex, proofs, validatorFields);
    }

Tools Used

Manual audit

Recommended Mitigation Steps

It is recommended to verify that the block number is relatively recent.

Assessed type

Invalid Validation

0xSorryNotSorry commented 1 year ago

The demonstration of the issue is not clear. If I understood correctly, the warden points out that the latest oracleBlockNumber should be checked instead of mostRecentWithdrawalBlockNumber which is updated only at withdraw().

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as primary issue

c4-sponsor commented 1 year ago

Sidu28 marked the issue as sponsor disputed

Sidu28 commented 1 year ago

This is by design. Forcing usage of the latest block number is impossible due to delays in posting beacon chain state roots to the execution layer. Additionally, users may need to prove against past roots, e.g. in the case where they previously withdrew a validator from the beacon chain, but have not yet called verifyWithdrawalCredentialsAndBalance for the validator in question.

GalloDaSballo commented 1 year ago

With the information I have available:

Meaning that no issue was demonstrated

Feel free to follow up if I missed something

c4-judge commented 1 year ago

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof