search

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

4 stars 4 forks source link

Out-of-Bounds Access in `_decodeUniswapV3Data` #100

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/0dc8467ccff27230e7c0530b619524cc8401e22a/src/PrelaunchPoints.sol#L454-L463

Vulnerability details

Description

The _decodeUniswapV3Data function within the PrelaunchPoints contract is responsible for parsing data received from the 0x API when Uniswap V3 is used for token swaps. However, there is a issue in the way the function calculates the offset to access the outputToken address from the encoded data.

The vulnerability arises from the following lines of code:

encodedPathLength := calldataload(add(p, 96))
...
outputToken := shr(96, calldataload(add(p, add(encodedPathLength, 108))))

This issue occurs due to the assumption that encodedPathLength aligns with 32-byte memory boundaries, which is a standard in EVM memory operations. When encodedPathLength results in an offset that is not aligned on a 32-byte boundary, the subsequent memory access to retrieve the outputToken will be incorrect.

This misalignment can happen when the encodedPathLength is not a multiple of 32 bytes. For example, if encodedPathLength is 50 bytes, the resulting offset for accessing the outputToken would be p + 158 bytes, which is not aligned with the 32-byte boundary.

Impact

The out-of-bounds access issue in the _decodeUniswapV3Data function can have several consequences:

Proof of Concept

Here is the affected code:

        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
        }

Consider the following scenario:

  1. The _data parameter passed to the _decodeUniswapV3Data function contains an encodedPathLength of 50 bytes.
  2. The function calculates the position of the outputToken by adding the encodedPathLength to the base pointer p and an offset of 108 bytes: add(p, add(encodedPathLength, 108)).
  3. In this case, the resulting offset for accessing the outputToken would be p + 158 bytes (108 + 50).
  4. However, since Solidity expects memory offsets to align on 32-byte boundaries, accessing data at the calculated offset of p + 158 bytes may lead to reading incorrect data or accessing memory beyond the bounds of the input data.

This scenario illustrates how an improperly aligned encodedPathLength can result in incorrect memory access and potential out-of-bounds access, leading to erroneous behavior or security vulnerabilities in the contract.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this vulnerability, I can recommend to modify the offset calculation to ensure proper memory alignment. The encodedPathLength should be rounded up to the nearest multiple of 32 bytes before using it to calculate the memory offset for accessing the outputToken.

Here's the recommended code change:

// Calculate the aligned path length by rounding up to the nearest multiple of 32
let alignedPathLength := and(add(encodedPathLength, 31), not(31))

// Use the aligned path length to calculate the offset for accessing outputToken
outputToken := shr(96, calldataload(add(p, add(alignedPathLength, 108))))

The modified code introduces a new variable alignedPathLength, which is calculated by rounding up encodedPathLength to the nearest multiple of 32 using bitwise operations. This ensures that the resulting offset for accessing the outputToken is always aligned with the 32-byte boundary.

Here's a diff representation of the changes:

- outputToken := shr(96, calldataload(add(p, add(encodedPathLength, 108))))
+ let alignedPathLength := and(add(encodedPathLength, 31), not(31))
+ outputToken := shr(96, calldataload(add(p, add(alignedPathLength, 108))))

Assessed type

Context

0xd4n1el commented 4 months ago

Uniswap V3 data uses abi.encodePacked which does not align with the 32 bytes memory packing of solidity. That is why the offset needs to be that specific value. The PoC does not show if the proposed solution gives the right decoding

koolexcrypto commented 3 months ago

Looks like a valid issue. However, I'm asking for a PoC (coded) to show that accessing out of bound memory will cause issues in this case. QA for now unless there is a demonstrated impact.

c4-judge commented 3 months ago

koolexcrypto marked the issue as primary issue

c4-judge commented 3 months ago

koolexcrypto changed the severity to QA (Quality Assurance)

c4-judge commented 3 months ago

koolexcrypto marked the issue as grade-c

mePopye commented 3 months ago

@koolexcrypto thank you for asking the PoC

Here is the coded PoC: Add this test file(PrelaunchPointsPoCTest.t.sol) to ./test folder. 

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.20;

import "forge-std/Test.sol";
import "../src/PrelaunchPoints.sol";
import "../src/interfaces/ILpETH.sol";
import "../src/interfaces/IWETH.sol";

import "../src/mock/AttackContract.sol";
import "../src/mock/MockLpETH.sol";
import "../src/mock/MockLpETHVault.sol";
import {ERC20Token} from "../src/mock/MockERC20.sol";
import {LRToken} from "../src/mock/MockLRT.sol";

import "forge-std/console.sol";

contract PrelaunchPointsPoCTest is Test {
    PrelaunchPoints public prelaunchPoints;
    ILpETH public lpETH;
    ILpETHVault public lpETHVault;
    IWETH public weth;
    ERC20Token public mockToken;
    uint256 public constant INITIAL_SUPPLY = 1000 ether;
    bytes32 referral = bytes32(uint256(1));
    address constant EXCHANGE_PROXY = 0xDef1C0ded9bec7F1a1670819833240f027b25EfF;
    address public constant ETH = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;
    address public constant WETH = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;
    address[] public allowedTokens;
    bytes public invalidData;

    function setUp() public {
        weth = IWETH(WETH);
        lpETH = new MockLpETH();
        lpETHVault = new MockLpETHVault();
        mockToken = new ERC20Token();

        address[] memory allowedTokens_ = new address[](1);
        allowedTokens_[0] = WETH;
        prelaunchPoints = new PrelaunchPoints(EXCHANGE_PROXY, WETH, allowedTokens_);
        vm.deal(address(this), 10 ether); // fund the test contract with 10 ether

        // Construct invalid data where encodedPathLength is not 32-byte aligned
        invalidData = abi.encodePacked(
            bytes4(0x803ba26d), // UniswapV3 selector
            uint256(1 ether), // inputTokenAmount
            address(this), // recipient
            uint256(50), // encodedPathLength (not aligned with 32-byte boundary)
            WETH, // inputToken
            address(0x1234567890AbcdEF1234567890aBcdef12345678) // arbitrary address to simulate misalignment
        );
    }

    function testDecodeUniswapV3DataOutOfBounds() public {
        // Lock some WETH
        weth.deposit{value: 1 ether}();
        IERC20(address(weth)).approve(address(prelaunchPoints), 1 ether);
        prelaunchPoints.lock(address(weth), 1 ether, referral);

        // Activate claiming period
        prelaunchPoints.setLoopAddresses(address(lpETH), address(lpETHVault));
        vm.warp(block.timestamp + 120 days + 1);
        prelaunchPoints.convertAllETH();

        // Attempt to claim with invalid data
        prelaunchPoints.claim(address(weth), 100, PrelaunchPoints.Exchange.UniswapV3, invalidData);
    }
}

Then run this command

forge test --match-path ./test/PrelaunchPointsPoCTest.t.sol

Output log:

Ran 1 test for test/PrelaunchPointsPoCTest.t.sol:PrelaunchPointsPoCTest
[FAIL. Reason: EvmError: Revert] testDecodeUniswapV3DataOutOfBounds() (gas: 5053)
Traces:
  [3252308] PrelaunchPointsPoCTest::setUp()
    ├─ [452586] → new MockLpETH@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
    │   └─ ← [Return] 2035 bytes of code
    ├─ [438773] → new MockLpETHVault@0x2e234DAe75C793f67A35089C9d99245E1C58470b
    │   └─ ← [Return] 1966 bytes of code
    ├─ [430367] → new ERC20Token@0xF62849F9A0B5Bf2913b396098F7c7019b51A820a
    │   └─ ← [Return] 1924 bytes of code
    ├─ [1573824] → new PrelaunchPoints@0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9
    │   └─ ← [Return] 7521 bytes of code
    ├─ [0] VM::deal(PrelaunchPointsPoCTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 10000000000000000000 [1e19])
    │   └─ ← [Return] 
    └─ ← [Stop] 

  [5053] PrelaunchPointsPoCTest::testDecodeUniswapV3DataOutOfBounds()
    └─ ← [Revert] EvmError: Revert

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.41ms (86.72µs CPU time)

Ran 1 test suite in 12.06s (1.41ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/PrelaunchPointsPoCTest.t.sol:PrelaunchPointsPoCTest
[FAIL. Reason: EvmError: Revert] testDecodeUniswapV3DataOutOfBounds() (gas: 5053)

The test testDecodeUniswapV3DataOutOfBounds confirms the existence of an out-of-bounds memory access issue in the _decodeUniswapV3Data function due to the misalignment of encodedPathLength. The PoC shows that when the encodedPathLength is not aligned with the 32-byte boundary, the function attempts to read incorrect memory, causing a revert.

koolexcrypto commented 3 months ago

Hi @mePopye

Thank you for the PoC.

If the demonstrated impact is only revert, the issue is deemed to be a QA. Since it is caused by a user input and the impact is on the user itself

mePopye commented 3 months ago

@koolexcrypto it's a valid concern ser. Maybe I am not able to demonstrate the real impact of this issue due to mine lack of experience/knowledge. But I think it should be considered

koolexcrypto commented 3 months ago

I understand this. but since this is depending on user input, many issues can be computed based on user input , ultimately those are QA according to severity categorisation