Lines of code

https://github.com/code-423n4/2024-06-vultisig/blob/cb72b1e9053c02a58d874ff376359a83dc3f0742/hardhat-vultisig/contracts/oracles/uniswap/uniswapv0.8/FullMath.sol#L1-L115 https://github.com/code-423n4/2024-06-vultisig/blob/cb72b1e9053c02a58d874ff376359a83dc3f0742/hardhat-vultisig/contracts/oracles/uniswap/uniswapv0.8/TickMath.sol#L1-L208

Vulnerability details

Title

Unchecked Arithmetic Operations in FullMath and TickMath Libraries Leading to Potential Overflows and Unexpected Behavior

Impact

First of all, take a look at the diffChecker of the implementation in UniswapV3 and Vultisig here:

As seen in both links above, the UniswapV3 contracts are wrapped in Unchecked blocks.

Now, the FullMath and TickMath libraries are used extensively throughout the protocol, including in critical contracts such as SqrtPriceMathPartial, LiquidityAmounts, ILOPool, OracleLibrary, and ILOManager. The lack of unchecked arithmetic operations in these libraries can lead to potential overflows, resulting in incorrect calculations.

In SqrtPriceMathPartial and LiquidityAmounts, the FullMath library is used for various calculations involving token amounts and liquidity. If an overflow occurs due to the absence of unchecked blocks, it could result in incorrect token amounts being transferred or liquidity being allocated, potentially leading to loss of funds or unfair distribution of tokens.

The ILOPool contract also relies on FullMath for calculations related to vesting, fee deductions, and liquidity management. Overflows in these calculations could cause incorrect vesting schedules, inaccurate fee deductions, and improper liquidity tracking, impacting the overall functionality and fairness of the ILO process.

In OracleLibrary, both FullMath and TickMath are used for price calculations and conversions. Overflows in these operations could lead to incorrect price quotes and oracle data.

The ILOManager contract uses TickMath for initializing ILO pools and validating price ranges. If overflows occur in these calculations, it could possible result in the creation of ILO pools with incorrect parameters or the acceptance of invalid price ranges, potentially affecting the logic of the ILO process.

Proof of Concept

Here are a few relevant code snippets that demonstrate the issue:

In FullMath.sol:

function mulDiv(uint256 a, uint256 b, uint256 denominator) internal pure returns (uint256 result) {
    // ...
    uint256 prod0; // Least significant 256 bits of the product
    uint256 prod1; // Most significant 256 bits of the product
    assembly {
        let mm := mulmod(a, b, not(0))
        prod0 := mul(a, b)
        prod1 := sub(sub(mm, prod0), lt(mm, prod0))
    }
    // ...
}

In TickMath.sol:

function getSqrtRatioAtTick(int24 tick) internal pure returns (uint160 sqrtPriceX96) {
    // ...
    uint256 absTick = tick < 0 ? uint256(-int256(tick)) : uint256(int256(tick));
    // ...
    if (absTick & 0x1 != 0) ratio = (ratio * 0xfffcb933bd6fad37aa2d162d1a594001) >> 128;
    if (absTick & 0x2 != 0) ratio = (ratio * 0xfff97272373d413259a46990580e213a) >> 128;
    // ...
}

As seen above, arithmetic operations such as mul, sub, and >> are used without being wrapped in unchecked blocks. This can lead to overflows if the intermediate results exceed the maximum value representable by uint256.

Tools Used

Manual review

Recommended Mitigation Steps

Wrap FullMath and TickMath libraries operations with unchecked block, similar to https://github.com/Uniswap/v3-core/blob/6562c52e8f75f0c10f9deaf44861847585fc8129/contracts/libraries/FullMath.sol and https://github.com/Uniswap/v3-core/blob/6562c52e8f75f0c10f9deaf44861847585fc8129/contracts/libraries/TickMath.sol respectively

Assessed type

Under/Overflow

Hi judge, thank you for judging this contest.

Please, I think you should have a look at this issue.

According to the notes left by the validator, "unchecked applies only to Solidity 8" was the reason why this invalidated.

First of all, The UniswapV3Oracle contract specifies Solidity version ^0.8.24.:

pragma solidity ^0.8.24;

Even the hardhat.config.ts file states that the contract is deployed with Solidity version 0.8.24.


    const config: HardhatUserConfig = {
❌    solidity: "0.8.24",
      paths: {
        sources: "./hardhat-vultisig/contracts",
        tests: "./hardhat-vultisig/test",
      },

The UniswapV3Oracle contract uses functions from the OracleLibrary, which in turn makes use of FullMath and TickMath.

The FullMath library supports Solidity version >=0.4.0 and TickMath library supports >=0.5.0. These libraries rely on arithmetic operations that wrap on overflow. However, starting with Solidity version 0.8.0, arithmetic operations revert by default on overflow. Since the code was originally written for Solidity version 0.7.6, these operations must be wrapped in an unchecked block when used in Solidity >=0.8.0.

The issue is that the version upper cap in this Vultsig's implementation(>=0.4.0 and >=0.5.0) is not set meaning it could be up to solidity 0.8 but the arithmetic operations are not be wrapped in an unchecked block.

This issue trickles down to many aspects of the protocol. For example, in peek function, the getQuoteAtTick function is used to get quotedWETHAmount.


    function peek(uint256 baseAmount) external view returns (uint256) {
        uint32 longestPeriod = OracleLibrary.getOldestObservationSecondsAgo(pool);
        uint32 period = PERIOD < longestPeriod ? PERIOD : longestPeriod;
        int24 tick = OracleLibrary.consult(pool, period);
❌      uint256 quotedWETHAmount = OracleLibrary.getQuoteAtTick(tick, BASE_AMOUNT, baseToken, WETH);
        return (quotedWETHAmount * baseAmount * 95) / 1e20; // 100 / 1e18
    }

getQuoteAtTick calls getSqrtRatioAtTick from TickMath to get sqrtRatioX96.


    function getQuoteAtTick(
        int24 tick,
        uint128 baseAmount,
        address baseToken,
        address quoteToken
    ) internal pure returns (uint256 quoteAmount) {
❌     uint160 sqrtRatioX96 = TickMath.getSqrtRatioAtTick(tick);

getSqrtRatioAtTick function should be wrapped in an unchecked block for Solidity versions >0.8.0, as seen in Uniswap's official implementation. Without this, the function will revert in Solidity >=0.8.0.

Same applies to the FullMath contract.

The solution is just to wrap the arithmetic operation in TickMath and FullMath in unchecked blocks.

Hey @Rhaydden, thank you for the PJQA contribution. I will preface all validation repository finding responses by stating that they are not evaluated by judges directly and are only evaluated by the validators if they are deemed unsatisfactory.

The referenced arithmetic operations cannot lead to an overflow, and they are wrapped in an unchecked code block as an optimization. As no vulnerability can arise from this discrepancy, this submission would be considered QA at best. You are free to prove under which values an overflow would occur, but based on prior analysis in a private contest no such value was a valid tick (i.e. the function safely calculates for all valid tick values between MIN_TICK and MAX_TICK).

This paragraph is included in all of my responses and confirms that no further feedback is expected in this submission as PJQA has concluded. You are free to refute any of my statements factually, however, I strongly implore you to do this with actual code references and examples.

code-423n4 / 2024-06-vultisig-validation

Unchecked Arithmetic Operations in FullMath and TickMath Libraries Leading to Potential Overflows and Unexpected Behavior #58