UniswapV3 Callback Miscalculation in SFPM Risks Loss of Funds for Payers.

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-panoptic/blob/153f0d82440b7e63075d55b0659706531431145f/contracts/SemiFungiblePositionManager.sol#L452-L459

Vulnerability details

Impact

The logic for determining the amountToPay, the current implementation assumes that one of the deltas (amount0Delta or amount1Delta) will always be positive and the other will be negative or zero. However, this assumption may not always hold.

If both amount0Delta and amount1Delta are negative, the amountToPay will be incorrectly set to the absolute value of amount1Delta, even though no tokens need to be paid. This can lead to unintended token transfers and potential loss of funds for the payer.

Consider the following scenario:

1. The Uniswap V3 pool executes a swap that results in both amount0Delta and amount1Delta being negative, indicating that the pool should receive tokens from the payer.
2. The uniswapV3SwapCallback function is called with these negative deltas.
3. The current implementation of the function incorrectly determines the amountToPay as the absolute value of amount1Delta.
4. The function proceeds to transfer the amountToPay from the payer to the pool, even though no tokens should be transferred in this case.

As a result, the payer may lose funds unnecessarily, and the pool may receive tokens that it shouldn't have received.

SemiFungiblePositionManager Contract (@contracts/SemiFungiblePositionManager.sol:452-459)

// Transform the amount to pay to uint256 (take positive one from amount0 and amount1)
// the pool will always pass one delta with a positive sign and one with a negative sign or zero,
// so this logic always picks the correct delta to pay
uint256 amountToPay = amount0Delta > 0 ? uint256(amount0Delta) : uint256(amount1Delta);

// Pay the required token from the payer to the caller of this contract
SafeTransferLib.safeTransferFrom(token, decoded.payer, msg.sender, amountToPay);

As mentioned earlier, the assumption that one of the deltas will always be positive and the other negative or zero is not always true. This can lead to the incorrect determination of amountToPay when both deltas are negative.

Proof of Concept

Let's consider an example:

1. The Uniswap V3 pool executes a swap that results in amount0Delta = -100 and amount1Delta = -50, indicating that the pool should receive 100 tokens of token0 and 50 tokens of token1 from the payer.
2. The uniswapV3SwapCallback function is called with these deltas.
3. The current implementation incorrectly sets amountToPay to uint256(amount1Delta), which is equivalent to uint256(-50). Due to the unsigned integer conversion, amountToPay becomes a large positive value.
4. The function proceeds to transfer this large amount of tokens from the payer to the pool, even though no tokens should be transferred.

This how the incorrect determination of amountToPay can lead to unintended token transfers and potential loss of funds for the payer.

Tools Used

Vs Code

Recommended Mitigation Steps

Modify the logic to handle the case when both deltas are negative.

// Transform the amount to pay to uint256 (take positive one from amount0 and amount1)
uint256 amountToPay;
if (amount0Delta > 0) {
    amountToPay = uint256(amount0Delta);
} else if (amount1Delta > 0) {
    amountToPay = uint256(amount1Delta);
} else {
    // Both deltas are negative, no need to pay
    return;
}

// Pay the required token from the payer to the caller of this contract
SafeTransferLib.safeTransferFrom(token, decoded.payer, msg.sender, amountToPay);

Assessed type

Error

[Picodes]

How can both be negative?

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid