Report on Incorrect Assumption in Contract Functionality giving users more LPETH THAN THEIR SWAPPED AMOUNT

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-05-loop/blob/0dc8467ccff27230e7c0530b619524cc8401e22a/src/PrelaunchPoints.sol#L257-L262
https://github.com/code-423n4/2024-05-loop/blob/0dc8467ccff27230e7c0530b619524cc8401e22a/src/PrelaunchPoints.sol#L388-L392
https://github.com/code-423n4/2024-05-loop/blob/0dc8467ccff27230e7c0530b619524cc8401e22a/src/PrelaunchPoints.sol#L491-L505

Vulnerability details

Impact

The assumption made by the developer in line 257 of the contract is flawed and could lead to users getting more lpeth token than they should. This assumption incorrectly assumes that no ETH will be present in the contract at a certain point, which may not hold true as ETH can be forwarded to the contract at any time, even after this function(convertAllETH) is called

/**
     * @dev Called by a owner to convert all the locked ETH to get lpETH
     */
    function convertAllETH() external onlyAuthorized onlyBeforeDate(startClaimDate) .

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

Consequently, users could benefit from receiving more ETH than the actual swapped amount, leading to potential exploitation of donated ETH.

Proof of Concept

The flawed assumption is evident in the following code snippet:

   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
            _fillQuote(IERC20(_token), userClaim, _data);

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

This code forwards all ETH present in the contract to the user and converts it to LPETH in a 1:1 ratio, without considering the actual value that was swapped.

   _fillQuote(IERC20(_token), userClaim, _data);

This can result in users receiving more ETH than they are entitled to, as all available ETH in the contract is converted to LPETH for the user.

Tools Used

Manual code analysis

Recommended Mitigation Steps

To address the flawed assumption and ensure users receive the correct amount of ETH based on the actual value that was swapped, the following mitigation steps are recommended:

1. Update _fillQuote Function: Modify the _fillQuote function to return the actual amount of ETH bought (boughtETHAmount). This ensures that the contract accurately tracks the amount of ETH received from the swap.

   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);
   
       
   }

2. Update Conversion Logic: Modify the conversion logic in the contract to use the actual amount of ETH bought (boughtETHAmount) instead of the total ETH balance in the contract. This ensures that users receive the correct amount of LPETH based on the actual value of the swap.

   uint256 boughtETHAmount = _fillQuote(IERC20(_token), userClaim, _data);
   claimedAmount = boughtETHAmount;
   lpETH.deposit{value: claimedAmount}(_receiver);

Assessed type

Other

[c4-judge]

koolexcrypto marked the issue as duplicate of #18

[c4-judge]

koolexcrypto changed the severity to 2 (Med Risk)

[c4-judge]

koolexcrypto marked the issue as partial-75

[c4-judge]

koolexcrypto changed the severity to 3 (High Risk)

[c4-judge]

koolexcrypto marked the issue as duplicate of #33

[Tomiwasa0]

Thanks for judging @koolexcrypto,
i believe this deserves full credit and not a partial, kindly review.

[c4-judge]

koolexcrypto marked the issue as partial-25

[koolexcrypto]

Thanks for judging @koolexcrypto, i believe this deserves full credit and not a partial, kindly review.

No mention of bypassing locking duration. Can not give full credit.