V3 Proxy does not send funds to the recipient, instead it sends to the msg.sender

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/helper/V3Proxy.sol#L112-L194

Vulnerability details

Impact

The functions above can be used to swap tokens, however the swaps are not sent to the provided address. Instead they are sent to the msg.sender. This could cause issues if the user has been blacklisted on a token. Or if the user has a compromised signature/allowance of the target token and they attempt to swap to the token, the user looses all value even though they provided an destination adress

Proof of Concept

//@audit-H does not send tokens to the required address

    function swapExactTokensForTokens(uint amountIn, uint amountOutMin, address[] calldata path, address to, uint deadline) external returns (uint[] memory amounts) {
        require(path.length == 2, "Direct swap only");
        ERC20 ogInAsset = ERC20(path[0]);
        ogInAsset.safeTransferFrom(msg.sender, address(this), amountIn);
        ogInAsset.safeApprove(address(ROUTER), amountIn);
        amounts = new uint[](2);
        amounts[0] = amountIn;         
//@audit-issue it should be the to address not msg.sender
        amounts[1] = ROUTER.exactInputSingle(ISwapRouter.ExactInputSingleParams(path[0], path[1], feeTier, msg.sender, deadline, amountIn, amountOutMin, 0));
        ogInAsset.safeApprove(address(ROUTER), 0);
        emit Swap(msg.sender, path[0], path[1], amounts[0], amounts[1]); 
    }

Here is one of the many functions with this issue, As we can see after the swap is completed, tokens are sent back to the msg.sender from the router not to the to address

Tools Used

Manual Review. Uniswap Router: https://github.com/Uniswap/v3-periphery/blob/main/contracts/SwapRouter.sol

Recommended Mitigation Steps

The uniswap router supports inputting of a destination address. Hence the router should be called with the to address not the msg.sender. Else remove the address to from the parameter list

Assessed type

Uniswap

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-sponsor]

Keref marked the issue as sponsor acknowledged

[Keref]

Cannot remove to from param list as the stated goal is to be compatible with uniswap v2 interface

[c4-sponsor]

Keref marked the issue as sponsor confirmed

[c4-sponsor]

Keref marked the issue as disagree with severity

[Keref]

Although it looks like a severe issue, in our case this was actually on purpose as this is a compatibility module used for OPM only, where it's always msg.sender == to, and to avoid potential spam under GE banner (ppl swapping spam tokens to famous addresses).

So it's not a high risk issue, maybe low only

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 marked the issue as selected for report

[c4-judge]

gzeon-c4 changed the severity to 2 (Med Risk)

[gzeon-c4]

recommend to restrict caller to OPM

[Keref]

We could have several (out of this audit scope) contracts that need to swap, restricting to msg.sender is enough

[gzeoneth]

right, so may be checking msg.sender == to ?

[Keref]

Accepted, see PR#10