code-423n4/2024-05-loop-findings

Potential Over-Issuance of lpETH Due to Unaccounted ETH Deposits Before Claim and Stake Operations

howlbot-integration opened this issue · 14 comments

howlbot-integration commented

Lines of code

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

Vulnerability details

Impact

This could lead to the over-issuance of lpETH tokens relative to the actual amount of ETH backing them. If additional ETH is deposited into the contract (not through the standard lockETH or convertAllETH functions) and is not accounted for in totalLpETH, then when claimAndStake is executed, this extra ETH could be wrongly converted into lpETH. This misalignment could inflate the lpETH supply without proper backing which could devalue the lpETH.

Proof of Concept

The issue in the claimAndStake function, which allows users to claim their vested lpETH and immediately stake it. The function internally calls _claim, which handles the conversion of tokens to lpETH.
https://github.com/code-423n4/2024-05-loop/blob/0dc8467ccff27230e7c0530b619524cc8401e22a/src/PrelaunchPoints.sol#L226-L235

function claimAndStake(address _token, uint8 _percentage, Exchange _exchange, bytes calldata _data)
    external
    onlyAfterDate(startClaimDate)
{
    uint256 claimedAmount = _claim(_token, address(this), _percentage, _exchange, _data);
    lpETH.approve(address(lpETHVault), claimedAmount);
    lpETHVault.stake(claimedAmount, msg.sender);

    emit StakedVault(msg.sender, claimedAmount);
}

The _claim function includes logic for converting non-ETH tokens to ETH and then to lpETH:
https://github.com/code-423n4/2024-05-loop/blob/0dc8467ccff27230e7c0530b619524cc8401e22a/src/PrelaunchPoints.sol#L252-L266

if (_token != ETH) {
    uint256 userClaim = userStake * _percentage / 100;
    _validateData(_token, userClaim, _exchange, _data);
    balances[msg.sender][_token] = userStake - userClaim;

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

If additional ETH is present in the contract that was not converted through convertAllETH, it will be included in the address(this).balance calculation, leading to an incorrect amount of lpETH being minted based on the user's original stake.

Tools Used

Manual review

Recommended Mitigation Steps

Track any ETH deposited into the contract outside of the standard functions (lockETH, convertAllETH). Probably by modifying the fallback function to update a specific balance or state variable.
Also, before converting ETH to lpETH in the _claim function, ensure that only the ETH amount corresponding to the user's stake (after token-to-ETH conversion) is used for lpETH conversion.

Assessed type

Error

c4-judge commented

koolexcrypto marked the issue as duplicate of #18

c4-judge commented

koolexcrypto changed the severity to 3 (High Risk)

c4-judge commented

koolexcrypto changed the severity to 2 (Med Risk)

c4-judge commented

koolexcrypto changed the severity to 3 (High Risk)

c4-judge commented

koolexcrypto marked the issue as duplicate of #33

c4-judge commented

koolexcrypto marked the issue as satisfactory

c4-judge commented

koolexcrypto marked the issue as partial-25

Rhaydden commented

Hello @koolexcrypto, thank you for the efforts.

Respectfully. I think marking this as a partial 25 is a mistake based on your comment here because this report talks about the direct transfer of funds and the bypassing of lock. Please have a second look at it again

koolexcrypto commented

Hello @koolexcrypto, thank you for the efforts.

Respectfully. I think marking this as a partial 25 is a mistake based on your comment here because this report talks about the direct transfer of funds and the bypassing of lock. Please have a second look at it again

Hi @Rhaydden

Where do you mention the direct transfer?

koolexcrypto commented

My bad

If additional ETH is deposited into the contract (not through the standard lockETH or convertAllETH functions

I missed this.

  • 👍1
c4-judge commented

koolexcrypto marked the issue as partial-50

Rhaydden commented

Thank you @koolexcrypto for your understanding. I believe a partial 75 is deserved just as in #57

koolexcrypto commented

There is no mentioning of bypassing locking duration.

This misalignment could inflate the lpETH supply without proper backing which could devalue the lpETH

This impact also is inaccurate. lpETH is not devalued due to this. ETH are actually converted and deposited into the vault.

koolexcrypto commented

#57 mentions this

more tokens via claimAndStake() than he had initially locked

It should take full credit, but other factors (quality of report) was taken into account.

  • 👍1