howlbot-integration[bot] commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-05-loop/blob/40167e469edde09969643b6808c57e25d1b9c203/src/PrelaunchPoints.sol#L262

Vulnerability details

Impact

The bug can lead to incorrect issuance of lpETH tokens during the claim process, potentially resulting in financial loss or discrepancy for users. If the contract holds any ETH before a swap, the claimedAmount calculation will be incorrect, causing either over-minting or under-minting of lpETH.

Root of the bug

the bug is in the assumption that the entire ETH balance of the contract post-swap corresponds to the amount of _token swapped. And The current implementation does not account for any pre-existing ETH balance in the contract. We have The _claim function is designed to allow users to claim their vested lpETH. If the _token is ETH, it calculates claimedAmount based on the user's share of totalLpETH. If the _token is not ETH, it calls _fillQuote to swap the token to ETH and then converts the received ETH to lpETH.

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;

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

The vulnerability Is in this line :


claimedAmount = address(this).balance;

Here in this calculation assumes that the entire ETH balance of the contract after calling _fillQuote is the result of the token swap. This assumption fails if the contract had any pre-existing ETH balance.

The calculation does not isolate the ETH received from the swap but instead uses the total ETH balance of the contract, which can be misleading if there was already ETH in the contract before the swap.

Proof of Concept

Here is the scenario that I test with:

Scenario Demonstration

Consider the following scenario:

let’s say The contract has a pre-existing balance of 10 ETH.
A user claims 1,000 units of _token that are swapped for 1 ETH via _fillQuote.
Post-swap, the contract's balance becomes 11 ETH.
The line claimedAmount = address(this).balance; assigns claimedAmount as 11 ETH.
lpETH is minted based on 11 ETH instead of the correct 1 ETH, leading to over-minting of lpETH.
And as result: Claimed Amount (ETH): 11 Contract lpETH Balance: 11 It’s show the incorrect claimed amount due to pre-existing ETH in the contract.

Tools Used

Manual review

Recommended Mitigation Steps

the ETH received from the swap should be calculated separately

Assessed type

Other

c4-judge commented 4 months ago

koolexcrypto marked the issue as duplicate of #18

c4-judge commented 4 months ago

koolexcrypto marked the issue as partial-75