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

4 stars 4 forks source link

Malicious user can lock all lpETH using wrapped LRT from being claimed via a direct donation #29

Closed howlbot-integration[bot] closed 3 months ago

howlbot-integration[bot] commented 4 months ago

Lines of code

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

Vulnerability details

Impact

In PreLaunchPoints.sol, users can claim their lpETH after the startClaimDate when all ETH balance within the contract is converted to lpETH via convertAllETH(). Users claim lpETH depending on whether ETH/WETH or wrapped LRTs are locked:

  1. ETH/WETH - 1:1 exchange of amount locked
  2. Wrapped LRTs - A swap is initiated via a uniswapV3 pool and/or other exchanges

However, due to a flaw in _fillQuote() revolving how boughtETHAmount is computed, it can potentially cause a permanent locking in funds. The impact of this is borderline medium/high, given ALL potential users funds are locked but would require the attacker to potentially donate an equal/greater value worth of ETH. Since it is mentioned explicitly as an attack vector to focus on, I will leave as high severity

  • User funds getting locked forever

Proof of Concept

function _fillQuote(IERC20 _sellToken, uint256 _amount, bytes calldata _swapCallData) internal {
    // Track our balance of the buyToken to determine how much we've bought.
    uint256 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);
}
  1. Assume that 50e18 amount of wrapped LRTs are locked within the PrelaunchPoints.sol contract. When users want to claim the tokens, _fillQuote() is executed to initiate the swap, notice how only userClaim worth of tokens is approved for swap.


  1. Depending on slippage in play and exchange rates, lets assume the maximum amount of ETH that can be exchanged from wrapped LRT would be 51e18 (some tokens could be priced more than ETH).


  1. Notice how boughtETHAmount is computed. At the point of time _fillQuote() is executed, ETH balance would be assumed to be zero. So it is presumed that boughtETHAmount would initially be zero, and thus would never underflow when subsequently recomputed after the swap is completed


  1. However, a malicious user/whale can simply back-run convertAllETH(), donate a necessary amount of ETH (For example in this case, 52e18) to essentially lock all users from claiming lpETH from previous positions locked using wrapped LRTs since now the computation of boughtETHAmount would underflow (51e18 - 52e18).


  1. The only way this issue could be elevated would be that the price of wrapped LRTs exceed ETH prices such that the total ETH balance retrieved would be greater than the current artificially inflated boughtETHAmount (which is highliy unlikely). In that case, the attacker can now simply donate a higher amount of funds to continue the attack.

Note that the impact described above is the maximum possible impact, but this issue can also be performed by front-running specific claims with smaller amount of funds to cause underflow, but would possibly not block all claims.

Tools Used

Manual Analysis

Recommended Mitigation Steps

Remove the computation of boughtETHAmount and track it off-chain, since it is not used within inscope contract.If not, consider allowing admin to retrieve any additional ETH donated that does not stem from honest locking positions represented by totalSupply.

Assessed type

DoS

c4-judge commented 4 months ago

koolexcrypto marked the issue as primary issue

0xd4n1el commented 3 months ago

By sending ETH to the contract, the attacker only allows a subsequent user to claim them as lpETH, not locking the contract neither having any negative impact for users.

c4-judge commented 3 months ago

koolexcrypto marked the issue as unsatisfactory: Invalid