Inconsistent Emergency Mode Handling in `withdraw` Function

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-05-loop/blob/0dc8467ccff27230e7c0530b619524cc8401e22a/src/PrelaunchPoints.sol#L290-L303

Vulnerability details

Description

The withdraw function in the PrelaunchPoints contract has an inconsistency in how it handles emergency mode for different token types. In emergency mode, the function treats ETH withdrawals differently compared to other tokens, such as other LRTs. This inconsistency can lead to unintended behavior and potential issues.

Let's examine the relevant code snippet:

function withdraw(address _token) external {
    if (!emergencyMode) {
        // Standard time checks
    } else {
        if (_token == ETH) {
            if (block.timestamp >= startClaimDate) {
                revert UseClaimInstead();
            }
        }
        // No similar check for other tokens
    }
    // Withdrawal logic...
}

In the code above, when the contract is in emergency mode, the function checks if the token being withdrawn is ETH. If it is ETH and the current timestamp is greater than or equal to startClaimDate, the function reverts with the UseClaimInstead error, indicating that the user should use the claim process instead of withdrawing.

However, the function does not apply the same logic to other token types (LRTs). This means that even if the contract is in emergency mode and startClaimDate has passed, users can still withdraw wrapped LRTs, without any restrictions.

Impact

The inconsistent handling of emergency mode in the withdraw function can have significant consequences:

• In emergency mode, after startClaimDate has passed, users should be directed to use the claim process for all token types. However, the current implementation allows users to withdraw wLRT tokens even when they should be using the claim process. This can lead to unintended token withdrawals and potential loss of control over the token management.

• The inconsistent behavior of the withdraw function in emergency mode can lead to confusion and frustration for users. They may expect all token withdrawals to be restricted after startClaimDate in emergency mode, but the current implementation allows them to withdraw wLRT tokens, leading to an inconsistent user experience.

Proof of Concept

Here is the code snippet: https://github.com/code-423n4/2024-05-loop/blob/0dc8467ccff27230e7c0530b619524cc8401e22a/src/PrelaunchPoints.sol#L284-L303

Let's consider the following scenario:

1. The PrelaunchPoints contract is in emergency mode.
2. The startClaimDate has already passed, indicating that all token withdrawals should be handled through the claim process.
3. A user attempts to withdraw rsETH tokens by calling the withdraw function with _token set to the rsETH token address.
4. The function does not have any specific checks for wLRT tokens in emergency mode, so the withdrawal of rsETH tokens is allowed.
5. The user successfully withdraws rsETH tokens (or any unintended behavior), bypassing the intended restriction of using the claim process (in lpETH) after startClaimDate in emergency mode.

This scenario illustrates how the inconsistent handling of emergency mode in the withdraw function can allow users to withdraw wLRT tokens even when they should be using the claim process.

Tools Used

Manual Review

Recommended Mitigation Steps

Simply create a modifier for emergency mode (EMERGENCY_MODIFIER) and add this to the withdraw function and rewrite the function like this, (I also added zero address check and the valid token checks for extra security)

    function withdraw(address _token) external EMERGENCY_MODIFIER {

        if ( _token == address(0) && !isTokenAllowed[_token]) {
            revert InvalidToken();
        }

        uint256 lockedAmount = balances[msg.sender][_token];
        balances[msg.sender][_token] = 0;

        if (lockedAmount == 0) {
            revert CannotWithdrawZero();
        }

        if (block.timestamp >= startClaimDate) {
             revert UseClaimInstead();
        }

        if (_token == ETH) {
            totalSupply = totalSupply - lockedAmount;
            (bool sent,) = msg.sender.call{value: lockedAmount}("");
            if (!sent) {
                revert FailedToSendEther();
            }
        } else {
            IERC20(_token).safeTransfer(msg.sender, lockedAmount);
        }

        emit Withdrawn(msg.sender, _token, lockedAmount);
    }

Assessed type

Context

[c4-judge]

koolexcrypto marked the issue as duplicate of #17

[c4-judge]

koolexcrypto marked the issue as unsatisfactory: Invalid