User can get as much `lpETH` as he wants with low locked amount of tokens

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-05-loop/blob/main/src/PrelaunchPoints.sol#L240-L266 https://github.com/code-423n4/2024-05-loop/blob/main/src/PrelaunchPoints.sol#L388-L392

Vulnerability details

Impact

User can get as much lpETH as he wants with low locked amount of tokens, sabotaging the whole idea of the locking mechanism

Proof of Concept

User can easily bypass the locking mechanism by locking a low amount of tokens (different than WETH). If he lock his tokens and wait for the convertAllETH function to be called he can additionally send ETH to the contract and then immediately call the claim/claimAndStake function to claim the lpETH. this is possible because of the following block of code in the _claim function:

uint256 userClaim = (userStake * _percentage) / 100;
            _validateData(_token, userClaim, _exchange, _data);
            balances[msg.sender][_token] = userStake - userClaim;

            // 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 first person to claim, gets it
            claimedAmount = address(this).balance;
            lpETH.deposit{value: claimedAmount}(_receiver);

It deposits the whole ETH balance of the contract to the recipient in form of lpETH, which means that the user's token deposit and the ETH he sent right before calling the claim function are now his in the form of lpETH, which sabotages the whole idea of the locking mechanism

Tools Used

manual review

Recommended Mitigation

Just sending the ETH to the owner via receive function wont be enough, since a malicious user can transfer ETH to the contract in other ways (for example create a contract with implemented selfdestruct function). The best I can think of is to make the _fillQuote function return the boughtETHAmount variable, and then deposit that amount to the receiver like this:

_fillQuote function:

function _fillQuote(
        IERC20 _sellToken,
        uint256 _amount,
        bytes calldata _swapCallData
    ) internal returns (uint256 boughtETHAmount) {
        // Track our balance of the buyToken to determine how much we've bought.
        boughtETHAmount = address(this).balance;

        require(_sellToken.approve(exchangeProxy, _amount));

        (bool success, ) = payable(exchangeProxy).call{value: 0}(_swapCallData);
        if (!success) {
            revert SwapCallFailed();
        }

        // Use our current buyToken balance to determine how much we've bought.
        boughtETHAmount = address(this).balance - boughtETHAmount;
        emit SwappedTokens(address(_sellToken), _amount, boughtETHAmount);
    }

_claim function:

    function _claim(
        address _token,
        address _receiver,
        uint8 _percentage,
        Exchange _exchange,
        bytes calldata _data
    ) internal returns (uint256 claimedAmount) {
        uint256 userStake = balances[msg.sender][_token];
        if (userStake == 0) {
            revert NothingToClaim();
        }
        if (_token == ETH) {
            claimedAmount = userStake.mulDiv(totalLpETH, totalSupply);
            balances[msg.sender][_token] = 0;
            lpETH.safeTransfer(_receiver, claimedAmount);
        } else {
            uint256 userClaim = (userStake * _percentage) / 100;
            _validateData(_token, userClaim, _exchange, _data);
            balances[msg.sender][_token] = userStake - userClaim;

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

            lpETH.deposit{value: claimedAmount}(_receiver);
        }
        emit Claimed(msg.sender, _token, claimedAmount);
    }

This way the excessive ETH will remain in the contract (as stated in the receive function natspec) and the users wont be able to bypass the locking mechanism by locking a small amount of tokens and the sending ETH to the contract

Assessed type

Other

[c4-judge]

koolexcrypto marked the issue as duplicate of #6

[c4-judge]

koolexcrypto marked the issue as duplicate of #33

[c4-judge]

koolexcrypto marked the issue as satisfactory