Asymmetry contest - HHK's results

Lines of code

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

Vulnerability details

Impact

rETH withdraw may revert if not enough ETH are available in the deposit pool making the unstaking impossible.

Proof of Concept

When staking for rETH, ETH are sent to the deposit pool waiting to be picked for new miniPool validators.

This pool can be empty or not have enough ETH than what's being asked for when calling burn.

In this case the burn function reverts.

rETH code:

// Burn rETH for ETH
    function burn(uint256 _rethAmount) override external {
        // Check rETH amount
        require(_rethAmount > 0, "Invalid token burn amount");
        require(balanceOf(msg.sender) >= _rethAmount, "Insufficient rETH balance");
        // Get ETH amount
        uint256 ethAmount = getEthValue(_rethAmount);
        // Get & check ETH balance
        uint256 ethBalance = getTotalCollateral();
        require(ethBalance >= ethAmount, "Insufficient ETH balance for exchange");
        // Update balance & supply
        _burn(msg.sender, _rethAmount);
        // Withdraw ETH from deposit pool if required
        withdrawDepositCollateral(ethAmount);
        // Transfer ETH to sender
        msg.sender.transfer(ethAmount);
        // Emit tokens burned event
        emit TokensBurned(msg.sender, _rethAmount, ethAmount, block.timestamp);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding a way to sell on the market like wsteth and sfrxeth derivatives do.

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L63-L101 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L228-L245 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L170-L183 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L56-L66 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L108-L128 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L74-L75

Vulnerability details

Impact

rETH derivative can be bought through uniswap if the deposit contract is not open.

While a maxSlippage variable is set, the price of rETH on uniswap is the spot price and is only determined during the transaction opening sandwich opportunity for MEV researchers as long as the slippage stays below maxSlippage.

This is also true for WstETH (on withdraw) and frxETH (on deposit and withdraw) that go through a curve pool when unstaking (and staking for frxETH). While the curve pool has much more liquidity and the assumed price is a 1 - 1 ratio for WstETH and frxETH seem to be using a twap price before applying the slippage, these attacks are less likely to happen so I will only describe rETH.

Proof of Concept

While the current rETH derivative contract uses uniswapv3 0,05% pool, I'll be using the uniswapv2 formula (https://amm-calculator.vercel.app/) to make this example simplier, in both case sandwiching is possible.

Default slippage is set to 1% on rETH contract at deployment. (see: https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L44)

Let's take a pool of 10,000 rETH for 10,695 eth (same ratio is on the univ3 0,05% pool on the 26th of march).

User wants to stake 100ETH, a third of it will be staked through rETH according to a 1/3 weight for each derivative.

Bundle:

TX1:

Researcher swap 100 ETH in for 92.63 rETH new pool balance: 9907.36 rETH - 10795 ETH

TX2:

User stake his ETH, the rocketPool deposit contract is close so the deposit function takes the current spot price of the pool and then applies 1% slippage to it to get minOut.

Current ratio: eth = 0.9177 rETH ETH to swap for reth: 33.3333~ So minOut -> 33.3333 * 0.9177 * 0.99 = 30.284 rETH

(see: https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L170-L183)

Contract swap 33.3333 ETH for 30.498 rETH (slippage of 0.61% so below 1% and received more than minOut)

New pool balance: 9876.86 rETH - 10828.33 ETH

TX3:

Researcher swap back the 92.63 rETH in for 100.61~ ETH new pool balance: 9969.49 rETH - 10727.72 ETH

Researcher made 0.61~ ETH of profit, could be more as we only applied a 0,61% slippage but we can go as far as 1% in the current rETH contract.

Univ3 pool would could even worse as Researcher with a lot of liquidity could be able to drain one side (liquidity is very concentrated), add liquidity in a tight range execute the stake and then remove liquidity and swap back.

Tools Used

Manual review + https://amm-calculator.vercel.app/

Recommended Mitigation Steps

The rETH price should be determined using the TWAP price and users should be able to input minOut in the stake, unstake and rebalanceToWeight function.

Custom errors instead of requires

require(pauseStaking == false, "staking is paused"); if(pauseStaking == false) revert StakingPaused();

In SafETH Stake() can have everything in the same loop.

Line 88 of SafEth.sol, if (weight == 0) continue should be one line above

This would save a sload as we wouldn't need to load IDerivative derivative = derivatives[i]; when weight == 0

All loops can move i++ in unckecked tags

Using a state variable in for condition and reading state variable multiple time should be avoided

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

Declaring a memory d = derivativeCount and then using it in the loop would save gas as each loop will mload instead of sload. Also because derivativeCount is used 2 times in this function it would save an extra sload to declare d at the beginning.

rETH ethPerDerivative() does a useless multiplication and division

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

Multiply by 1e18 and then instantly divide by 1e18 is a waste of gas.

In SafETH addDerivative() localTotalWeight should be calculated at the beginning

Calculate localTotalWeight first and then add _weight to get the new totalWeight, this will save one loop and one sload.

In SafETH rebalanceToWeights() the check ethAmountToRebalance == 0 is useless

It should be checked before the loop and return or not checked at all since there is low chance this function gets called if there is no eth to rebalance.