Users don't claim the correct amount of `lpETH` token, violating the invariant

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Issue Description

There is an invariant that says : Users that deposit ETH/WETH get the correct amount of lpETH on claim (1 to 1 conversion) The protocol's invariant, which guarantees a 1:1 conversion ratio of deposited ETH to lpETH upon claim, is being violated due to the presence of the receive() function and the convertAllETH function.

    function convertAllETH() external onlyAuthorized onlyBeforeDate(startClaimDate) {
        // ...

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

        totalLpETH = lpETH.balanceOf(address(this));

        // ...
    }

The convertAllETH function improperly calculates totalLpETH by assigning the contract's ETH balance directly to this variable, rather than ensuring accurate handling of ETH and lpETH balances. This miscalculation allows totalLpETH to be increased through the receipt of ETH without corresponding lpETH issuance, leading to a disparity between totalLpETH and totalSupply. This all happens because of the simple math calculation in the _claim function:

function _claim(address _token, address _receiver, uint8 _percentage, Exchange _exchange, bytes calldata _data)
        internal
        returns (uint256 claimedAmount)
    {
        //...

        if (_token == ETH) {
              // Users gets more lpETH because of `totalLpETH > totalSupply`
@>            claimedAmount = userStake.mulDiv(totalLpETH, totalSupply);
            balances[msg.sender][_token] = 0;
            lpETH.safeTransfer(_receiver, claimedAmount);
        }

        // ...
}

Although users get more than they expected it broke one of the main invariants which everything comes from a lack of ETH handling There is another important note. the locking process also emits the Locked event for handling the points but due to this bug, it can't handle the points very well.

Impact

Users are receiving more lpETH than expected due to incorrect handling of ETH within the protocol. This violation undermines the fundamental 1:1 conversion ratio between ETH deposits and lpETH issuance, compromising protocol integrity.

Proof of Concept

add this test to the PrelaunchPointsTest.t.sol:

    function test_MainInvariantBreaksWithETHDeposit() public {
        address alice = makeAddr("alice");

        // `address(this)` could represent some users
        vm.deal(address(this), 90e18);
        prelaunchPoints.lockETH{value: 90e18}("");

        // let the alice lock her ETH
        vm.deal(alice, 10e18);
        vm.prank(alice);
        prelaunchPoints.lockETH{value: 10e18}("");
        uint256 aliceETHBalanceBefore = prelaunchPoints.balances(alice, ETH);
        assertTrue(prelaunchPoints.totalSupply() == 100e18);
        assertTrue(prelaunchPoints.totalSupply() == address(prelaunchPoints).balance);
        assertTrue(aliceETHBalanceBefore == 10e18);

        // some users (or alice) send ETH to PrelaunchPoints contract
        // Also alice gets lpETH out of Lock process
        vm.deal(alice, 20e18);
        vm.prank(alice);
        (bool sent,) = address(prelaunchPoints).call{value: 20e18}("");
        assertTrue(sent);
        assertTrue(prelaunchPoints.totalSupply() != 120e18);
        assertTrue(prelaunchPoints.totalSupply() != address(prelaunchPoints).balance);

        // let the owner convert all ETH
        prelaunchPoints.setLoopAddresses(address(lpETH), address(lpETHVault));
        assertTrue(prelaunchPoints.loopActivation() == block.timestamp);
        vm.warp(prelaunchPoints.loopActivation() + 8 days);
        prelaunchPoints.convertAllETH();
        assertTrue(prelaunchPoints.startClaimDate() == block.timestamp);
        assertTrue(prelaunchPoints.totalLpETH() == 120e18);
        assertTrue(prelaunchPoints.totalLpETH() != prelaunchPoints.totalSupply());

        // alice now claims her vested lpETH. it should be 1 to 1 conversion
        // but alice gets more
        vm.warp(prelaunchPoints.startClaimDate() + 1 days);
        vm.prank(alice);
        prelaunchPoints.claim(ETH, 0, PrelaunchPoints.Exchange.UniswapV3, emptydata);

        // (userStake * totalLpETH) / totalSupply =
        // (10e18 * 120e18) / 100e18 = 12e18 != 10e18
        // Main Invariant Broked
        assertTrue(lpETH.balanceOf(alice) != aliceETHBalanceBefore);
    }

Tools Used

Manual Review, foundry

Recommended Mitigation Steps

There are 2 ways to mitigate this:

1. Implement the LockETH function in the receive() function:

   receive() external payable {
   +       lockETH("");
   }

2. Assign the totalSupply to the totalBalance:

   
   function convertAllETH() external onlyAuthorized onlyBeforeDate(startClaimDate) {
       // ...

• uint256 totalBalance = address(this).balance;

• uint256 totalBalance = totalSupply;

  // ...

  }

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 changed the severity to 3 (High Risk)

[c4-judge]

koolexcrypto marked the issue as duplicate of #33

[c4-judge]

koolexcrypto marked the issue as satisfactory