`SwapRouter.sol` is vulnerable to address collission

Github username: @0xRizwan Twitter username: 0xRizwann Submission hash (on-chain): 0xe7139592dc505a9c73a3990d06c82ac8687a861f70355efe47640b5b3e023bda Severity: high

Description: Summary:

SwapRouter.sol 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.

Vulnerability Detail:

Before going in details, This issue is referenced from this issue which was also found in Kyberswap audit at sherlock.This issue was extensively discussed at sherlock.

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,

For more details, refer this article on The probability of a hash collision. Also refer this issue.

algebraSwapCallback() calls CallbackValidation.verifyCallback() to verify the callback.

    function algebraSwapCallback(int256 amount0Delta, int256 amount1Delta, bytes calldata _data) external override {
        require(amount0Delta > 0 || amount1Delta > 0, 'Zero liquidity swap'); // swaps entirely within 0-liquidity regions are not supported
        SwapCallbackData memory data = abi.decode(_data, (SwapCallbackData));
        (address tokenIn, address tokenOut) = data.path.decodeFirstPool();
        CallbackValidation.verifyCallback(poolDeployer, tokenIn, tokenOut);

   . . . some code
  }

CallbackValidation.verifyCallback(),

    function verifyCallback(
        address poolDeployer,
        PoolAddress.PoolKey memory poolKey
    ) internal view returns (IAlgebraPool pool) {
        pool = IAlgebraPool(PoolAddress.computeAddress(poolDeployer, poolKey));
        require(msg.sender == address(pool), 'Invalid caller of callback');
    }

The verifyCallback() used in algebraSwapCallback() function is used to verify that msg.sender is the address of the pool and only a require check is performed to verify if msg.sender has been computed via computeAddress().

However, the function never checks with the factory that the pool exists or any of the inputs are valid in any way. tokenIn can be constant and we can achieve the variation in the hash by changing tokenOut. The attacker could use tokenIn = WETH and vary tokenOut. 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.

This comment during sherlock discussion further proves the attack is possible.

Impact

The pool address check in the the callback function isn't strict enough and can suffer issues with collision.

Address collision can cause all allowances to be drained.

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.

Additional References

Reference 1

Reference 2

Additional information to consider:

Vitalik Buterin in November 2021 claims that: “even without address space extension, collisions today take 2^80 time to compute, and that length of time is already within range of nation-state-level actors willing to spend huge amounts of resources. For reference, the Bitcoin network performs 2^80 SHA256 hashes once every two hours.” https://ethresear.ch/t/what-would-break-if-we-lose-address-collision-resistance/11356

Mysten Labs have recently (October 2023) estimated the cost of a collision attack. They claim that “it's not a surprise that today, a 2^80 attack would cost a few million dollars”. https://mystenlabs.com/blog/ambush-attacks-on-160bit-objectids-addresses

Recommendation

Verify with the factory that msg.sender is a valid pool i.e Change the verifyCallback to check that msg.sender is a pool deployed by the factory

hats-finance / Fenix-Finance-0x83dbe5aa378f3ce160ed084daf85f621289fb92f

`SwapRouter.sol` is vulnerable to address collission #15