c4-bot-10 commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L415 https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L449 https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/libraries/CallbackLib.sol#L28-L52

Vulnerability details

Impact

SemiFungiblePositionManager.sol#uniswapV3SwapCallback() never verifies that the callback msg.sender is actually a deployed pool. This allows for a provable address collision that can be used to drain all allowances to the router.

Proof of Concept

The pool address check in the the callback function isn't strict enough and can suffer issues with collision. Due to the truncated nature of the create2 opcode the collision resistance is already impaired to 2^160 as that is total number of possible hashes after truncation. Obviously if you are searching for a single hash, this is (basically) impossible. The issue here is that one does not need to search for a single address as the router never verifies that the pool actually exists. This is the crux of the problem, but first let's do a little math as to why this is a problem.

A primer on hash collision probability can be found here (https://kevingal.com/blog/collisions.html). The odds of a collision are dependant on both the amount of addresses to search against as well as the number of brute force attempts to match them. From the article linked we can see that the estimated odds of collision are:

1 - e ^ {-k(k-1) \over 2N }

Where k is the number of hash values and N is the number of possible hashes

For very large numbers we can further approximate the exponent to:

-k^2 \over 2N

This exponent is now trivial to solve for an approximate attack value which is: k = \sqrt{2N}

In our scenario N is 2^160 (our truncated keccak256) which means that our approximate attack value is 2^80 since we need to generate two sets of hashes. The first set is to generate 2^80 public addresses and the second is to generate pool address from variations in the pool specifics(token0, token1, fee). Here we reach a final attack value of 2^81 hashes. Using the provided calculator we see that 2^81 hashes has an approximate 86.4% chance of a collision. Increase that number to 2^82 and the odds of collision becomes 99.96%. In this case, a collision between addresses means breaking this check and draining the allowances of all users to a specific token. This is because the EOA address will collide with the supposed pool address allowing it to bypass the msg.sender check. Now we considered the specific of the contract.

SemiFungiblePositionManager.sol#L415-L462

    function uniswapV3MintCallback(uint256 amount0Owed, uint256 amount1Owed,bytes calldata data ) external 
    {
        // Decode the mint callback data
        CallbackLib.CallbackData memory decoded = abi.decode(data, (CallbackLib.CallbackData));
        // Validate caller to ensure we got called from the AMM pool
        CallbackLib.validateCallback(msg.sender, address(FACTORY), decoded.poolFeatures);

        // ... CODE TRANSFERRING TOKEN TO MSG.SENDER //
    }

    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, address(FACTORY), decoded.poolFeatures);

        // ... CODE TRANSFERRING TOKEN TO MSG.SENDER //

    }

libraries/CallbackLib.sol#L28-L52

    struct PoolFeatures {
        address token0;
        address token1;
        uint24 fee;
    }
    function validateCallback(
        address sender,
        address factory,
        PoolFeatures memory features
    ) internal pure {
        // compute deployed address of pool from claimed features and canonical factory address
        // then, check against the actual address and verify that the callback came from the real, correct pool
        if (
            address( uint160(uint256(keccak256(abi.encodePacked(
                                bytes1(0xff),
                                factory,
                                keccak256(abi.encode(features)),
                                Constants.V3POOL_INIT_CODE_HASH
             ))))) != sender ) revert Errors.InvalidUniswapCallback();
    }
}

We see that these lines never check with the factory that the pool exists or any of the inputs are valid in any way. token0 can be constant and we can achieve the variation in the hash by changing token1. The attacker could use token0 = WETH and vary token1. This would allow them to steal all allowances of WETH. Since allowances are forever until revoked, this could put hundreds of millions of dollars at risk.

Although this would require a large amount of compute it is already possible to break with current computing. Given the enormity of the value potentially at stake it would be a lucrative attack to anyone who could fund it. In less than a decade this would likely be a fairly easily attained amount of compute, nearly guaranteeing this attack.

Impact => Address collision can cause all allowances to the SPFM by the owner of it to be drained

Tools Used

Visual Studio Code same Sherlock Issue

Recommended Mitigation Steps

Verify with the factory that msg.sender is a valid pool

Assessed type

Uniswap

c4-judge commented 10 months ago

Picodes marked the issue as duplicate of #128