howlbot-integration[bot] commented 4 months ago

Lines of code

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

Vulnerability details

Impact

I am reporting this submission as Medium because I know that this issue is not High but I am also pretty much confident that this is not even Low because it might affect the decision of the liquidity providers and negatively affect the ETH liquidity bootstrapping activity.

In the contest's Readme.md, it is written as There is an emergency mode that allows users to withdraw without any time restriction. If ETH was converted already users can call claim instead.".

Being an investor (ETH/WETH depositor/locker), this statement impresses me that in case of any unfortunate event like Hack/Attack, I will be able to recover my investment if emergency mode is applied by the protocol admin. Even if the conversion is done, I will be able to claim instead.

But the point here is that, investor will only be able to claim the lpETH. We should explicitly mention this in document that after conversion, even if emergency mode is activated, ETH/WETH depositors/lockers will only be able to claim lpETH not ETH/WETH. Because if the protocol is attacked then definitely lpETH will also crash and in such case claiming lpETH will make no benefit to the ETH/WETH depositor/locker.

Another point to note here is that LRT depositor/locker has an edge in this case because even after conversion and activation of emergency mode, they can withdraw their principally staked LRTs at any point in time until and unless those are not claimed. However, this is mentioned in the docs, that this behaviour is intended to prevent locking of LRTs in contract due to malfunction by 0x API. But if depositors know this fact then they might prefer to deposit LRT instead of ETH/WETH directly.

Source

Code

function withdraw(address _token) external {
    ...
    if (_token == ETH) {
        if (block.timestamp >= startClaimDate){
@>      revert UseClaimInstead();
        }
    ...
}

Actual Impact

Liquidity providers might get mislead by reading the docs and not understand the actual implication. If this happens, then in future it might affect the credibility of the protocol and might affect the market confidence of lpETH token.

If understood correctly, then liquidity providers might prefer to deposit/lock LRTs instead of ETH/WETH directly because it can be perceived that depositing LRT is more secure than ETH/WETH and this will in turn affect the Liquidity bootstrapping activity for which this Prelaunch program is all about.

Proof of Concept

forge test --mt test_WY_EmergencyModeWithdrawBehaviour -vv

Add below test in PrelaunchPoints.t.sol

Code

function test_WY_EmergencyModeWithdrawBehaviour() public {

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

    lrt.mint(user2, 200 ether);

    // Amount of LRTs to lock
    uint256 lrtAmount = 50 ether;
    vm.prank(user2);
    lrt.approve(address(prelaunchPoints), lrtAmount);

    // Amount of eth to lock
    uint256 ethAmount = 10 ether;
    vm.deal(user1, ethAmount);

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

    // User2 locks LRT
    vm.prank(user2);
    prelaunchPoints.lock(address(lrt), lrtAmount, referral);

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

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

    // All ETH converted
    prelaunchPoints.convertAllETH();

    // No one claimed anything

    // Suddenly emergency mode activated
    prelaunchPoints.setEmergencyMode(true);

    // LRT lockers will be able to recover their LRTs
    vm.prank(user2);
    prelaunchPoints.withdraw(address(lrt));

    // ETH/WETH lockers will not be able to do that even though did not claim yet
    vm.prank(user1);
    vm.expectRevert(PrelaunchPoints.UseClaimInstead.selector);
    prelaunchPoints.withdraw(ETH);

}

Ran 1 test for test/PrelaunchPoints.t.sol:PrelaunchPointsTest
[PASS] test_WY_EmergencyModeWithdrawBehaviour() (gas: 268356)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.82ms (1.07ms CPU time)

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

Tools Used

Manual review

Recommended Mitigation Steps

The treatment of withdrawal in case of emergency mode should be equitable with LRT/ETH/WETH lockers. It would be more fair, if we allow the ETH/WETH depositor/locker to withdraw the unclaimed amount of principally staked ETH/WETH in case of emergency.

Assessed type

Other

c4-judge commented 4 months ago

koolexcrypto marked the issue as primary issue

0xd4n1el commented 4 months ago

The emergency is clearly documented to be a 0x API failure and not an lpETH contract problem. lpETH will be just a wrapper where users can deposit/withdraw at any time, so users can still recover their ETH/WETH but with an extra step

c4-judge commented 3 months ago

koolexcrypto changed the severity to QA (Quality Assurance)