Revert Lend - MohammedRizwan's results

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/automators/Automator.sol#L148 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/transformers/AutoCompound.sol#L129 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Oracle.sol#L363 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Oracle.sol#L422

Vulnerability details

Impact

Tokens value can be manipulated to cause loss of funds for the protocol and other users via flash loans, MEV searchers etc. This allows a malicious user to manipulate the valuation of the LP. An example of this kind of manipulation would be to use large buys/sells to alter the composition of the LP to make it worth less or more. A big swap using a flash loan can push the liquidity to one side only.

Proof of Concept

Revert lend contracts like Automator.sol, AutoCompound.sol and V3Oracle.sol has used slot0 data of uniswap v3 pool.

File: src/automators/Automator.sol

148        (sqrtPriceX96, currentTick,,,,,) = pool.slot0();

File: src/transformers/AutoCompound.sol
128                IUniswapV3Pool pool = _getPool(state.token0, state.token1, state.fee);
129                (state.sqrtPriceX96, state.tick,,,,,) = pool.slot0();

File: src/transformers/AutoCompound.sol

363            (sqrtPriceX96,,,,,,) = pool.slot0();


421        state.pool = _getPool(token0, token1, fee);
422        (state.sqrtPriceX96, state.tick,,,,,) = state.pool.slot0();

The slot0 function returns various parameters of the pool, including the sqrtPriceX96 value and slot0 is the most recent data point and is therefore extremely easy to manipulate. More info on slot0 can be checked here

All of above use the function sqrtPriceX96 pulled from Uniswap.slot0. An attacker can simply manipulate the sqrtPriceX96 and if the Uniswap.swap function is called with the sqrtPriceX96, the token will be bought at a higher price and the attacker would run the transaction to sell; thereby earning gains but causing a loss of funds to whoever called those functions.

Tools Used

Manual review

Recommendation

Use TWAP instead of slot0

Assessed type

Uniswap

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/utils/Swapper.sol#L161

Vulnerability details

Summary

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.

Impact

1. The pool address check in the the swap callback function isn't strict enough and can suffer issues with collision.
2. Address collision can cause all allowances to be drained.
3. 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.

Proof of Concept

Before going in details, This issue is referenced from this issue which was also found in Kyberswap audit 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.

Swapper.sol is an abstract contract which acts as base contract for FlashloanLiquidator.sol, V3Utils.sol, LeverageTransformer.sol, Automator.sol, AutoRange.sol and so on.

uniswapV3SwapCallback() is an external function and anyone can call it.

uniswapV3SwapCallback() calls _getPool() to check the call is really from pool.

    function uniswapV3SwapCallback(int256 amount0Delta, int256 amount1Delta, bytes calldata data) external override {
        require(amount0Delta > 0 || amount1Delta > 0); // swaps entirely within 0-liquidity regions are not supported

        // check if really called from pool
        (address tokenIn, address tokenOut, uint24 fee) = abi.decode(data, (address, address, uint24));
@>      if (address(_getPool(tokenIn, tokenOut, fee)) != msg.sender) {
            revert Unauthorized();
        }

        // transfer needed amount of tokenIn
        uint256 amountToPay = amount0Delta > 0 ? uint256(amount0Delta) : uint256(amount1Delta);
        SafeERC20.safeTransfer(IERC20(tokenIn), msg.sender, amountToPay);
 }

_getPool() is given as,

    function _getPool(address tokenA, address tokenB, uint24 fee) internal view returns (IUniswapV3Pool) {
@>      return IUniswapV3Pool(PoolAddress.computeAddress(address(factory), PoolAddress.getPoolKey(tokenA, tokenB, fee)));
    }

The _getPool() used in uniswapV3SwapCallback() 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().

According to the UniswapV3 Doc,

when using uniswapV3SwapCallback, the caller of this method must be verified to be a UniswapV3Pool deployed by the canonical UniswapV3Factory.

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 kyberswap issue discussion further proves the attack is possible.

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

Tools Used

Manual review

Recommended Mitigation Steps

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

Assessed type

Access Control