User can lose funds with `swapTokensForExactETH` and these funds can be stolen.

[code423n4]

Lines of code

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

Vulnerability details

Impact

After a swap is made with swapTokensForExactETH, leftover tokens don't be refunded to the caller and funds not refunded can be stolen by a malicious user.

Proof of Concept

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

The goal of the swapTokensForExactETH function in the V3Proxy contract is to make a swap in sending tokens in exchange for an amount of WETH. Through the call to Uniswap Router with exactOutputSingle, the user sends a quantity of tokens into the V3Proxy contract to make a swap in expectation to receive a precise amount of WETH he wants at the end. He receive is WETH, but because of missing an action to send back the unused token to the caller. After the swap is made, these tokens stay in the V3Proxy contract. When a user makes a swap using swapTokensForExactETH, the sending remaining tokens are not refunded, the user lose a part of its tokens.

Furthermore, after a bad swap, a malicious user can use swapTokensForExactTokens function to make a swap and steal the tokens present in the contract.

Example of a scenario where a malicious user steal the remaining assets of and these remaining fund are

1. Alice make a swap using the swapTokensForExactETH function, send amount of TokenA.
2. Alice receive the WETH amount she has requested from the swap, but its remaining TokenA are not refund and stay in the balance of V3Proxy contract.
3. Bob (malicious user) see this transactions and decide to steal TokenA by make a swap with the swapTokensForExactTokens function.
4. Bob make a minimal swap of TokenA to TokenB, he send amount of Token A.
5. Bob receive the amount of TokenB he requested. And the function give them back its remaining TokenA and the remaining TokenA of Alice because the swapTokensForExactTokens send all the contract balance of the TokenA.

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

At the end Bob receive the amount of TokenB he want for the swap and steal the funds of the TokenA losed by Alice and by the others potentials users.

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend to add the following line in the swapTokensForExactETH function :

function swapTokensForExactETH(uint amountOut, uint amountInMax, address[] calldata path, address to, uint deadline) payable external returns (uint[] memory amounts) {

        ...

        amounts[0] = ROUTER.exactOutputSingle(ISwapRouter.ExactOutputSingleParams(path[0], path[1], feeTier, address(this), deadline, amountOut, amountInMax, 0));         
        amounts[1] = amountOut;
         ++ ogInAsset.safeTransfer(msg.sender, ogInAsset.balanceOf(address(this)));
        ogInAsset.safeApprove(address(ROUTER), 0);
        IWETH9 weth = IWETH9(ROUTER.WETH9());

        ...

        emit Swap(msg.sender, path[0], path[1], amounts[0], amounts[1]); 
    }

Assessed type

Token-Transfer

[c4-pre-sort]

141345 marked the issue as duplicate of #64

[c4-judge]

gzeon-c4 marked the issue as satisfactory