Availability of deposit invariant can be bypassed

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

One of the main invariants stated in the contest is the following:

Deposits are active up to the lpETH contract and lpETHVault contract are set

However, there are currently two ways this invariant can be broken, allowing users to gain lpETH without explicitly locking tokens before contracts are set.

1. Sandwich a call to convertAllETH by front-running to directly donate ETH and then back-running to claim lpETH via multiple lock positions

2. Bundle a transaction of donation and claiming with a previously locked position of wrapped LRT

This bypasses the onlyBeforeDate(loopActivation) modifier included in all lock functions. It also potentially allows no cap in lpETH minted and also discourages users from locking ETH in the PrelaunchPoints.sol contract before contract addresses are set (i.e. loopActivation is assigned)

Proof of Concept

Scenario 1

Bundle a transaction of donation and claiming with a previously locked position of wrapped LRT. User can execute the following in one transaction, assuming the user previously has a locked position with any of the allowed LRTs.

1. Donate ETH directly to contract
2. Call claim()/claimAndStake(), claiming all tokens, this swaps users wrapped LRT to ETH via _fillQuote.
3. Since claimedAmount is set as address(this).balance here, claimedAmountis computed as the addition of ETH receive from LRT swapped + user donated ETH
4. claimedAmount worth of lpETH is subsequently claimed/staked via lpETH.deposit()/lpETHVault.stake().

Scenario 2

Sandwich a call to convertAllETH by front-running to directly donate ETH and then back-running to claim lpETH via multiple lock positions

1. Assume user has 10 positions of ETH/WETH locked each with 1 ether with different addresses, with 100 ETH total locked
2. Front-run call to convertAllETH by donating 1 ETH. This sets totalLpETH to be 101 ETH as seen here
3. Ratio of totalLpETH : totalSupply computed here is now 1.01 (1.01)
4. Back-run convertAllETH by executing claims for all 10 position, each claim will mint 0.01 ETH extra (1.01 lpETH)

This scenario is less likely than scenario 1 as it requires user to perfectly back-run convertAllETH() with claims, although it also breaks another invariant of a 1:1 swap

Users that deposit ETH/WETH get the correct amount of lpETH on claim (1 to 1 conversion)

Tools Used

Manual Analysis

Recommended Mitigation Steps

1. For scenario 1, set claimedAmount to amount of ETH bought from LRT swap (such as buyAmount for uniV3)
2. For scenario 2, no fix is likely required since it would require users to risk their funds to be transferred by other users due to inflated totalLpETH : totalSupply ratio. If not you can consider setting totalLpETH to totalSupply and allow admin to retrieve any additional ETH donated.

Assessed type

Other

[c4-judge]

koolexcrypto marked the issue as primary issue

[c4-judge]

koolexcrypto changed the severity to 3 (High Risk)

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto marked the issue as selected for report

[k4zanmalay]

I believe this issue has no impact. It incorrectly assumes that only stakers can mint lpETH, however the sponsor has repeatedly mentioned that the locking feature is only needed to collect points that depend on the duration of the lock, and users can freely mint lpETH without locking their tokens.

[d3e4]

@koolexcrypto This report contains two issues:
Scenario 1: Direct transfer of ETH before _claim() to bypass locking requirement.
Scenario 2: Donate ETH to every holder equally.

Firstly, I would like to argue that only Scenario 1 is a High severity issue, whereas Scenario 2 is at best QA. There is no incentive or advantage for the attacker to do this, and allowing someone to distribute funds to everyone equally is a normal feature of a tokenized vault and should not be considered an issue.

Secondly, many of the duplicates fail to mention the exploitative aspect of Scenario 1. Some speak of an accidental transfer to the contract, and others about there randomly being some ETH lying around in the contract. User mistakes are not grounds for a HM issue, and the argument about ETH already present in the contract is either simply false or demonstrates an insufficient or incorrect understanding of the implications of whatever it might have in common with the root cause of Scenario 1. I believe only those reports mentioning a deliberate donation of ETH in order to bypass the locking requirement before calling claim() or claimAndStake() should be considered duplicates of #33. All others should be invalid or QA.

Of the current duplicates of #33 these are the only ones mentioning (however vaguely) a deliberate donation:
#58, #57, #56, #55, #53, #51, #48, #46, #40, #38, #37, #34, #33, #32, #30, #27, #26, #25, #22, #20, #19, #6.
I believe these should be full duplicates of #33.

Of the remaining current duplicates, #82, #70, #69, #67, #64, #54, #44, #35, #28, #24 only mention a user mistake or a (false, unjustified or unexplained) prior presence of ETH in the contract, and #43, #41, #39, #23, #21, #18 only mention Scenario 2, and #36 is based on the weird claim that "due to market conditions, the actual amount of ETH received from the token swap may differ".
I believe these should be invalid or QA.

[koolexcrypto]

I believe this issue has no impact. It incorrectly assumes that only stakers can mint lpETH, however the sponsor has repeatedly mentioned that the locking feature is only needed to collect points that depend on the duration of the lock, and users can freely mint lpETH without locking their tokens.

Bypassing the locking duration is a clear impact. The sponsor confirmed the issue.

[koolexcrypto]

@koolexcrypto This report contains two issues: Scenario 1: Direct transfer of ETH before _claim() to bypass locking requirement. Scenario 2: Donate ETH to every holder equally.

Firstly, I would like to argue that only Scenario 1 is a High severity issue, whereas Scenario 2 is at best QA. There is no incentive or advantage for the attacker to do this, and allowing someone to distribute funds to everyone equally is a normal feature of a tokenized vault and should not be considered an issue.

Secondly, many of the duplicates fail to mention the exploitative aspect of Scenario 1. Some speak of an accidental transfer to the contract, and others about there randomly being some ETH lying around in the contract. User mistakes are not grounds for a HM issue, and the argument about ETH already present in the contract is either simply false or demonstrates an insufficient or incorrect understanding of the implications of whatever it might have in common with the root cause of Scenario 1. I believe only those reports mentioning a deliberate donation of ETH in order to bypass the locking requirement before calling claim() or claimAndStake() should be considered duplicates of #33. All others should be invalid or QA.

Of the current duplicates of #33 these are the only ones mentioning (however vaguely) a deliberate donation: #58, #57, #56, #55, #53, #51, #48, #46, #40, #38, #37, #34, #33, #32, #30, #27, #26, #25, #22, #20, #19, #6. I believe these should be full duplicates of #33.

Of the remaining current duplicates, #82, #70, #69, #67, #64, #54, #44, #35, #28, #24 only mention a user mistake or a (false, unjustified or unexplained) prior presence of ETH in the contract, and #43, #41, #39, #23, #21, #18 only mention Scenario 2, and #36 is based on the weird claim that "due to market conditions, the actual amount of ETH received from the token swap may differ". I believe these should be invalid or QA.

Thank you for your feedback.

Generally speaking, I agree with you. I am considering lowering the partial credits given. I won't make it as QA since the exploit scenario is mentioned, those for issues that mention a deliberate donation without the impact of bypassing locking duration. However, for issues that mention it as a user mistake, I think a QA would be fairer.

[k4zanmalay]

Bypassing the locking duration is a clear impact. The sponsor confirmed the issue.

@koolexcrypto hi, what do you mean by this? Users can't claim lpETH until the startClaimDate

    function claim(address _token, uint8 _percentage, Exchange _exchange, bytes calldata _data)
        external
        onlyAfterDate(startClaimDate)

As for the lpETH minting, sponsor commented that it is available for all, no need to lock tokens and implement attack scenarios described in this report, user can call lpETH.deposit directly whenever he wishes.

[koolexcrypto]

@k4zanmalay

The core concept is to lock funds for some time to grant the users points based on that