code-423n4 / 2024-05-loop-findings

4 stars 4 forks source link

Mismatch in Total Supply and Total lpETH Due to WETH Conversion #59

Closed howlbot-integration[bot] closed 3 months ago

howlbot-integration[bot] commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-05-loop/blob/0dc8467ccff27230e7c0530b619524cc8401e22a/src/PrelaunchPoints.sol#L188-L191

Vulnerability details

Impact

The contract's handling of WETH conversions and the subsequent update of totalSupply and totalLpETH can lead to a mismatch between the total ETH and equivalent in the system and the actual lpETH held by the contract. This can affect users' expectations when claiming or withdrawing, potentially leading to issues with the amount of lpETH received or unexpected reverts.

Proof of Concept

In the _processLock function, when locking WETH: https://github.com/code-423n4/2024-05-loop/blob/0dc8467ccff27230e7c0530b619524cc8401e22a/src/PrelaunchPoints.sol#L188-L191

if (_token == address(WETH)) {
    WETH.withdraw(_amount); // Converts WETH to ETH
    totalSupply = totalSupply + _amount;
    balances[_receiver][ETH] += _amount;
}

Here, the contract withdraws WETH to ETH and adds the amount to totalSupply and the user's balance as if it were ETH. This is correct in terms of balance management but does not account for the potential change in totalLpETH when convertAllETH is called.

However, when convertAllETH is called, all ETH held by the contract is converted into lpETH, and totalLpETH is updated:

https://github.com/code-423n4/2024-05-loop/blob/0dc8467ccff27230e7c0530b619524cc8401e22a/src/PrelaunchPoints.sol#L321-L324

uint256 totalBalance = address(this).balance;
lpETH.deposit{value: totalBalance}(address(this));
totalLpETH = lpETH.balanceOf(address(this));

This function converts all ETH held by the contract into lpETH and updates totalLpETH. However, if there were any operations affecting the ETH balance between deposits (like WETH withdrawals that are converted to ETH and then potentially used or lost in transactions), the totalLpETH might not accurately reflect the totalSupply of ETH that users expect to be backed by lpETH.

Tools Used

Manual review

Recommended Mitigation Steps

Track conversions and ensure that any operation involving WETH and ETH updates are reflected in totalLpETH when necessary.

Assessed type

Error

c4-judge commented 4 months ago

koolexcrypto marked the issue as primary issue

0xd4n1el commented 3 months ago

Yes, it was initially intended to get lost ETH converted to lpETH, so users would get a bonus lpETH in that case. We are implementing extra checking but not specifically for this reason

koolexcrypto commented 3 months ago

like WETH withdrawals that are converted to ETH and then potentially used or lost in transactions

WETH withdrawals are not possible. Because when you lock WETH, you are expected to withdraw ETH.

Withdrawals are also not allowed after convertAllETH is called.

            if (block.timestamp >= startClaimDate) {
                revert NoLongerPossible();
            }

Also in emergency mode, withdrawal are not allowed if it is ETH. Instead, the user has to claim it as lpETH.

if (block.timestamp >= startClaimDate){
                revert UseClaimInstead();
            }

As a result, mismatch seems not possible in this case.

c4-judge commented 3 months ago

koolexcrypto marked the issue as unsatisfactory: Insufficient proof