howlbot-integration[bot] commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-05-loop/blob/40167e469edde09969643b6808c57e25d1b9c203/src/PrelaunchPoints.sol#L497

Vulnerability details

Impact

Users are unable to utilize the UniswapV3Feature for claiming their LRTs due to incorrect encoding of function parameters.

Proof of Concept

Issue Description

In order to claim their locked LRTs for a relative amount of lpETH, users should invoke either the claim() or claimAndStake() function. These functions require users to provide arguments including the token they want to claim, the percentage of their locked token amount, the exchange for the swap, and bytes data used in _fillQuote() to perform a call to the 0x API for swapping tokens. However, due to improper encoding of the bytes data, it becomes impossible to call sellTokenForEthToUniswapV3() from the UniswapV3Feature, leading to a failure in the claim process.

Root Cause

The issue arises because the bytes data provided for the 0x API call includes unnecessary data, such as the address of the output token. When attempting to call the 0x API with this data, it reverts due to the unexpected parameter, causing the entire claim process to fail.

PoC

User attempts to swap their locked LRT for a relative amount of lpETH by calling claim() with the exchange set to UniswapV3Feature:

 function claim(address _token, uint8 _percentage, Exchange _exchange, bytes calldata _data)
        external
        onlyAfterDate(startClaimDate)
    {
        _claim(_token, msg.sender, _percentage, _exchange, _data);
    }

Within the _claim() function, the internal _validateData() function is called to ensure that the provided bytes data contains correct information for calling the exchange:

function _validateData(address _token, uint256 _amount, Exchange _exchange, bytes calldata _data) internal view {
    ...
    (inputToken, outputToken, inputTokenAmount, recipient, selector) = _decodeUniswapV3Data(_data);
    ...
}

However, as we can see in the _decodeUniswapV3Data() function there is unnecessary data for the 0x API, which is outputToken address. This address is needed for validating data withing PrelaunchPoints.sol, but not for the 0x API. Exactly this unnecessary data will make the call to 0x API to revert.

    function _decodeUniswapV3Data(bytes calldata _data)
        internal
        pure
        returns (address inputToken, address outputToken, uint256 inputTokenAmount, address recipient, bytes4 selector)
    {
        uint256 encodedPathLength;
        assembly {
            let p := _data.offset
            selector := calldataload(p)
            p := add(p, 36) // Data: selector 4 + lenght data 32
            inputTokenAmount := calldataload(p)
            recipient := calldataload(add(p, 64))
            encodedPathLength := calldataload(add(p, 96)) // Get length of encodedPath (obtained through abi.encodePacked)
            inputToken := shr(96, calldataload(add(p, 128))) // Shift to the Right with 24 zeroes (12 bytes = 96 bits) to get address
            outputToken := shr(96, calldataload(add(p, add(encodedPathLength, 108)))) // Get last address of the hop
        }
    }

Calling the 0x API with the bytes data provided in the claim() function without any internal modifications.

    function _fillQuote(IERC20 _sellToken, uint256 _amount, bytes calldata _swapCallData) internal {
        ...
        (bool success,) = payable(exchangeProxy).call{value: 0}(_swapCallData);
         ...
    }

This will result in an attempted call to the API with the outputToken parameter, causing the call to revert because sellTokenForEthToUniswapV3() does not accept this parameter.

0x UniswapV3Feature#sellTokenForEthToUniswapV3()

    function sellTokenForEthToUniswapV3(
        bytes memory encodedPath,
        uint256 sellAmount,
        uint256 minBuyAmount,
        address payable recipient
    ) public override returns (uint256 buyAmount) {
       ...
    }

Simple PoC test

This simple contract demonstrates that calling a function with more parameters than needed will result in a revert.

PoC contract

```solidity contract I { function encodeDataIncorrectParams() external pure returns (bytes memory) { return abi.encodeWithSignature("div(uint256,address,uint256)", 10, address(1), 10); } function encodeData() external pure returns (bytes memory) { return abi.encodeWithSignature("div(uint256,uint256)", 10, 5); } function callDiv(bytes calldata data) external returns (uint) { (bool success, bytes memory returnedData) = address(this).call(data); require(success, "Call to div failed"); uint256 number; for (uint256 i = 0; i < returnedData.length; i++) { number += uint256(uint8(returnedData[i])) * (2**(8 * (returnedData.length - (i + 1)))); } return number; } function div(uint256 a, uint256 b) public pure returns (uint256) { return a / b; } } ```

Steps to use the contract:

Copy and paste it in Remix
Invoke encodeDataIncorrectParams() and with the received output call callDiv(). As you can see it reverts.
Invoke encodeData() and with the received output call callDiv(). As it can be seen, the call to div() was succesful and, from the console logs, we can see that the division was successful as well.

Remix console output

decoded input   {
    "bytes data": "0xa391c15b000000000000000000000000000000000000000000000000000000000000000a0000000000000000000000000000000000000000000000000000000000000005"
}
decoded output  {
    "0": "uint256: 2"
}

Tools Used

Manual code review. Remix IDE

Recommended Mitigation Steps

One approach to mitigate this issue is to manipulate the initial bytes data from the claim() and claimAndStake() functions internally before passing it to the 0x API. By doing so, unnecessary data such as the output token address can be removed, ensuring that the call to the 0x API contains only the required parameters.

Assessed type

call/delegatecall

c4-judge commented 4 months ago

koolexcrypto marked the issue as primary issue

0xd4n1el commented 3 months ago

As shown in the tests, the calls do not revert. The outputToken is encoded in the encodedPath of Univ3

c4-judge commented 3 months ago

koolexcrypto changed the severity to QA (Quality Assurance)