User may claim more LpETH due to ETH being mistakenly sent to the contract

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-05-loop/blob/0dc8467ccff27230e7c0530b619524cc8401e22a/src/PrelaunchPoints.sol#L392
https://github.com/code-423n4/2024-05-loop/blob/0dc8467ccff27230e7c0530b619524cc8401e22a/src/PrelaunchPoints.sol#L259-L263
https://github.com/code-423n4/2024-05-loop/blob/0dc8467ccff27230e7c0530b619524cc8401e22a/src/PrelaunchPoints.sol#L503-L504
https://github.com/code-423n4/2024-05-loop/blob/0dc8467ccff27230e7c0530b619524cc8401e22a/src/PrelaunchPoints.sol#L321-L324

Vulnerability details

Impact

If a user mistakenly sends ETH to the contract before convertAllETH has been called by the owner, every user could claim more lpETH than expected. If he mistakenly sends ETH to the contract after convertAllETH has been called by the owner, the first user to claim after him could get more lpETH.

Proof of Concept

The contract is able to receive ETH.

    /**
     * Enable receive ETH
     * @dev ETH sent to this contract directly will be locked forever.
     */
    receive() external payable {}

However, if a user mistakenly sends ETH to the contract, this would affect the claiming process.

Consider the following 2 cases:

1. If a user mistakenly sends ETH to the contract before convertAllETH has been called by the owner, every user could claim more lpETH than expected.

   When convertAllETH is being called by the owner, all ETH in the contract will be converted to lpETH. When a user claims his locked ETH afterwards, the calculation claimedAmount = userStake.mulDiv(totalLpETH, totalSupply) will give him more lpETH than actual amount.

        uint256 totalBalance = address(this).balance;
        lpETH.deposit{value: totalBalance}(address(this));
        totalLpETH = lpETH.balanceOf(address(this));

2. If he mistakenly sends ETH to the contract after convertAllETH has been called by the owner, the first user to claim after him could get more lpETH.

   When _claim is later being called, lpETH.deposit{value: claimedAmount}(_receiver) has been called to convert all ETH in the contract to lpETH. Thus the first user to claim afterwards could get more lpETH than actual amount.

            claimedAmount = address(this).balance;
            lpETH.deposit{value: claimedAmount}(_receiver);

Tools Used

Manual, VSCode

Recommended Mitigation Steps

Three steps:

1. Since totalSupply is used to record ETH balance, it is better to use totalSupply to convert to lpETH.
2. Use the boughtETHAmount obtained in _fillQuote to convert to ETH.
3. Add a retrieve function to retrieve address(this).balance-totalSupply before claiming and address(this).balance after claiming.

Assessed type

ETH-Transfer

[c4-judge]

koolexcrypto marked the issue as duplicate of #18

[c4-judge]

koolexcrypto changed the severity to 3 (High Risk)

[c4-judge]

koolexcrypto changed the severity to 2 (Med Risk)

[c4-judge]

koolexcrypto marked the issue as partial-50

[c4-judge]

koolexcrypto changed the severity to 3 (High Risk)

[c4-judge]

koolexcrypto marked the issue as duplicate of #33

[c4-judge]

koolexcrypto marked the issue as not a duplicate

[c4-judge]

koolexcrypto changed the severity to QA (Quality Assurance)

[c4-judge]

koolexcrypto marked the issue as grade-c