Open code423n4 opened 1 year ago
Well demonstrated with referrable code snippets, hyperlinks and coded POC. Marking as HQ.
0xSorryNotSorry marked the issue as high quality report
0xSorryNotSorry marked the issue as primary issue
Sidu28 marked the issue as sponsor confirmed
The Warden has shown how, due to a lack of length check, an empty proof could be provided, which would pass validation
This is an example of how a lack of a check can be chained into a proper exploit, and because the proof will pass, funds can be stolen
For these reasons I agree with High Severity
GalloDaSballo marked the issue as selected for report
Lines of code
https://github.com/code-423n4/2023-04-eigenlayer/blob/5e4872358cd2bda1936c29f460ece2308af4def6/src/contracts/libraries/Merkle.sol#L80-L87 https://github.com/code-423n4/2023-04-eigenlayer/blob/5e4872358cd2bda1936c29f460ece2308af4def6/src/contracts/libraries/BeaconChainProofs.sol#L245-L295 https://github.com/code-423n4/2023-04-eigenlayer/blob/5e4872358cd2bda1936c29f460ece2308af4def6/src/contracts/pods/EigenPod.sol#L305-L359
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
ifproof.lenght < 32
(e.g. empty proof) andleaf == root
. Although this might be intended behaviour, I see no use case for empty proofs and wouldrequire
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 ofproofs.slotProof
andproofs.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. theproofs
can be modified in the following way: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 ofexecutionPayloadRoot
, theproofIsForValidBlockNumber
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 emptyblockNumberProof
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 emptyslotProof
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 withforge test --match-contract EigenPod
:We can see that all the test cases are still passing, whereby the following ones are confirming the aforementioned outcomes:
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:
Alternative: Non-empty proofs can also be required in the
Merkle
library.Assessed type
Invalid Validation