code-423n4 / 2024-04-panoptic-findings

2 stars 2 forks source link

Uniswap v3 callbacks access control should be hardened #541

Closed c4-bot-9 closed 2 months ago

c4-bot-9 commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/SemiFungiblePositionManager.sol#L435

Vulnerability details

Impact

Uniswap v3 uses a callback pattern to pull funds from the caller. The caller, (in this case Hypervisor) has to implement a callback function which will be called by the Uniswap’s pool. Both uniswapV3MintCallback and uniswapV3SwapCallback restrict the access to the callback functions only for the pool. However, this alone will not block a random call from the pool contract in case the latter was hacked, which will result in stealing all the funds held in Hypervisor or of any user that approved the Hypervisor contract to transfer tokens on his behalf.

Proof of Concept

/// @param data Contains the payer address and the pool features required to validate the callback
function uniswapV3SwapCallback(
    int256 amount0Delta,
    int256 amount1Delta,
    bytes calldata data
) external {
    // Decode the swap callback data, checks that the UniswapV3Pool has the correct address.
    CallbackLib.CallbackData memory decoded = abi.decode(data, (CallbackLib.CallbackData));
    // Validate caller to ensure we got called from the AMM pool
    CallbackLib.validateCallback(msg.sender, FACTORY, decoded.poolFeatures);

    // Extract the address of the token to be sent (amount0 -> token0, amount1 -> token1)
    address token = amount0Delta > 0
        ? address(decoded.poolFeatures.token0)
        : address(decoded.poolFeatures.token1);

    // 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);
}

Tools Used

Recommended Mitigation Steps

Consider adding (boolean) storage variables that will help to track whether a call

Assessed type

Uniswap

c4-judge commented 2 months ago

Picodes marked the issue as unsatisfactory: Invalid