Incorrect ETH Handling in Claim Function of PrelaunchPoints Contract When `_token != ETH`

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-05-loop/blob/main/src/PrelaunchPoints.sol#L262 https://github.com/code-423n4/2024-05-loop/blob/main/src/PrelaunchPoints.sol#L439-L441

Vulnerability details

Impact

In the _claim function of the PrelaunchPoints contract, if the swap operation within the _claim function sends ETH to the zero address (address(0)) instead of the contract address (address(this)), the user's ETH converted via the wrapped LRT is irretrievably lost. This results in a direct financial loss for the user attempting the claim, and the intended claimedAmount calculation fails because the contract does not receive the ETH. Furthermore, this error undermines the reliability of the contract and can significantly impact user trust, as the contract does not perform as expected, leading to failed transactions and potential financial detriment to users.

Proof of Concept

Consider a scenario where a user triggers the _claim function, expecting to convert a locked token into ETH and then into lpETH. The function flow is as follows:

1. Token Swap Execution: The function calls an external swap service to convert a specified amount of a locked LRT token to ETH.
2. ETH Receipt Handling: The expected behavior is for the swapped ETH to be sent to address(this). However, due to a configuration error, the ETH is sent to address(0) where _validateData() will not revert.

https://github.com/code-423n4/2024-05-loop/blob/main/src/PrelaunchPoints.sol#L439-L441

        if (recipient != address(this) && recipient != address(0)) {
            revert WrongRecipient(recipient);
        }

3. Balance Assignment: After the swap, the contract assigns claimedAmount based on its current ETH balance using address(this).balance.
4. Failed Claim: If the ETH was incorrectly sent to address(0), the contract's balance remains unchanged (typically 0 still), leading to an erroneous claimedAmount calculation—often resulting in a zero value.

https://github.com/code-423n4/2024-05-loop/blob/main/src/PrelaunchPoints.sol#L257-L263

            // At this point there should not be any ETH in the contract
            // Swap token to ETH
            _fillQuote(IERC20(_token), userClaim, _data);

            // Convert swapped ETH to lpETH (1 to 1 conversion)
            claimedAmount = address(this).balance; // @audit
            lpETH.deposit{value: claimedAmount}(_receiver);

Recommended Mitigation Steps

Modify the _validateData function to ensure that all swap operations explicitly specify address(this) as the recipient. Implement checks to prevent address(0) from being used in critical transactions where asset receipt is expected.

https://code4rena.com/audits/2024-05-loopfi/submit

-        if (recipient != address(this) && recipient != address(0)) {
+        if (recipient != address(this)) {

Alternatively, ensure that boughtETHAmount in _fillQuote() isn't zero:

https://github.com/code-423n4/2024-05-loop/blob/main/src/PrelaunchPoints.sol#L503

        boughtETHAmount = address(this).balance - boughtETHAmount;
+        require(boughtETHAmount != 0);

Assessed type

Invalid Validation

[0xd4n1el]

We assumed the data we send is correct, so this is very unlikely. The fix can be applied for QA.

[c4-judge]

koolexcrypto marked the issue as primary issue

[c4-judge]

koolexcrypto changed the severity to QA (Quality Assurance)

[c4-judge]

koolexcrypto marked the issue as grade-c

[mystery0x]

@koolexcrypto High impact + Low likelihood should be rated a medium finding instead of a low. Please reconsider it. Thanks.

[imsrybr0]

@mystery0x isn't this being taken care of by the 0x UniswapV3 feature recipient normalization?

[koolexcrypto]

Hi @mystery0x

User input error is considered as QA or invalid

Findings that require the user to be careless or enter the wrong information into a contract call are QA or Invalid

https://docs.code4rena.com/awarding/judging-criteria/severity-categorization#conditional-on-user-mistake