Incorrect integration with UniswapRouter causes all swaps to revert.

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-05-bakerfi/blob/main/contracts/core/hooks/UseSwapper.sol#L67-L76
https://github.com/code-423n4/2024-05-bakerfi/blob/main/contracts/core/hooks/UseSwapper.sol#L84-L93

Vulnerability details

Impact

Core functions on the Strategy contract are impacted because the Strategy won't be able to do swaps caused an incorrect integration with the UniswapRouter.

• The deploy(), undeploy() and harvest() will revert when swapping the flashloaned borrowed WETH for collateral.

Proof of Concept

The UseSwapper._swap() function makes use of the UniswapRouter when executing swaps. The problem is that the parameters that are passed to the UniswapRouter are missing the deadline parameter, on both types of swaps (EXACT_INPUT && EXACT_OUTPUT).

• Not passing the deadline parameter to the UniswapRouter will cause the tx to be reverted because when the Router tries to read that parameter it won't find it. This throws an EvmError: Revert, which ultimately ends up reverting the whole tx.

> UseSwapper.sol

function _swap(
    ISwapHandler.SwapParams memory params
) internal override returns (uint256 amountOut) {
    ...

    // Exact Input
    if (params.mode == ISwapHandler.SwapType.EXACT_INPUT) {
        amountOut = _uniRouter.exactInputSingle(
          //@audit-issue => Not passing the deadline parameter
            IV3SwapRouter.ExactInputSingleParams({
                tokenIn: params.underlyingIn,
                tokenOut: params.underlyingOut,
                amountIn: params.amountIn,
                amountOutMinimum: 0,
                fee: fee,
                recipient: address(this),
                sqrtPriceLimitX96: 0
            })
        );
        if (amountOut == 0) {
            revert SwapFailed();
        }
        emit Swap(params.underlyingIn, params.underlyingOut, params.amountIn, amountOut);
        // Exact Output
    } else if (params.mode == ISwapHandler.SwapType.EXACT_OUTPUT) {
        uint256 amountIn = _uniRouter.exactOutputSingle(
          //@audit-issue => Not passing the deadline parameter
            IV3SwapRouter.ExactOutputSingleParams({
                tokenIn: params.underlyingIn,
                tokenOut: params.underlyingOut,
                fee: fee,
                recipient: address(this),
                amountOut: params.amountOut,
                amountInMaximum: params.amountIn,
                sqrtPriceLimitX96: 0
            })
        );
        ...
    }
}

If we take a look at the code of the SwapRouter, we can notice that the deadline is a parameter that must be sent as part of the params when calling the exactInputSingle() or exactOutputSingle() functions.

function exactInputSingle(ExactInputSingleParams calldata params)
    external
    payable
    override
    //@audit-issue => Because of this missing parameter, the whole tx will be reverted
    checkDeadline(params.deadline)
    returns (uint256 amountOut)
{
    ...
}

function exactOutputSingle(ExactOutputSingleParams calldata params)
    external
    payable
    override
    //@audit-issue => Because of this missing parameter, the whole tx will be reverted
    checkDeadline(params.deadline)
    returns (uint256 amountIn)
{
    ...
}

Coded PoC

I coded a PoC in Foundry to demonstrate the integration error, and show that the missing deadline parameter causes the tx to be reverted.

First of all, help me to create a new folder where we are going to create a couple of test files for this PoC. Create the new folder under the tests folder

• Now, from this gist we can obtain the code for the 3 files required to run the PoC. Each of these files are:

1. ISwapRouter.sol => This is simply the exact same interface used on the Original SwapRouter
2. MockUniswapRouter.sol => This is a Mock Contract of the SwapRouter. I extracted the minimum required code to reproduce the integration error on the exactInputSingle() function
3. UseSwapper.t.sol => In this file we create a new contract by inheriting the original UseSwapper.sol contract, and we just simply add some functions to allow us setting the address of the UniRouter that will be called when the _swap() is invoked. We also create a new function that executes the call to the SwapRouter in the correct way (without missing the deadline parameter)

• Last but not least, in this file is the actual test that we execute to demonstrate the integration error and the correct way of how the SwapRouter should be called.

Before running the PoC, is required to change the visibility of the uniRouter variable in the UseSwapper.sol contract. Update it to internal instead of private

Run the PoC with this command forge test --match-test test_swapFunctionFromUseSwapperContract_PoC -vvvv

Output of running the PoC

forge test --match-test test_swapFunctionFromUseSwapperContract_PoC -vvvv
[⠘] Compiling...
No files changed, compilation skipped

Ran 1 test for test/foundryUseSwapPoC/UseSwapper.t.sol:TestUseSwapper
[PASS] test_swapFunctionFromUseSwapperContract_PoC() (gas: 804034)
Traces:
  [804034] TestUseSwapper::test_swapFunctionFromUseSwapperContract_PoC()
    ├─ [121365] → new SwapRouterMock@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
    │   └─ ← [Return] 606 bytes of code
    ├─ [564600] → new UseSwapperPoC@0x2e234DAe75C793f67A35089C9d99245E1C58470b
    │   └─ ← [Return] 2820 bytes of code
    ├─ [44634] UseSwapperPoC::setUniRouter(SwapRouterMock: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f])
    │   └─ ← [Stop]
  
    //@audit => First call - Missing deadline parameter
    ├─ [0] VM::expectRevert(custom error f4844814:)
    │   └─ ← [Return]
    ├─ [1556] UseSwapperPoC::swapReverts()
    │   ├─ [80] SwapRouterMock::exactInputSingle(ExactInputSingleParams({ tokenIn: 0x0000000000000000000000000000000000000014, tokenOut: 0x000000000000000000000000000000000000000A, fee: 500, recipient: 0x2e234DAe75C793f67A35089C9d99245E1C58470b, amountIn: 1000, amountOutMinimum: 0, sqrtPriceLimitX96: 0 }))
    │   │   └─ ← [Revert] EvmError: Revert
    │   └─ ← [Revert] EvmError: Revert

    //@audit => Second call - Passing all the required parameters!
    ├─ [2910] UseSwapperPoC::swapSucceeds()
    │   ├─ [1365] SwapRouterMock::exactInputSingle(ExactInputSingleParams({ tokenIn: 0x0000000000000000000000000000000000000014, tokenOut: 0x000000000000000000000000000000000000000A, fee: 500, recipient: 0x2e234DAe75C793f67A35089C9d99245E1C58470b, deadline: 1, amountIn: 1000, amountOutMinimum: 0, sqrtPriceLimitX96: 0 }))
    │   │   └─ ← [Return] 1000
    │   └─ ← [Stop]
    └─ ← [Return] 0

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.59ms (1.74ms CPU time)

Ran 1 test suite in 361.52ms (6.59ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

• As we can see, the first call which uses the original UseSwapper._swap() function ends up reverting.
• Only the second call succeeds, which is the one that uses the new function where the correct parameters are sent to the SwapRouter contract .

Tools Used

Manual Audit, Foundry & SwapRouter contract

Recommended Mitigation Steps

The mitigation for this integration error is to pass all the required parameters when calling the functions of the SwapRouter.
Specifically, update the parameters that are sent to the SwapRouter like this: (We are going to use the original ISwapRouter.ExactInputSingleParams to prevent any errors)
Also, make sure that the UniRouter variable is of the type ISwapRouter (the same interface used by the UniswapRouter!), insteaf of IV3SwapRouter.

Note: Make sure to have imported the correct ISwapRouter interface

abstract contract UseSwapper is ISwapHandler, Initializable {
    
-   IV3SwapRouter private _uniRouter;
+   ISwapRouter private _uniRouter;

    function _initUseSwapper(ServiceRegistry registry) internal onlyInitializing {
-       _uniRouter = IV3SwapRouter(registry.getServiceFromHash(UNISWAP_ROUTER_CONTRACT));
+       _uniRouter = ISwapRouter(registry.getServiceFromHash(UNISWAP_ROUTER_CONTRACT));
        if (address(_uniRouter) == address(0)) revert InvalidUniRouterContract();
    }

-   function uniRouter() public view returns (IV3SwapRouter) {
-   function uniRouter() public view returns (ISwapRouter) {  
        return _uniRouter;
    }

    function uniRouterA() public view returns (address) {
        return address(_uniRouter);
    }

    function _swap(
        ISwapHandler.SwapParams memory params
    ) internal override returns (uint256 amountOut) {
        ...

        // Exact Input
        if (params.mode == ISwapHandler.SwapType.EXACT_INPUT) {
            amountOut = _uniRouter.exactInputSingle(
-               IV3SwapRouter.ExactInputSingleParams({
+               ISwapRouter.ExactInputSingleParams({
                    tokenIn: params.underlyingIn,
                    tokenOut: params.underlyingOut,
                    amountIn: params.amountIn,
                    amountOutMinimum: 0,
                    fee: fee,
                    recipient: address(this),
+                   deadline: block.timestamp,                    
                    sqrtPriceLimitX96: 0
                })
            );
            ...

        } else if (params.mode == ISwapHandler.SwapType.EXACT_OUTPUT) {
            uint256 amountIn = _uniRouter.exactOutputSingle(
-               IV3SwapRouter.ExactOutputSingleParams({
+               ISwapRouter.ExactOutputSingleParams({  
                    tokenIn: params.underlyingIn,
                    tokenOut: params.underlyingOut,
                    fee: fee,
                    recipient: address(this),
                    amountOut: params.amountOut,
                    amountInMaximum: params.amountIn,
+                   deadline: block.timestamp,     
                    sqrtPriceLimitX96: 0
                })
            );
            ...
        }
    }
}

Assessed type

Uniswap

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as selected for report

[ickas]

Fixed → https://github.com/baker-fi/bakerfi-contracts/pull/43

[t0x1cC0de]

Hi @0xleastwood,
I think it's an interesting report. May I ask however if the current issue is in any way different from the invalidated one which was raised in the Decent protocol audit contest? I found one of the comments there by the warden; another pretty involved comment by a different warden here which finds the judge's response here & a final comment by the judge here, but haven't had the chance to sift through all the nuances of the discussion as there are multiple interlinked threads. All I could quickly see at first glance was that the judge invalidated the primary issue for some reason. Is there any difference here for it to not be considered invalid?

[stalinMacias]

Hi @0xleastwood, I think it's an interesting report. May I ask however if the current issue is in any way different from the invalidated one which was raised in the Decent protocol audit contest? I found one of the comments there by the warden; another pretty involved comment by a different warden here which finds the judge's response here & a final comment by the judge here, but haven't had the chance to sift through all the nuances of the discussion as there are multiple interlinked threads. All I could quickly see at first glance was that the judge invalidated the primary issue for some reason. Is there any difference here for it to not be considered invalid?

Hey @t0x1cC0de Thanks for pointing out to those reports of the Decent contest. This is why all the reports about not setting deadline were invalidated, including one I reported back then, because I also failed to point out the real problem, and, instead, I just mentioned the possible mev problem (which is totally incorrect)

• From the judge's comment we can see that he invalidated all those reports because none of them correctly pointed the real problem of not passing the deadline parameter to the UniRouter, all those reports were talking about as if deadline would be set to 0, and those reports were talking about the potential MEV issue when deadline is not set by the user, instead of pointing out the real problem that is that deadline doesn't exists, which is what causes the tx to revert.

@0xleastwood This is exactly what I said in my comment on issue #12
#12 does not point out the real problem of not passing the deadline parameter, instead, it just talks about the possible mev issue.

[t0x1cC0de]

Thanks @stalinMacias for the explanation. I think code-423n4/2024-01-decent-findings#117 (and the analysis in 172) cite a different reason for invalidation.
But will let @0xleastwood analyse it further.
Nothing more from my side.

Thanks

[hvasconcelos]

We are using SwapRouter02 instead of SwapRouter , on new deployments like Base the SwapRouter is not deployed anymore by uniswap team , so we decided to use this contract:

https://github.com/Uniswap/swap-router-contracts/blob/v1.1.0/contracts/interfaces/IV3SwapRouter.sol

to be compatible with all the deployments(Ethereum, Base, Optimism, Arbitrum,... ).

We already tested integrations with production chains(Ethereum, Artbitrum) and everything looks to work fine.

[0xleastwood]

As per sponsor's comment, #42 and #12 are invalid.

[c4-judge]

0xleastwood marked the issue as unsatisfactory:
Invalid