howlbot-integration[bot] commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-05-loop/blob/0dc8467ccff27230e7c0530b619524cc8401e22a/src/PrelaunchPoints.sol#L439 https://github.com/code-423n4/2024-05-loop/blob/0dc8467ccff27230e7c0530b619524cc8401e22a/src/PrelaunchPoints.sol#L259 https://github.com/code-423n4/2024-05-loop/blob/0dc8467ccff27230e7c0530b619524cc8401e22a/src/PrelaunchPoints.sol#L497 https://github.com/code-423n4/2024-05-loop/blob/0dc8467ccff27230e7c0530b619524cc8401e22a/src/PrelaunchPoints.sol#L459

Vulnerability details

Impact

If the _validateData function receives a decoded recipient address of address(0) from _decodeUniswapV3Data and does not revert, the contract will proceed with the swap, and the resulting ETH will be sent to address(0), effectively burning the ETH. This could lead to a loss of funds for users attempting to claim their lpETH.

Proof of Concept

In the _validateData function, the recipient address is checked against address(this) and address(0):

function _validateData(address _token, uint256 _amount, Exchange _exchange, bytes calldata _data) internal view {
        // ...
        if (recipient != address(this) && recipient != address(0)) {
            revert WrongRecipient(recipient);
        }
    }

The _decodeUniswapV3Data function can potentially return address(0) as the recipient:

function _decodeUniswapV3Data(bytes calldata _data)
        internal
        pure
        returns (address inputToken, address outputToken, uint256 inputTokenAmount, address recipient, bytes4 selector)
    {
      // ...
      recipient := calldataload(add(p, 64))
      // ...
    }

If address(0) is returned and not reverted, the swap will send ETH to address(0):

function _fillQuote(IERC20 _sellToken, uint256 _amount, bytes calldata _swapCallData) internal {
        // ...
        (bool success,) = payable(exchangeProxy).call{value: 0}(_swapCallData);
        if (!success) {
            revert SwapCallFailed();
        }
        // ...
    }

The condition in _validateData does not revert when recipient is address(0), which is an oversight. The intention is likely to ensure that the recipient is either the contract itself or not set (implicitly address(0)), but this logic allows for the possibility that ETH could be sent to address(0) if the _swapCallData is crafted to do so.

Tools Used

Manual Review

Recommended Mitigation Steps

Modify the _validateData function to explicitly check that the recipient is address(this) only in the if (_exchange == Exchange.UniswapV3) branch and revert if not. This ensures that the contract does not allow ETH to be sent to address(0):

function _validateData(address _token, uint256 _amount, Exchange _exchange, bytes calldata _data) internal view {
        address inputToken;
        address outputToken;
        uint256 inputTokenAmount;
-       address recipient;
        bytes4 selector;

        if (_exchange == Exchange.UniswapV3) {
-           (inputToken, outputToken, inputTokenAmount, recipient, selector) = _decodeUniswapV3Data(_data);
+           (inputToken, outputToken, inputTokenAmount, address recipient, selector) = decodeUniswapV3Data(_data);
            if (selector != UNI_SELECTOR) {
                revert WrongSelector(selector);
            }
            if (outputToken != address(WETH)) {
                revert WrongDataTokens(inputToken, outputToken);
            }
+           if (recipient != address(this)) {
+           revert WrongRecipient(recipient);
+           }
        } 
       // ...
-       if (recipient != address(this) && recipient != address(0)) {
-           revert WrongRecipient(recipient);
-       }
    }

This change will prevent the contract from proceeding with a swap that would result in sending ETH to address(0), thereby safeguarding users' funds during the claim process.

Assessed type

ETH-Transfer