code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/UserWithdrawalManager.sol#L174-L176 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/UserWithdrawalManager.sol#L165-L181 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/UserWithdrawalManager.sol#L106

Vulnerability details

high

Title: The incorrect check implemented in the UserWithdrawalManager undermines the effectiveness of the timelock for claiming ETH.

Links:

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/UserWithdrawalManager.sol#L174-L176 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/UserWithdrawalManager.sol#L165-L181 https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/UserWithdrawalManager.sol#L106

Impact

Any malicious users can bypass the timelock mechanism for claiming ETH. The issue lies in the claim() function, where it only checks for ethExpected to be zero and not ethFinalized.

Proof of Concept

To exploit this vulnerability, a user can initiate the process by calling the requestWithdraw function,
which sets the ethExpected value of the next withdrawal request to a specific amount of "assets."

...
    userWithdrawRequests[nextRequestId] = UserWithdrawInfo(  
        payable(_owner),//owner
        _ethXAmount, //ethXAmount
        assets,//ethExpected
        0,
        block.number
    );
    requestIdsByUserAddress[_owner].push(nextRequestId);

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/UserWithdrawalManager.sol#L106

With this setup, the following check inside the claim function will pass:

...
    if (userRequest.ethExpected == 0) {
            revert RequestAlreadyRedeemed(_requestId);
    }

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/UserWithdrawalManager.sol#L174-L176

This allows the attacker to bypass the timelock and claim ETH without restrictions.

Recommended Mitigation Steps

Change the check inside claim() to:

    if (userRequest.ethFinalized == 0) {
        revert RequestAlreadyRedeemed(_requestId);
    }

Assessed type

ETH-Transfer

manoj9april commented 1 year ago

issuer confused eth finalized as the finaliazation step than amount at finalization.

c4-sponsor commented 1 year ago

manoj9april marked the issue as sponsor disputed

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Invalid

code-423n4 / 2023-06-stader-findings