Asymmetry contest - SaeedAlipoor01988's results

Lines of code

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

Vulnerability details

Impact

Based on the current implmention of Reth.sol#L228.poolPrice() function,

As the code extracts the active sqrtPriceX96 instead of the price associated with the active tick, it is possible to manipulate the price trivially in one direction or the other within the tick which may also bypass secondary protection measures such as TWAPs based on ticks.

the code contains no protection against an artificial price and users are able to skew the price of the UniswapV3 pool.

Proof of Concept

The sqrtPriceX96 that is stored within slot0 of a Uniswap V3 pool is not strictly associated with the current tick of the pool as a tick is valid for a range of prices; the square ratio generated at a particular tick, however, is associated in a one-to-one fashion with it.

as well as the Reth.sol#L228.poolPrice() function utilizes the current price of the UniswapV3 pool which is susceptible to price manipulations and Flash-Loan price manipulation , permitting its users to artificially manipulation the asset prices.

//(uint160 sqrtPriceX96, , , , , , ) = pool.slot0();

Tools Used

Manually

Recommended Mitigation Steps

Change the code to be revamped to utilize the current tick rather than the actual sqrtPriceX96, ensuring that the code is not susceptible to in-tick price manipulation attacks and add TWAP mechanism or similar protection measure as it is presently insecure and can compromise user assets.

Lines of code

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

Vulnerability details

Impact

The transaction for Swap tokens through Uniswap lack of expiration timestamp check, The transaction can be pending in mempool for a long time and can be executed for a long time after the user/contract submit the transaction.

Proof of Concept

This is Reth.sol#L83.swapExactInputSingleHop() function,

   // 
            ISwapRouter.ExactInputSingleParams memory params = ISwapRouter
        .ExactInputSingleParams({
            tokenIn: _tokenIn,
            tokenOut: _tokenOut,
            fee: _poolFee,
            recipient: address(this),
            amountIn: _amountIn,
            amountOutMinimum: _minOut,
            sqrtPriceLimitX96: 0
        });

There is not any parameter for a deadline or expiration timestamp check. But in uniswap, you can find

   // 
            modifier ensure(uint deadline) {
require(deadline >= block.timestamp, 'UniswapV2Router: EXPIRED');
_;

}

AMMs provide their users with an option to limit the execution of their pending actions, such as swaps or adding and removing liquidity. The most common solution is to include a deadline timestamp as a parameter (for example see Uniswap V2 and Uniswap V3). If such an option is not present, users can unknowingly perform bad trades:

Alice wants to swap 100 tokens for 1 ETH and later sell the 1 ETH for 1000 DAI. The transaction is submitted to the mempool, however, Alice chose a transaction fee that is too low for miners to be interested in including her transaction in a block. The transaction stays pending in the mempool for extended periods, which could be hours, days, weeks, or even longer.

When the average gas fee dropped far enough for Alice's transaction to become interesting again for miners to include it, her swap will be executed. In the meantime, the price of ETH could have drastically changed. She will still get 1 ETH but the DAI value of that output might be significantly lower. She has unknowingly performed a bad trade due to the pending transaction she forgot about.

An even worse way this issue can be maliciously exploited is through MEV:

The swap transaction is still pending in the mempool. Average fees are still too high for miners to be interested in it. The price of tokens has gone up significantly since the transaction was signed, meaning Alice would receive a lot more ETH when the swap is executed. But that also means that her maximum slippage value (sqrtPriceLimitX96 and minOut in terms of the Papr contracts) is outdated and would allow for significant slippage.

An MEV bot detects the pending transaction. Since the outdated maximum slippage value now allows for high slippage, the bot sandwiches Alice, resulting in significant profit for the bot and significant loss for Alice.

Since Reth.sol#L156.deposit() function directly connected with Uniswap V3, such deadline parameters should also be offered to the ExactInputSingleParams.

Tools Used

Manually

Recommended Mitigation Steps

Introduce a deadline parameter to Reth.sol#L83.swapExactInputSingleHop() function which potentially perform a swap.

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L71 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L101 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L176 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L76 https://github.com/rocket-pool/rocketpool/blob/967e4d3c32721a84694921751920af313d1467af/contracts/contract/deposit/RocketDepositPool.sol#L77

Vulnerability details

Impact

The deposit function for Deposit into a derivative depends on the deposit function from external contracts. So, if the external function fails, the deposit function will fall after that. if the deposit function in one of staked derivatives contracts doesn’t work, the main deposit function in the SafEth.sol contract will get reverted because of for loop.

Proof of Concept

When the user wants to deposit ETH in the protocol, the SafEth.sol#L63.stake() function will run for loop over 3 staked derivatives contracts to make deposits in the external projects.

        // 
    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;
        uint256 depositAmount = derivative.deposit{value: ethAmount}();
        uint derivativeReceivedEthValue = (derivative.ethPerDerivative(
            depositAmount
        ) * depositAmount) / 10 ** 18;
        totalStakeValueEth += derivativeReceivedEthValue;
    }

So, if the external function fails, the deposit function will fall after that. if the deposit function in one of staked derivatives contracts doesn’t work, the main deposit function in the SafEth.sol contract will get reverted because of for loop.

https://i.ibb.co/zf7DBvC/canvas.png

let's check Reth.sol contract deposit function. the only check for before processing the deposit into RocketDepositPool, is Reth.sol#L120.poolCanDeposit function to check whether or not the rETH deposit pool has room users amount. but if you check RocketDepositPool.sol#L77 contract directly, there is another requirement check, to check Deposits into Rocket Pool are currently disabled or not !

        // 
            require(rocketDAOProtocolSettingsDeposit.getDepositEnabled(), "Deposits into Rocket Pool are currently disabled");

So if Deposits into Rocket Pool are currently disabled, the deposit transaction from Reth.sol#L156 contract will return an error, and then for loop in the SafEth.sol#L91 cannot get completed and there is not any code to continue the deposit in other staked derivatives contracts, and the deposit transaction from the user will get reverted.

Tools Used

Manually

Recommended Mitigation Steps

Use try/catch block. The logic for deposit should be placed in the try block, while some fallback logic when the deposit is not successful should be placed in the catch block.