110.md

Wobbly Topaz Mule

Medium

L1Staking.claimWithdrawal becomes executable 1 block later than it should be per the invariants

Summary

The problem is the comparison operator that is used in the claimWithdrawals checks series:

        require(withdrawals[_msgSender()] < block.number, "withdrawal locked");

in the claimWithdrawal function, the withdrawer can only claim a withdrawal when not only the withdrawalLockBlocks N of blocks have passed, but also at least 1 more block has elapsed since that end time.

Vulnerability Detail

That is because < is used instead of <=, which in this case is not accurate nor correct.

Impact

Although it doesn't pose funds lose risks, but it violates an invariant that a withdrawer can claimWithdrawal of their own once block.number + withdrawalLockBlocks has come in terms of time, and that is intuitively evident by looking at this comment:

State Variable: withdrawals

Contract: L1Staking /// @notice withdraw unlock block height

Which implies that when that withdraw unlock block height is REACHED, the staker can ALREADY claim a withdrawal --- he shouldn't have to wait for 1 more block to elapse.

---

It is evident speicifically thanks to one more thing, in addition to the protocol's "unlock block height" comment about withdrawals:

The thing is that a similar approach of setting claimable/executable time was also applied to the stakers deletion logic, namely storing "deletableAtBlockHeight" block numbers in the deleteableHeight array:

    /// @notice clean staker store
    function _cleanStakerStore() internal {
        uint256 i = 0;
        while (i < deleteList.length) {
            if (deleteableHeight[deleteList[i]] <= block.number) {
                // clean stakerSet
                delete stakerSet[stakerIndexes[deleteList[i]] - 1];
                delete stakerIndexes[deleteList[i]];

                // clean staker info
                delete stakers[deleteList[i]];

                // clean deleteList
                delete deleteableHeight[deleteList[i]];
                deleteList[i] = deleteList[deleteList.length - 1];
                deleteList.pop();
            } else {
                i++;
            }
        }
    }

Here can see the discrepancy in timing logic among the contract functions at this line: if (deleteableHeight[deleteList[i]] <= block.number).

The <= operator is used.

Likely, in L2Staking, a similar >= approach is used in multiple places:

    /// @notice start reward
    function startReward() external onlyOwner {
        require(block.timestamp >= rewardStartTime, "can't start before reward start time");

That is why I believe that in claimWithdrawal there should be a withdrawals[_msgSender()] <= block.number check instead of withdrawals[_msgSender()] < block.number.

Code Snippet

https://github.com/sherlock-audit/2024-08-morphl2/blob/98e0ec4c5bbd0b28f3d3a9e9159d1184bc45b38d/morph/contracts/contracts/l1/staking/L1Staking.sol#L309

Tool used

Manual review.

Recommendation

Update the claimWithdrawal function accordingly for the stakers to be able to timely claim withdrawals of their stakes:

    /// @notice claim withdrawal
    /// @param receiver  receiver address
    function claimWithdrawal(address receiver) external nonReentrant {
        require(withdrawals[_msgSender()] > 0, "withdrawal not exist");
-       require(withdrawals[_msgSender()] < block.number, "withdrawal locked");
+       require(withdrawals[_msgSender()] <= block.number, "withdrawal locked");

        delete withdrawals[_msgSender()];
        _cleanStakerStore();

        emit Claimed(_msgSender(), receiver);

        _transfer(receiver, stakingValue);
    }