User can Claim Entire Eth Balance of Contract instead of just their own Eth Claim Value

[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#L503

Vulnerability details

Impact

Risk of Fund Loss to Protocol as User can Claim Entire Eth Balance of Contract instead of just their own Eth Claim Value in the PrelaunchPoints.sol contract during claim function call.

Proof of Concept

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

            // Convert swapped ETH to lpETH (1 to 1 conversion)
>>>            claimedAmount = address(this).balance; ❌
            lpETH.deposit{value: claimedAmount}(_receiver);
        }
        emit Claimed(msg.sender, _token, claimedAmount);
    }

The function above shows how _claim(...) function is implemented in the PrelaunchPoints.sol contract, it can be noted from the pointers that when token is Non Eth a swap is done before a claim of Eth value is done and deposited to receiver address, from the first pointer, the protocol assumes there should not be any Eth in the contract which is why the last pointer assigns the complete balance of eth in contract as claimed Amount instead of the actual Amount that is to be claimed. However the implementation of _fillQuote(...) function which handles the swap to determine the Eth value to be claimed as provided below paints a different picture of the Eth that should be in balance. As noted from the pointers below the value of Eth Bought is derived by comparing the Eth balance before and after without assuming it is previously empty, this is the correct way to derived the actual Eth that should be claimed. Now back to the _claim(...) function above, it is this boughtETHAmount from _fillQuote(...) function that should be used as claimedAmount value and not using the entire balance of the contract as wrongly done in the last pointer above which risks a user claiming the whole contract value instead of just its personal funds, a bad actor simply needs to keep track of the contract balance to claim at the perfect time to get excess fund above its claimable range

function _fillQuote(IERC20 _sellToken, uint256 _amount, bytes calldata _swapCallData) internal {
        // Track our balance of the buyToken to determine how much we've bought.
>>>        uint256 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);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

As adjusted in the code below the actual boughtEthAmount from sell token should be returned by the _fillQuote(...) function and this value should be used to represent claimedAmount and not the overall balance of the contract as previously done

--- function _fillQuote(IERC20 _sellToken, uint256 _amount, bytes calldata _swapCallData) internal {
+++ function _fillQuote(IERC20 _sellToken, uint256 _amount, bytes calldata _swapCallData) internal  returns (uint256 boughtETHAmount ) {
        ...
        // Use our current buyToken balance to determine how much we've bought.
        boughtETHAmount = address(this).balance - boughtETHAmount;
        emit SwappedTokens(address(_sellToken), _amount, boughtETHAmount);
    }

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) {
            ...
        } 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);
+++          uint256 boughtETHAmount =  _fillQuote(IERC20(_token), userClaim, _data); 
            // Convert swapped ETH to lpETH (1 to 1 conversion)
---            claimedAmount = address(this).balance; 
+++            claimedAmount = boughtETHAmount; 
            lpETH.deposit{value: claimedAmount}(_receiver);
        }
        emit Claimed(msg.sender, _token, claimedAmount);
    }

Assessed type

ETH-Transfer

[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 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

[Topmark1]

I believe this finding is worth full reward as it noted the major problem which is the use of address(this).balance for claimedAmount just as noted in the primary report at https://github.com/code-423n4/2024-05-loop-findings/issues/33 and it offers more explanation and elaboration than the bot primary report at https://github.com/code-423n4/2024-05-loop-findings/issues/18 which is also getting full reward, and some other examples just to mention a few, Great Judging @koolexcrypto thanks for the hard work

[koolexcrypto]

Hi @Topmark1

The issues doesn't clearly explain where the funds in the contract comes from. It is just assumes to have funds which shouldn't happen normally. For this, it is not fair to give it full credit.

[c4-judge]

koolexcrypto marked the issue as partial-25

[Topmark1]

I don't seem to understand why this issue is being downgraded again instead of an upgrade???, you gave reasons for giving it a partial 50 which I would have argued but don't want to extended the PQA for a mere duplicate but suddenly downgrading it even lower seems rather unfair, thanks