Asymmetry contest - wen's results

Lines of code

https://github.com/NaryaAI/2023-03-c4-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L240

Vulnerability details

Impact

The price of Reth in ETH can be manipulated by flashloan. An attacker can exploit this fact to steal funds from other stakers.

Proof of Concept

https://github.com/NaryaAI/2023-03-c4-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L240

    /**
        @notice - Price of derivative in liquidity pool
     */
    function poolPrice() private view returns (uint256) {
        address rocketTokenRETHAddress = RocketStorageInterface(
            ROCKET_STORAGE_ADDRESS
        ).getAddress(
                keccak256(
                    abi.encodePacked("contract.address", "rocketTokenRETH")
                )
            );
        IUniswapV3Factory factory = IUniswapV3Factory(UNI_V3_FACTORY);
        IUniswapV3Pool pool = IUniswapV3Pool(
            factory.getPool(rocketTokenRETHAddress, W_ETH_ADDRESS, 500)
        );
        (uint160 sqrtPriceX96, , , , , , ) = pool.slot0(); // (1)
        return (sqrtPriceX96 * (uint(sqrtPriceX96)) * (1e18)) >> (96 * 2);
    }

At (1), the function fetches the slot0.sqrtPriceX96 of the pool which shows the current price without any historical price information.

This value can be manipulated by an attacker through flashloan. For example, an attacker can (1) Use flashloan to get a large amount of WETH from any pool. (2) Add liquidity to the rETH-WTH pool at a large tick range (e.g., 29000 ~ 29010) (3) Swap all the WETH through the rETH-WTH pool Then the current price of rETH in the pool will be dramatically increased, which will affect the stake() function (if totalSupply is 0, the attacker can gain many more safETH than being supposed to).

Tools Used

Recommended Mitigation Steps

Use TWAP instead of the current price (see https://docs.uniswap.org/contracts/v3/reference/core/libraries/Oracle).

1. Unnecessary loop in addDerivative()

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L182

addDerivative() function doesn’t need to have a for loop for updating totalWeight. totalWeight could just be increased by _weight.

2. Unnecessary loop in rebalanceToWeights()

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L138

In the rebalanceToWeights() function, the second loop checks ethAmountToRebalance == 0 every time. If ethAmountToRebalance is 0, no need to have the second loop. So this check could be moved to before the second loop.

1. stake() function reads derivativeCount twice.

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L71 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L84

2. rebalanceToWeights() function reads derivativeCount twice.

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L140 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L147

3. addDerivative() function reads derivativeCount twice.

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L182

4. setPauseStaking() could emit local variable(_pause) instead of storage variable(pauseStaking).

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L234

5. setPauseUnstaking() could emit local variable(_pause) instead of storage variable(pauseUnstaking).

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L243