Asymmetry contest - 0x52's results

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L228-L242

Vulnerability details

Impact

Reth#poolPrice will revert at higher sqrtPriceX96 values causing complete DOS of withdrawals and deposits at that price

Proof of Concept

Reth.sol#L228-L242

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();
    return (sqrtPriceX96 * (uint(sqrtPriceX96)) * (1e18)) >> (96 * 2);
}

sqrtPriceX96 is the square root of the price shifted 96 bits. When this value is squared it results in the actual price bit shifted 192 bits. This value is multiplied by 1e18 which is a 60 bit value. The result is that the the calculation will result in an overflow if the value is greater than ~16 (256 - 192 - 60 = 4 -> 2^4 = 16). This checks out as the overflow happens when sqrtPriceX96 = 3.41e29 which corresponds to a price of ~18.4. Considering the rate of accumulation for rETH this isn't a price it would realistically reach. The problem with this is that it provides a method by which an adversary could maliciously DOS another user's withdraw or deposit via a sandwich attack.

Tools Used

Manual Review

Recommended Mitigation Steps

Import and use the UniV3 math lib which is specifically designed to handle internal overflows

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L107-L114

Vulnerability details

Impact

Low eth balances in rETH will DOS withdrawals

Proof of Concept

Reth.sol#L107-L114

function withdraw(uint256 amount) external onlyOwner {
    RocketTokenRETHInterface(rethAddress()).burn(amount);
    // solhint-disable-next-line
    (bool sent, ) = address(msg.sender).call{value: address(this).balance}(
        ""
    );
    require(sent, "Failed to send Ether");
}

When withdrawing rETH, it calls the burn method on the rETH contract. This will only succeed if the contract currently has enough ETH to process the withdraw. Historically this value is extremely low and would not be able to service many withdrawals. This will remain problematic even after the Shanghai updates enables withdrawals from validators because an adversary could easily DOS a withdraw by front-running the withdraw request that would claim enough of the balance to cause the call to revert.

Tools Used

Manual Review

Recommended Mitigation Steps

Reth#withdraw should burn as much as it can and swap the rest

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L228-L242

Vulnerability details

Impact

Adversary can gain unfair share of pool after staking

Proof of Concept

Reth.sol#L228-L242

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(); <- @audit-issue slot0 is easily manipulated
    return (sqrtPriceX96 * (uint(sqrtPriceX96)) * (1e18)) >> (96 * 2);
}

When calculating the price of the rETH, Reth#ethPerDerivative uses poolPrice to calculate the price ETH price of rETH. The issues lies in Reth#poolPrice, which uses the sqrtPriceX96 to get the current price of the pool. This is problematic as outlined below:

UniswapV3Pool.sol#L733-L752

    if (state.tick != slot0Start.tick) {
        (uint16 observationIndex, uint16 observationCardinality) =
            observations.write(
                slot0Start.observationIndex,
                cache.blockTimestamp,
                slot0Start.tick,
                cache.liquidityStart,
                slot0Start.observationCardinality,
                slot0Start.observationCardinalityNext
            );
        (slot0.sqrtPriceX96, slot0.tick, slot0.observationIndex, slot0.observationCardinality) = (
            state.sqrtPriceX96,
            state.tick,
            observationIndex,
            observationCardinality
        );
    } else {
        // otherwise just update the price
        slot0.sqrtPriceX96 = state.sqrtPriceX96; <- @audit-issue slot0.sqrtPriceX96 updated with every swap
    }

With each swap slot0.sqrtPriceX96 is updated. This means that the price can be manipulated on an instantaneous basis. This creates huge issues especially because of how the deposits are valued in the flow of SafEth#stake.

SafEth.sol#L71-L96

    for (uint i = 0; i < derivativeCount; i++)
        underlyingValue +=
            (derivatives[i].ethPerDerivative(derivatives[i].balance()) *
                derivatives[i].balance()) /
            10 ** 18;

    uint256 totalSupply = totalSupply();
    uint256 preDepositPrice; // Price of safETH in regards to ETH
    if (totalSupply == 0)
        preDepositPrice = 10 ** 18; // initializes with a price of 1
    else preDepositPrice = (10 ** 18 * underlyingValue) / totalSupply;

    uint256 totalStakeValueEth = 0; // total amount of derivatives worth of ETH in system
    for (uint i = 0; i < derivativeCount; i++) {
        uint256 weight = weights[i];
        IDerivative derivative = derivatives[i];
        if (weight == 0) continue;
        uint256 ethAmount = (msg.value * weight) / totalWeight;

        // This is slightly less than ethAmount because slippage
        uint256 depositAmount = derivative.deposit{value: ethAmount}();
        uint derivativeReceivedEthValue = (derivative.ethPerDerivative(
            depositAmount
        ) * depositAmount) / 10 ** 18;
        totalStakeValueEth += derivativeReceivedEthValue;
    }
    uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;

Looking at the function above we can see how the flow of staking goes:

1) Total value of all derivatives is determined
2) Total value used to calculate the share value
3) Each derivative is deposited and value AFTER deposit
4) Amount to mint is determined by using share value calculated before deposit

This flow in combination with the user of slot0 in Reth#getPrice leaves a hole that can be exploited. The flow of the attack would goes as follows:

1) Sell rETH on UniV3. Since it uses slot0 to value the rETH, it will be valued lower than expected
2) Call SafEth#stake. This will cause the pool to buy rETH which will increase the price of rETH. Now all the rETH bought during stake will be valued higher than it was before the swap

This allows the adversary to gain more stake than expected.

Example: For simplicity assume that rETH, wstETH and sfrxETH are all worth 1 ETH and the composition of safETH is 100 rETH 100 wstETH and 100 sfrxETH with 100 shares. Before the swap, the malicious user lowers the price to 0.99 ETH by selling rETH. When the shares are valued they will be valued at 2.99 (100 * 1 + 100 * 1 + 100 * 0.99). Now the adversary stakes some amount of ETH which will result in the price of rETH being returned to 1 after the swap. Now post swap each rETH will be valued at 1 whereas before the swap they were valued each at 0.99. The result is that the adversary can profit off of this difference to get more shares than expected. The default slippage is 1% which is why I use 0.99 in my example but the larger the allowed slippage the worse this issue gets.

Tools Used

Manual Review

Recommended Mitigation Steps

For Reth#getPrice use a TWAP instead of slot0.

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L83-L102

Vulnerability details

Impact

Users will suffer greater slippage and swap fees

Proof of Concept

Reth.sol#L83-L102

function swapExactInputSingleHop(
    address _tokenIn,
    address _tokenOut,
    uint24 _poolFee,
    uint256 _amountIn,
    uint256 _minOut
) private returns (uint256 amountOut) {
    IERC20(_tokenIn).approve(UNISWAP_ROUTER, _amountIn);
    ISwapRouter.ExactInputSingleParams memory params = ISwapRouter
        .ExactInputSingleParams({
            tokenIn: _tokenIn,
            tokenOut: _tokenOut,
            fee: _poolFee,
            recipient: address(this),
            amountIn: _amountIn,
            amountOutMinimum: _minOut,
            sqrtPriceLimitX96: 0
        });
    amountOut = ISwapRouter(UNISWAP_ROUTER).exactInputSingle(params);
}

When making swaps, Reth.sol uses the Uniswap 0.05% pool to swap the ETH to rETH. This results in both higher slippage and fees for users meaning they net less than rETH than they otherwise could.

Uniswap -> 1524 rETH 0.05% fee

Balancer -> 19545 rETH 0.04% fee

The Balancer pool offers ~14x more liquidity with a 20% lower swap fee, which makes it a much better choice.

Tools Used

Manual Review

Recommended Mitigation Steps

Use Balancer instead of UniV3