Unused user funds not refunded after `exactInputSingle()` swap

[c4-bot-9]

Lines of code

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

Vulnerability details

Description

The exactInputSingle() call assumes that all of the user funds or params.amountIn will be consumed. This is not necessarily true. A swap can be interrupted earlier than expected when there's not enough liquidity in a pool. Hence it's recommended that the protocol adds the logic to handle any unspent funds after exactInputSingle().

Proof of Concept

• See a similar past issue: https://solodit.xyz/issues/swaprouter-doesnt-refund-unspent-eth-after-swapping-spearbit-none-velodrome-finance-pdf

Please refer the second point "A swap can be interrupted earlier when there's not enough liquidity in a pool.". Also refer its recommendation section which suggests returning unspent ETH at the end of all swap styles, even for SwapRouter.exactInputSingle().

Also refer the exact fix made by the protocol inside exactInputSingle() here (click "Expand all" there to see the full code and the function name).

Tools Used

Manual review

Recommended Mitigation Steps

Just like the protocol has implemented a transfer after exactOutputSingle here, add the logic to transfer any unspent funds after exactInputSingle() too.

Assessed type

Other

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as selected for report

[stalinMacias]

Hello Judge @0xleastwood . I'd like to dispute the validity of this report since there is a big difference between the VelodromeRouter (referenced in this report), and the UniswapRouter, as well, as there is not enough evidence to support what is claimed on this report.

The difference between the two is that the VelodromeRouter supports swapping ETH for ERC20. By reading the referenced report, it is clear that the problem on the VelodromeRouter only happens when swapping ETH <=> ERC20, not when swapping ERC20 <=> ERC20. From the report itself:

The difference between selling ETH and an ERC20 token is that the contract can compute and request from the user the exact amount of ERC20 tokens to sell, but, when selling ETH, the user has to send the entire amount when making the call.

• The above makes a reference to a difference between the msg.value that is sent to the Router and the actual amountIn that is requested to be swapped, which, is completely different from assuming that the swap won't use the full amountIn when doing the swap.

I don't see any way how the problem reported in the referenced report applies to this protocol, since this protocol is interacting with the UniV3Router, not with a personalized Router. From the Uniswap Documentation: exactInputSingle() => Swaps amountIn of one token for as much as possible of another token

I think there is not enough evidence on this report for it to be granted a medium severity. I'd like to ask you to re-asses the validity of this report, and, also, I think it would like to be beneficial to all parties if the reporter could provide more evidence about when the UniswapRouter doing an exactInput swap of ERC20 <=> ERC20 would not consume all the amountIn.

[ickas]

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

[t0x1cC0de]

I believe enough evidence has been provided in my report along with the sponsor's acknowledgement & fix applied. But will let the judge decide if any further info is really required from my side.

[0xleastwood]

Does not seem viable because in ERC20 <-> ERC20 token swaps, each partial swap will only transferFrom() assets required for that partial swap. So there is no concern that funds would get "stuck" in the same fashion as per this report.

[c4-judge]

0xleastwood marked the issue as unsatisfactory: Insufficient proof