howlbot-integration[bot] commented 4 months ago

Lines of code

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

Vulnerability details

Impact

The protocol's design allows users to withdraw ERC20 tokens before claim time, or at emergency times, but it's actually possible for users to withdraw locked ERC20 tokens even at those restricted times.

Proof of Concept

In withdraw function, we can see when such action is permitted:

    function withdraw(address _token) external {
        if (!emergencyMode) {
            if (block.timestamp <= loopActivation) {
                revert CurrentlyNotPossible();
            }
            if (block.timestamp >= startClaimDate) {
                revert NoLongerPossible();
            }
        }
    // ....

only after loopActivation, and before startClaimDate. When an admin calls convertAllETH, the claim period starts, and unless an emergency mode is declared, users cannot withdraw their locked assets anymore.

On the other hand, in _claim, the exchangers are provided to the protocol to swap ERC20 tokens to ETH, and then use the swapped amount to deposit for lpETH and stake. The exchangers supported are only UniswapV3 and TransformERC20 in the 0x protocol. During the validation of call data, not all entries are checked for transformer exchanger:

    function _decodeTransformERC20Data(bytes calldata _data)
        internal
        pure
        returns (address inputToken, address outputToken, uint256 inputTokenAmount, bytes4 selector)
    {
        assembly {
            let p := _data.offset
            selector := calldataload(p)
            inputToken := calldataload(add(p, 4)) // Read slot, selector 4 bytes
            outputToken := calldataload(add(p, 36)) // Read slot
            inputTokenAmount := calldataload(add(p, 68)) // Read slot
        }
    }

We see, three parameters are loaded from memory, but in fact, there are more parameters to the function:

    function transformERC20(
        IERC20Token inputToken,
        IERC20Token outputToken,
        uint256 inputTokenAmount,
        uint256 minOutputTokenAmount,
        Transformation[] memory transformations
    ) public payable override returns (uint256 outputTokenAmount) {

The transformations field provides some additional transforming info to the function, and the struct looks like this:

    struct Transformation {
        // The deployment nonce for the transformer.
        // The address of the transformer contract will be derived from this
        // value.
        uint32 deploymentNonce;
        // Arbitrary data to pass to the transformer.
        bytes data;
    }

In the transformer, this data part of the struct will be passed into different transformers user have specified:

        {
            // Perform transformations.
            for (uint256 i = 0; i < args.transformations.length; ++i) {
                _executeTransformation(
                    state.wallet,
                    args.transformations[i],
                    state.transformerDeployer,
                    args.recipient
                );
            }
            // Transfer output tokens from wallet to recipient
            outputTokenAmount = _executeOutputTokenTransfer(args.outputToken, state.wallet, args.recipient);
        }

and:

        bytes memory resultData = wallet.executeDelegateCall(
            // The call target.
            transformer,
            // Call data.
            abi.encodeWithSelector(
                IERC20Transformer.transform.selector,
                IERC20Transformer.TransformContext({
                    sender: msg.sender,
                    recipient: recipient,
                    data: transformation.data
                })
            )
        );

For example, in one of the transformers:

    function transform(TransformContext calldata context) external override returns (bytes4 success) {
        TokenFee[] memory fees = abi.decode(context.data, (TokenFee[]));

        // Transfer tokens to recipients.
        for (uint256 i = 0; i < fees.length; ++i) {
            uint256 amount = fees[i].amount;
            if (amount == MAX_UINT256) {
                amount = LibERC20Transformer.getTokenBalanceOf(fees[i].token, address(this));
            }
            if (amount != 0) {
                fees[i].token.unsafeTransformerTransfer(fees[i].recipient, amount);
            }
        }

        return LibERC20Transformer.TRANSFORMER_SUCCESS;
    }

Tokens can be transferred out to recipients specified by users, but not controlled by the prelaunch contract. After those calls, the rest of the balance will be considered as output amount, and proceeds to transfer them to the prelaunch contract. Since there is no ETH in the 0x contract, so the output value will also be 0, the transaction will succeed and transferred ERC20 tokens will go back to the users' hand.

In general, this doesn't hurt the protocol's funds and anything, but simply allows users to withdraw ERC20 tokens during disallowed periods, violating the protocol design.

Tools Used

Manual review

Recommended Mitigation Steps

If the only transformer is meant to be used is FillQuoteTransformer, then consider adding a check for such transformer's info.

Assessed type

Context

0xd4n1el commented 4 months ago

Validating any possible path is unfeasible for gas limits. By using Claimed event we track if user indeed withdraw and adjust points accordingly. In this case the contract does not need further modifications

c4-judge commented 3 months ago

koolexcrypto changed the severity to QA (Quality Assurance)