When minting or increasing liquidity through NonfungiblePositionManager, excess ETH sent to the contract is not refunded

[c4-bot-8]

Lines of code

https://github.com/ronin-chain/katana-v3-contracts/blob/03c80179e04f40d96f06c451ea494bb18f2a58fc/src/periphery/NonfungiblePositionManager.sol#L246
https://github.com/ronin-chain/katana-v3-contracts/blob/03c80179e04f40d96f06c451ea494bb18f2a58fc/src/periphery/NonfungiblePositionManager.sol#L181

Vulnerability details

When minting or increasing liquidity through NonfungiblePositionManager, excess ETH sent to the contract is not refunded to the caller and stays in the contract. This results in a permanent loss for the user.

Proof of Concept

The NonfungiblePositionManager serves as the primary entry point for users interacting with pools in the protocol. Users can mint new positions or increase liquidity in existing ones .

When either the functions mint or increaseLiquidity is called, these functions trigger addLiquidity, which in turn invokes the mint function on the respective pool. This interaction triggers katanaV3MintCallback, which executes the pay function for each token in the pool to initiate the token transfers

    function pay(
        address token,
        address payer,
        address recipient,
        uint256 value
    ) internal {
        if (token == WETH9 && address(this).balance >= value) {
            // pay with WETH9
            IWETH9(WETH9).deposit{value: value}(); // wrap only what is needed to pay
            IWETH9(WETH9).transfer(recipient, value);
        } else if (payer == address(this)) {
            // pay with tokens already in the contract (for the exact input multihop case)
            TransferHelper.safeTransfer(token, recipient, value);
        } else {
            // pull payment
            TransferHelper.safeTransferFrom(token, payer, recipient, value);
        }
    }

If one of the tokens in the pool is WETH (WETH9) and the user send ETH with his call to be used for deposit, the NonfungiblePositionManager will wrap the exact amount of required ETH to WETH and transfer it to the pool.

It is nearly impossible to send in the exact amount of ETH to be exchanged to WETH since the current price in the pool at the moment the transaction will be executed might have changed since the submission of the transaction. This means, to ensure the transaction passes, users will be sending some additional ETH as a buffer. The problem arises from the fact that this additional ETH is not refunded to the user but stays in the NonfungiblePositionManager and anybody can claim it by calling refundETH which normal users will never do in the same transaction.

Recommended Mitigation Steps

Ensure that excess Eth is refunded to the caller .

function mint(
        MintParams calldata params
    )
        external
        payable
        override
        checkDeadline(params.deadline)
        returns (uint256 tokenId, uint128 liquidity, uint256 amount0, uint256 amount1)
    {
	…
+         if (address(this).balance > 0) TransferHelper.safeTransferETH(msg.sender, address(this).balance);
}

function increaseLiquidity(
        IncreaseLiquidityParams calldata params
    )
        external
        payable
        override
        checkDeadline(params.deadline)
        returns (uint128 liquidity, uint256 amount0, uint256 amount1)
    {
      ...
+         if (address(this).balance > 0) TransferHelper.safeTransferETH(msg.sender, address(this).balance);
}

Assessed type

ETH-Transfer

[nevillehuang]

This might be OOS, since uniswap has the same exact design as seen here. Additionally, if the user utilizes a multicall, this would be a non-issue.

All public known issues, including public audit reports of Uniswap V3 that affect Katana V3