Uniswap V3 calls will revert with `Transaction too old` due to missing deadline param

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-01-decent/blob/main/src/swappers/UniSwapper.sol#L129-L135 https://github.com/code-423n4/2024-01-decent/blob/main/src/swappers/UniSwapper.sol#L149-L156 https://github.com/Uniswap/v3-periphery/blob/main/contracts/base/PeripheryValidation.sol#L7-L10 https://github.com/Uniswap/v3-periphery/blob/main/contracts/SwapRouter.sol#L132-L137 https://github.com/Uniswap/v3-periphery/blob/main/contracts/SwapRouter.sol#L224-L229

Vulnerability details

Impact

UniSwapper::swapExactIn() and UniSwapper:swapExactOut() will revert when called due to a missing param of deadline that is required by the Uniswap V3 router.

Proof of Concept

Users won't be able to perform swaps in UniSwapper due to a missing deadline param. The following functions are affected:

• swapExactIn()
• swapExactOut()
• swap() (affected as it's a wrapper for the two above)

The issue stems from the following two parts of the code:

IV3SwapRouter.ExactInputParams memory params = IV3SwapRouter
    .ExactInputParams({
        path: swapParams.path,
        recipient: address(this),
        amountIn: swapParams.amountIn,
        amountOutMinimum: swapParams.amountOut
    });

IERC20(swapParams.tokenIn).approve(uniswap_router, swapParams.amountIn);
amountOut = IV3SwapRouter(uniswap_router).exactInput(params);

IV3SwapRouter.ExactOutputParams memory params = IV3SwapRouter
    .ExactOutputParams({
        path: swapParams.path,
        recipient: address(this),
        //deadline: block.timestamp,
        amountOut: swapParams.amountOut,
        amountInMaximum: swapParams.amountIn
    });

IERC20(swapParams.tokenIn).approve(uniswap_router, swapParams.amountIn);
amountIn = IV3SwapRouter(uniswap_router).exactOutput(params);

The invocations for exactInput() and exactOutput() here will revert when a swap is attempted. The reason is that the Uniswap V3 router checks if the deadline passed in is less than the current block.timestamp, if it is, it reverts (default is zero for uint256):

function exactInput(ExactInputParams memory params)
  external
  payable
  override
  checkDeadline(params.deadline)
  returns (uint256 amountOut) {
  // ...
}

function exactOutput(ExactOutputParams calldata params)
  external
  payable
  override
  checkDeadline(params.deadline)
  returns (uint256 amountIn) {
  // ...
}

modifier checkDeadline(uint256 deadline) {
    require(_blockTimestamp() <= deadline, 'Transaction too old');
    _;
}

Tools Used

Manual Analysis

Recommended Mitigation Steps

Let users pass the deadline into the swapParams struct param and use it for the params passed into V3SwapRouter::exactInput() and V3SwapRouter::exactOutput().

Assessed type

Uniswap

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #117

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg marked the issue as selected for report

[alex-ppg]

Awarded as best due to a short-and-sweet submission with direct excerpts from Uniswap V3 to properly argue why the code would fail.

[DadeKuma]

@alex-ppg

I think this issue is not High risk according to the severity categorization, as none of the assets are stolen/lost/compromised:

3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).

Medium seems more appropriate IMO, as only the function/availability of the protocol will be impacted:

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

[ZdravkoHr]

Hello, @alex-ppg. I think this issue should not be valid as it assumes that Decent uses the Uniswap Router from the periphery. Uniswapper.sol has the following import

import {IV3SwapRouter} from "@uniswap/swap-contracts/interfaces/IV3SwapRouter.sol";

If we have a look at the remappings, we can see that this path leads to swap-router-contracts

@uniswap/swap-contracts/=lib/swap-router-contracts/contracts

The router there does not implement any deadlines and the transaction won't be reverted.

[0xNentoR]

@ZdravkoHr This is a pretty interesting thing you found there. I dug deeper and found out that Uniswap V3 actually has 3 swap routers, which I wasn't aware of.

The periphery one is called SwapRouter, while the one you linked is SwapRouter02.

In SwapRouter02, they turned the checkDeadline modifier into an optional function: https://github.com/Uniswap/swap-router-contracts/commit/ca72039303eaa46df02d5000bb3e519ee04100c5. The decision was made based on the following issue: https://github.com/Uniswap/swap-router-contracts/issues/3. I'm not sure what he meant by "don't need to fail or can fail fast" since you can always pass a deadline far higher than the current block timestamp but they agreed and proceeded with implementing it that way.

Interestingly enough, the docs are, perplexingly, still providing integration examples with the old router: https://docs.uniswap.org/contracts/v3/guides/swaps/single-swaps

Now, to make things even more confusing, there's a third one called UniversalRouter and apparently that's the one recommended for use at the moment. I found the following information in the deployment addresses section of the docs:

The UniversalRouter contract is the current preferred entrypoint for ERC20 and NFT swaps, replacing, among other contracts, SwapRouter02. An up-to-date list of deploy addresses by chain is hosted on Github.

If we take a peek at that one, we will see that the checkDeadline is back as a modifier and is still reverting in the same case I described in this submission: https://github.com/Uniswap/universal-router/blob/main/contracts/UniversalRouter.sol#L15-L18. But this one is totally different in terms of implementation and not backwards-compatible with the two above.

@alex-ppg So, in the end, if this is the intended interface and the sponsors still want to ensure transactions are always executed within a specified time frame, they should modify the code to make a call tocheckDeadline() inside swapExactIn() or swapExactOut() (or the wrapper one, swap()) and the deadline should be bundled inside swapParams (or a new func param added).

This is an example of a submission that has correctly identified the root cause: https://github.com/code-423n4/2024-01-decent-findings/issues/601. The mitigation proposed, however, is to switch to the periphery router.

[alex-ppg]

First, I would like to commend all Wardens who have contributed to the issue. When I was directly discussing with the Sponsor, they did not state that the conventional Uniswap V3 router should not be supported and I had directly linked its deployment to them.

@DadeKuma

I think this issue is not High risk according to the severity categorization, as none of the assets are stolen/lost/compromised:

The severity categorization rulebook you reference is slightly outdated and does not reflect the latest understanding of how judges grade issues. I will refer to the Appendix of the latest Supreme Court and specifically the following views of SC judges who state that the protocol being halted (or a major invariant breached) may fall under high-risk classification. In this case, the protocol is meant to support a feature it does not and this constitutes a high-risk vulnerability.

@ZdravkoHr

Hello, @alex-ppg. I think this issue should not be valid as it assumes that Decent uses the Uniswap Router from the periphery.

Thank you for your due diligence; when I first brought up this issue to the attention of the Sponsor they were unaware of why their tests were succeeding but this clarifies the discrepancy. To note, I had linked the live deployment of the Uniswap V3 router to the Sponsor and discussed it yet they did not claim that it should not be supported, so their overall stance on this matter was slightly unclear.

To aid the Sponsor, I will link them to this thread and clarify how each router behaves as they may ultimately wish to move on with a different one depending on their use cases.

@0xNentoR thank you for expanding on the reply of @ZdravkoHr

Based on the repository the interface is fetched from, I cannot consider this vulnerability as valid given that no deadline member is present in the struct. Additionally, based on this issue present in the repository the interface is fetched from, the absence of a deadline check is not recommended for user interactions. As such, a form of deadline should be evaluated (like #601 specifies). However, I do not consider such an issue as a high-risk or medium-risk vulnerability due to the cross-chain nuisances that the Decent protocol faces.

In reality, a cross-chain transaction should be processed whenever it can, and enforcing slippage checks is sufficient in guaranteeing it has executed as its caller intended. Culminating the above discussions, I will invalidate this submission once the PJQA period is over.

[c4-sponsor]

wkantaros marked the issue as disagree with severity

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid