howlbot-integration[bot] commented 4 months ago

Lines of code

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

Vulnerability details

Impact

The protocol defines that users cannot lock tokens/ETH anymore after the loopActivation time, but this breaks the rule, and can potentially affect other state variables.

Proof of Concept

For any locking operations, they all call _processLock internally:

    function _processLock(address _token, uint256 _amount, address _receiver, bytes32 _referral)
        internal
        onlyBeforeDate(loopActivation)
    {
        if (_amount == 0) {
            revert CannotLockZero();
        }
        if (_token == ETH) {
            totalSupply = totalSupply + _amount;
            balances[_receiver][ETH] += _amount;
        } else {
            if (!isTokenAllowed[_token]) {
                revert TokenNotAllowed();
            }
            IERC20(_token).safeTransferFrom(msg.sender, address(this), _amount);

            if (_token == address(WETH)) {
                WETH.withdraw(_amount);
                totalSupply = totalSupply + _amount;
                balances[_receiver][ETH] += _amount;
            } else {
                balances[_receiver][_token] += _amount;
            }
        }

        emit Locked(_receiver, _amount, _token, _referral);
    }

As we can see, this function handles the transfer of ETH and other ERC20 tokens. Also the onlyBeforeDate modifier implies this function is only supposed to be called before loopActivation timestamp.

Another timestamp is startClaimDate, this indicates when the claiming starts. In constructor, it's set to the max value of uint32, and updated when an admin comes in and calls convertAllETH to set the value to block.timestamp. This implies lpETH are ready to be claimed and distributed to whoever locked their tokens in the protocol.

In _claim function, which is called internally by both claim and claimAndStake function converts the locked assets to lpETH and deposit to the receivers:

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

            // At this point there should not be any ETH in the contract
            // Swap token to ETH
            _fillQuote(IERC20(_token), userClaim, _data);

            // Convert swapped ETH to lpETH (1 to 1 conversion)
            claimedAmount = address(this).balance; // <=(1)
            lpETH.deposit{value: claimedAmount}(_receiver);
        }
        emit Claimed(msg.sender, _token, claimedAmount);
    }

The problem lies here. In (1), we see all ETH balance will be deposited to lpETH. Also from the comment above we know it's expected for the contract to have 0 ETH at the moment of calling such function, but in receive function, there are no restrictions on who or when ETH can be sent. This leaves a way for anyone to send ETH right before claim of lpETH, and getting more lpETH than the user has locked. This breaks the intention of locking period, and makes depositors to get more lpETH than intented. Also, in this way of getting more lpETH, totalSupply is not updated, which may also cause some problems in the protocol.

Moreover, if such action is done in claimAndStake, it may cause permanent DoS because:

    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 claimed amount will be approved and staked, but if the staked amount is increased using the unintented way, eventually lpETH of contract will run out, making other users not being able to stake, and hence DoS.

Tools Used

Manual review.

Recommended Mitigation Steps

Only allow WETH and other approved addresses to send ETH to the contract.

Assessed type

Context

c4-judge commented 4 months ago

koolexcrypto marked the issue as duplicate of #6

c4-judge commented 3 months ago

koolexcrypto marked the issue as duplicate of #33