Skip to content

Latest commit

 

History

History
39 lines (27 loc) · 3.27 KB

104.md

File metadata and controls

39 lines (27 loc) · 3.27 KB
Raw

Rich Bubblegum Tardigrade

Medium

Possible wrong accounting in L1Staking.sol

Summary

Possible wrong accounting in L1Staking.sol during some slashing occasions.

Vulnerability Detail

Stakers are permitted to commit batches in the rollup contract and these batches can be challenged by challengers, if the challenge is successful the challenger gets part of the stakers ETH and staker is removed, the owner also takes the rest; if it wasn't succesful the prover takes all the challengeDeposit from the challenger. According to the readMe https://github.com/sherlock-audit/2024-08-morphl2-Pascal4me#q-are-there-any-limitations-on-values-set-by-admins-or-other-roles-in-the-codebase-including-restrictions-on-array-lengths proofWindow which is the time a challenged batch has to go without being proven for it to be successfully challenged can be set to as high as 604800 seconds which is 7 days and withdrawalLockBlocks default value is also 7 days(No relation between the two was done and owner can change proofWindow value in rollup contract). For this vulnerability we'll assume proofWindow is set to 7 days. The issue stems from a staker being able to commit a wrong batch and still being able to withdraw from the staking contract without that batch being finalized or proven. Let's site this example with proofWindow being set to 7 days

  • Alice a staker commits a wrong batch and immediately goes ahead to withdraw her ETH from the staking contract, her 7 days period countdown start
  • Bob a challenger sees that wrong batch and challenges it and it since it's a wrong batch theres no proof for it, then 7 days couuntdown starts. But remember the withdraw function 7 days started counting first so when it elapses Alice quickly goes ahead to withdraw her ETH, then when Bob calls proveState(), Alice is supposedly slashed but it's useless as she's already left the system. Then the contract is updated as though Alice's ETH is still in the contract

uint256 reward = (valueSum * rewardPercentage) / 100; slashRemaining += valueSum - reward; _transfer(rollupContract, reward);

So a current staker's ETH is the one being sent to the rollup contract and being assigned to the owner via slashRemaining. So there's less ETH than the contract is accounting for which already an issue. This will be detrimental when all the ETH is being withdrawn by stakers and owner the last person transaction will revert becuase that ETH is not in the contract.

Impact

Incorrect contract accounting

Code Snippet

https://github.com/sherlock-audit/2024-08-morphl2/blob/main/morph/contracts/contracts/l1/staking/L1Staking.sol#L197-L201 https://github.com/sherlock-audit/2024-08-morphl2/blob/main/morph/contracts/contracts/l1/rollup/Rollup.sol#L484-L487 https://github.com/sherlock-audit/2024-08-morphl2/blob/main/morph/contracts/contracts/l1/rollup/Rollup.sol#L697-L707 https://github.com/sherlock-audit/2024-08-morphl2/blob/main/morph/contracts/contracts/l1/staking/L1Staking.sol#L307-L311 https://github.com/sherlock-audit/2024-08-morphl2/blob/main/morph/contracts/contracts/l1/staking/L1Staking.sol#L217-L237

Tool used

Manual Review

Recommendation

Ensure stakers can't withdraw if they have a batch that is unfinalized or unproven and always ensure that withdraw time block is > proofWindow