Asymmetry contest - HollaDieWaldfee's results

Lines of code

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

Vulnerability details

Impact

The SafEth.rebalanceToWeights function is used to rebalance the derivatives held by the protocol to match the intended weights.

For example the protocol might hold 3 derivatives and there is an issue with one of them. A bug might exist in the protocol or it might have just become untrustworthy.

The issue is that the algorithm to rebalance the assets is very inefficient. This leads to a substantial loss for users of the protocol.

If the value of tokens held in the Asymmetry protocol is very large there might even be insufficient liquidity in the pools to withdraw all tokens at once. In this case it is not even possible to call the rebalanceToWeights function (which ironically might be favorable to users due to the large losses they suffer otherwise).

Proof of Concept

Let's first have a look at the rebalanceToWeights function:
Link

    function rebalanceToWeights() external onlyOwner {
        uint256 ethAmountBefore = address(this).balance;
        for (uint i = 0; i < derivativeCount; i++) {
            if (derivatives[i].balance() > 0)
                derivatives[i].withdraw(derivatives[i].balance());
        }
        uint256 ethAmountAfter = address(this).balance;
        uint256 ethAmountToRebalance = ethAmountAfter - ethAmountBefore;


        for (uint i = 0; i < derivativeCount; i++) {
            if (weights[i] == 0 || ethAmountToRebalance == 0) continue;
            uint256 ethAmount = (ethAmountToRebalance * weights[i]) /
                totalWeight;
            // Price will change due to slippage
            derivatives[i].deposit{value: ethAmount}();
        }
        emit Rebalanced();
    }

First the function withdraws ALL derivatives and exchanges them for ETH:

for (uint i = 0; i < derivativeCount; i++) {
    if (derivatives[i].balance() > 0)
        derivatives[i].withdraw(derivatives[i].balance());
}

Then the ETH is deposited again according to the weights:

for (uint i = 0; i < derivativeCount; i++) {
    if (weights[i] == 0 || ethAmountToRebalance == 0) continue;
    uint256 ethAmount = (ethAmountToRebalance * weights[i]) /
        totalWeight;
    // Price will change due to slippage
    derivatives[i].deposit{value: ethAmount}();
}

We can easily see how this leads to an unnecessary loss of funds by considering a simple scenario:
Assume the weights look like this and the derivative balances curently match these weights:

derivative 1: 30
derivative 2: 30
derivative 3: 40

Then there is a bug in derivative 1 and we change the weights (substitute derivative 1 for derivative 4):

derivative 1: 0
derivative 2: 30
derivative 3: 40
derivative 4: 30

The efficient way to achieve this is (i.e. leading to the smallest loss of funds for users) by withdrawing only all of derivative 1 and then deposit this into derivative 4.

However as explained above, the current implementation will trade ALL the funds. Not just the 30% that it needs to.

The fees / slippage that leads to a loss of funds for users stems from multiple sources:

• deposit fees (e.g. Rocketpool Link)
• Curve + Uniswap fees (e.g. Link,Link)
• Slippage from trading

This is especially concerning since ALL funds need to be traded. So the value can possibly be huge.

Tools Used

VSCode

Recommended Mitigation Steps

We have seen that the current implementation incurs completely unnecessary losses for users when doing a rebalance.

The alogrithm that I propose should execute the following steps:

1. Calculate the ETH value that is held in each derivative and the total ETH held in the protocol
2. Calculate how much ETH should be held in each derivative (based on the new weights and the total ETH held in the protocol)
3. For each derivative withdraw the difference between ETH held in derivative - ETH supposed to be held in derivative
4. Divide the withdrawn ETH across the derivatives that we need to hold more of

This algorithm avoids double trading and ensures that fees + slippage are as small as possible.

By using this new algorithm, a greater weight adjustment can be performed in multiple smaller steps such that each call to rebalanceToWeights encounters sufficient liquidity in the pools.

Lines of code

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

Vulnerability details

Impact

The calculation in the SfrxEth.ethPerDerivative function is wrong.

It is assumed that the price_oracle function of the Curve pool returns the price with frxETH / ETH as the unit. However the unit is ETH / frxETH. Depending on how much the exchange rate deviates from parity (i.e. 1), the impact that this issue has varies.

The protocol will value the sfrxETH derivative token incorrectly. The valuation can either be too low or too high. This leads to accounting errors when calling the SafEth.stake function which is where the ethPerDerivative function is used.

This means that the amount of SafETH that the user is minted is either too low or too high.

I estimate this to be of "Medium" severity because the ETH / frxETH price should be close to 1 so the deviation is not too big.
But it definitely leads to small errors.

Apart from this issue, the ETH / frxETH exchange rate can just be assumed to be 1.
This is because according to Frax Finance 1 frxETH always represents 1 ETH in the Frax system:

Link

frxETH acts as a stablecoin loosely pegged to ETH, so that 1 frxETH always represents 1 ETH and the amount of frxETH in circulation matches the amount of ETH in the Frax ETH system. When ETH is sent to the frxETHMinter, an equivalent amount of frxETH is minted. Holding frxETH on its own is not eligible for staking yield and should be thought of as analogous as holding ETH.

This makes the relationship between frxETH and ETH similar to that between stETH and ETH from Lido.

In the case of stETH and ETH the protocol treats them like they always have a 1:1 exchange rate (Link)

It is an intended property of the protocol to treat the tokens from derivative protocols that are backed 1:1 by ETH as equal to ETH, i.e. to assume a 1:1 exchange rate.

Proof of Concept

Let's have a look at the value returned from the price_oracle function from the Curve pool.
You can find the current value here (https://etherscan.io/address/0xa1F8A6807c402E4A15ef4EBa36528A3FED24E577/#readContract)

At the point I took the screenshot the price_oracle result was 998607137138305022.

When looking at the Curve front end (Link) we can see that for 1 frxETH we get ~ 0.998 ETH:

So the unit is 0.998 ETH / frxETH.

However the ethPerDerivative function (Link) performs the following calculation:

return ((10 ** 18 * frxAmount) /
    IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle());

So say frxAmount is 1e18 then with the above price we get a result that is >1e18.

Obviously this is wrong. For 1 frxETH we should get less than 1 ETH (i.e. <1e18).

We can also see this issue by looking at the units of the calculation: frxETH / (ETH / frxETH) = frxETH * (frxETH / ETH) = frxETH^2 / ETH. This is obviously wrong.

The correct calculation would be (IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle() * frxAmount) / 1e18.

But as explained above we don't even need this calculation. It should be assumed that the exchange rate is 1 since 1 frxETH represents 1 ETH in the Frax system.

Note:
The result of the ethPerDerivative function is actually also used in the SfrxEth.withdraw function (Link) where the wrong result can lead to an unexpected calculation of the allowed slippage.

Tools Used

VSCode

Recommended Mitigation Steps

Fix:

diff --git a/contracts/SafEth/derivatives/SfrxEth.sol b/contracts/SafEth/derivatives/SfrxEth.sol
index 98a89bb..8725129 100644
--- a/contracts/SafEth/derivatives/SfrxEth.sol
+++ b/contracts/SafEth/derivatives/SfrxEth.sol
@@ -112,8 +112,7 @@ contract SfrxEth is IDerivative, Initializable, OwnableUpgradeable {
         uint256 frxAmount = IsFrxEth(SFRX_ETH_ADDRESS).convertToAssets(
             10 ** 18
         );
-        return ((10 ** 18 * frxAmount) /
-            IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle());
+        return frxAmount;
     

Lines of code

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

Vulnerability details

Impact

(Note: This report only deals with the WstEth contract. The slippage calculations in SfrxEth and Reth are generally correct. There exists an issue with the price in SfrxEth (wrong units are used) which ends up affecting the slippage calculation as well but I have detailed this in another issue I submitted (#141).)

When the WstEth.withdraw function (Link) is called, wstETH is unwrapped and the resulting stETH is swapped for ETH in a Curve pool.

There is an issue with how minOut is calculated which leads to insufficient slippage protection in some cases and in other cases impacts the availability of the withdraw function.

Proof of Concept

Let's have a look at how minOut is calculated:

Link

uint256 minOut = (stEthBal * (10 ** 18 - maxSlippage)) / 10 ** 18;

mintOut is an amount in ETH since stETH is exchanged for ETH.
It is assumed that 1 stETH = 1 ETH. While this is reasonable to assume for calculating how many SafETH token a user gets minted this leads to issues in the slippage calculation.

For slippage protection it is important that the actual price is used:

uint256 minOut = ((ETH per stETH * stEthBal / 10**18) * (10 ** 18 - maxSlippage)) / 10 ** 18;

With ETH per stETH being how much ETH one stETH is worth.

But why does the current code cause issues?

We can see in the following image that the price fluctuates:

Link

When the price drops, i.e. 1 stETH becomes worth less ETH, then the current calculation results in a minOut amount that is too big (because it is assumed that 1 stETH can buy 1 ETH). When the price drops enough (e.g. from parity to 1 stETH can buy 0.98 ETH) and slippage protection is set to 1 percent, minOut is calculated as 0.99 even though we can only buy 0.98 (even without price impact). So this leads to an availability issue. The swap fails.

On the other hand say 1 stETH can buy 0.98 ETH and the slippage percentage has been changed to 3 percent by the owner such that it is 1 percent below the price (where the owner intends it to be).

When the price increases say to parity, i.e. 1 stETH can buy 1 ETH, the minOut is still calculated to be 0.97 ETH which allows for a too big slippage (1 percent intended vs. 3 percent). So in the case of an increase in price there is insufficient slippage protection.

In summary I have shown how a decrease in price can lead to availability problems and an increase leads to insufficient slippage protection.

This should be mitigated by incorporating the price into the calculation of minOut.

Tools Used

VSCode

Recommended Mitigation Steps

The minOut amount must be calculated based on the current price in the Curve pool.

You can use the get_dy function from the Curve pool to calculate the current price:

Lines of code

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

Vulnerability details

Impact

The Asymmetry protocol promises that a user can call SafETH.unstake at all times. What I mean by that is that a user should be able at all times to burn his SafETH tokens and receive ETH in return. This requires that the derivatives held by the protocol can at all times be withdrawn (i.e. converted to ETH).

Also the rebalanceToWeights functionality requires that the derivatives can be withdrawn at all times. If a derivative cannot be withdrawn then the rebalanceToWeights function cannot be executed which means that the protocol cannot be adjusted to use different derivatives.

For the WstEth and SfrxEth derivatives this is achieved by swapping the derivative in a Curve pool for ETH. The liquidity in the respective Curve pool ensures that withdrawals can be processed at all times.

The Reth derivative works differently.
Withdrawals are made by calling the RocketTokenRETH.burn function:

Link

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

The issue with this is that the RocketTokenRETH.burn function only allows for excess balance to be withdrawn. I.e. ETH that has been deposited by stakers but that is not yet staked on the Ethereum beacon chain. So Rocketpool allows users to burn rETH and withdraw ETH as long as the excess balance is sufficient.

The issue is obvious now: If there is no excess balance because enough users burn rETH or the Minipool capacity increases, the Asymmetry protocol is bascially unable to operate.

Withdrawals are then impossible which bricks SafEth.unstake and SafEth.rebalanceToWeights.

Proof of Concept

I show in this section how the current withdrawal flow for the Reth derivative is dependend on there being excess balance in the RocketDepositPool.

The current withdrawal flow calls RocketTokenRETH.burn which executes this code:

Link

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);
}

This executes withdrawDepositCollateral(ethAmount):

Link

function withdrawDepositCollateral(uint256 _ethRequired) private {
    // Check rETH contract balance
    uint256 ethBalance = address(this).balance;
    if (ethBalance >= _ethRequired) { return; }
    // Withdraw
    RocketDepositPoolInterface rocketDepositPool = RocketDepositPoolInterface(getContractAddress("rocketDepositPool"));
    rocketDepositPool.withdrawExcessBalance(_ethRequired.sub(ethBalance));
}

This then calls rocketDepositPool.withdrawExcessBalance(_ethRequired.sub(ethBalance)) to get the ETH from the excess balance:

Link

function withdrawExcessBalance(uint256 _amount) override external onlyThisLatestContract onlyLatestContract("rocketTokenRETH", msg.sender) {
    // Load contracts
    RocketTokenRETHInterface rocketTokenRETH = RocketTokenRETHInterface(getContractAddress("rocketTokenRETH"));
    RocketVaultInterface rocketVault = RocketVaultInterface(getContractAddress("rocketVault"));
    // Check amount
    require(_amount <= getExcessBalance(), "Insufficient excess balance for withdrawal");
    // Withdraw ETH from vault
    rocketVault.withdrawEther(_amount);
    // Transfer to rETH contract
    rocketTokenRETH.depositExcess{value: _amount}();
    // Emit excess withdrawn event
    emit ExcessWithdrawn(msg.sender, _amount, block.timestamp);
}

And this function reverts if the excess balance is insufficient which you can see in the require(_amount <= getExcessBalance(), "Insufficient excess balance for withdrawal"); check.

Tools Used

VSCode

Recommended Mitigation Steps

The solution for this issue is to have an alternative withdrawal mechanism in case the excess balance in the RocketDepositPool is insufficient to handle the withdrawal.

The alternative withdrawal mechanism is to sell the rETH tokens via the Uniswap pool.

You can use the RocketDepositPool.getExcessBalance to check if there is sufficient excess ETH to withdraw from Rocketpool or if the withdrawal must be made via Uniswap.

The pseudocode of the new withdraw flow looks like this:

function withdraw(uint256 amount) external onlyOwner {
    if (rocketDepositPool excess balance is sufficient) {
        RocketTokenRETHInterface(rethAddress()).burn(amount);
        // solhint-disable-next-line
        (bool sent, ) = address(msg.sender).call{value: address(this).balance}(
            ""
        );
        require(sent, "Failed to send Ether");
    } else {
        // swap rETH for ETH via Uniswap pool
    }
}

I also wrote the code for the changes that I suggest:

diff --git a/contracts/SafEth/derivatives/Reth.sol b/contracts/SafEth/derivatives/Reth.sol
index b6e0694..b699d5c 100644
--- a/contracts/SafEth/derivatives/Reth.sol
+++ b/contracts/SafEth/derivatives/Reth.sol
@@ -105,11 +105,24 @@ contract Reth is IDerivative, Initializable, OwnableUpgradeable {
         @notice - Convert derivative into ETH
      */
     function withdraw(uint256 amount) external onlyOwner {
-        RocketTokenRETHInterface(rethAddress()).burn(amount);
-        // solhint-disable-next-line
-        (bool sent, ) = address(msg.sender).call{value: address(this).balance}(
-            ""
-        );
+        if (canWithdrawFromRocketPool(amount)) {
+            RocketTokenRETHInterface(rethAddress()).burn(amount);
+            // solhint-disable-next-line
+        } else {
+
+            uint256 minOut = ((((poolPrice() * amount) / 10 ** 18) *
+                ((10 ** 18 - maxSlippage))) / 10 ** 18);
+
+            IWETH(W_ETH_ADDRESS).deposit{value: msg.value}();
+            swapExactInputSingleHop(
+                rethAddress(),
+                W_ETH_ADDRESS,
+                500,
+                amount,
+                minOut
+            );
+        }
+        (bool sent, ) = address(msg.sender).call{value: address(this).balance}("");
         require(sent, "Failed to send Ether");
     }
 
@@ -149,6 +162,21 @@ contract Reth is IDerivative, Initializable, OwnableUpgradeable {
             _amount >= rocketDAOProtocolSettingsDeposit.getMinimumDeposit();
     }
 
+    function canWithdrawFromRocketPool(uint256 _amount) private view returns (bool) {
+        address rocketDepositPoolAddress = RocketStorageInterface(
+            ROCKET_STORAGE_ADDRESS
+        ).getAddress(
+                keccak256(
+                    abi.encodePacked("contract.address", "rocketDepositPool")
+                )
+            );
+        RocketDepositPoolInterface rocketDepositPool = RocketDepositPoolInterface(
+                rocketDepositPoolAddress
+            );
+        uint256 _ethAmount = RocketTokenRETHInterface(rethAddress()).getEthValue(_amount);
+        return rocketDepositPool.getExcessBalance() >= _ethAmount;
+    }
+

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-L242 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L211-L216

Vulnerability details

Impact

The SafEth.stake function makes use of the ethPerDerivative function of the derivative contracts.

First in order to calculate the value for each of the derivatives already held by the protocol:

Link

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

And then to calculate the value of the newly staked derivatives:

Link

uint derivativeReceivedEthValue = (derivative.ethPerDerivative(
    depositAmount
) * depositAmount) / 10 ** 18;
totalStakeValueEth += derivativeReceivedEthValue;

Each derivative contract has a slighlty different implementation of the ethPerDerivative function.

There is an issue with this implementation in the Reth derivative.

The Reth derivative uses the plain Uniswap spot price derived from sqrtPriceX96 (Link) to value the derivative tokens (Link). The spot price is prone to manipulations e.g. via a flash loan. An attacker could manipulate the price prior to interacting with Asymmetry to mint an unfair amount of SafEth token, thereby gaining an unfair share of the underlying derivative tokens.

(Note: The SfrxEth derivative also makes use of a price from a DEX, in this case Curve, to value the frxETH tokens. However the function that is used calculates that price with an Exponential Moving Average. So this price is less prone to manipulation)

Proof of Concept

I will provide a scenario that is highly prone to price manipulation:

Let's say we currently have 3 derivatives (one of them is rETH) with the following weights:

D1, weight=100
D2, weight=100
rETH, weight=50

Further assume that we currently have the following ETH values (price without manipulation):

D1, 100ETH
D2, 100ETH
rETH, 50ETH

And say we have 250 safETH minted (to simplify the preDepositPrice calculation).
This means the preDepositPrice is:

(100 ETH + 100 ETH + 50 ETH) / 250 safETH = 1 ETH per safETH

Now let's say the weights are adjusted to the following:

D1, weight=50
D2, weight=100
rETH, weight=100

Assume all 3 derivatives have a 1:1 price to Ethereum.
Without price manipulation when we stake 60 ETH we get the following amount of safETH:

preDepositPrice = 1 ETH per safETH
totalStakeValueEth = 60 ETH
mintAmount = 60 ETH / (1 ETH per safETH) = 60 safETH

Now we perform the same stake but this time with manipulating the price of rETH 5% up (i.e. rETH is now worth 5% more than what it should be).
The following calculations apply:

derivative values:
D1, 100ETH
D2, 100ETH
rETH, 52.5ETH

preDepositPrice = 252 ETH / 250 safETH = 1.008 ETH per safETH

totalStakeValueEth = 61.2 ETH (12 ETH + 24 ETH + 25.2 ETH)

mintAmount = 61.2 ETH / (1.008 ETH per safETH) = 62.5 safETH

By manipulating the price we were able to receive 60.7 safETH which is ~1.2% more than what we should have received.

This means we have gained an unfair share of the underlying derivative tokens of the Asymmetry protocol and the other users have lost.

The issue becomes more pronounced if for example the weight of rETH has been 0 previously or when the weight of rETH is not set to 100 but instead to a higher number.

Tools Used

VSCode

Recommended Mitigation Steps

While it makes sense to use the Uniswap spot price for the calculation of minOut (aka to provide slippage protection), for valuation of the rETH tokens a TWAP price should be used.

You can implement a new function that gets the TWAP price from the Uniswap pool.

Have a look at the following article for how to implement such a function:

https://tienshaoku.medium.com/a-guide-on-uniswap-v3-twap-oracle-2aa74a4a97c5

function getSqrtTwapX96(address uniswapV3Pool, uint32 twapInterval) public view returns (uint160 sqrtPriceX96) {
    if (twapInterval == 0) {
        // return the current price if twapInterval == 0
        (sqrtPriceX96, , , , , , ) = IUniswapV3Pool(uniswapV3Pool).slot0();
    } else {
        uint32[] memory secondsAgos = new uint32[](2);
        secondsAgos[0] = twapInterval; // from (before)
        secondsAgos[1] = 0; // to (now)

        (int56[] memory tickCumulatives, ) = IUniswapV3Pool(uniswapV3Pool).observe(secondsAgos);

        // tick(imprecise as it's an integer) to price
        sqrtPriceX96 = TickMath.getSqrtRatioAtTick(
            int24((tickCumulatives[1] - tickCumulatives[0]) / twapInterval)
        );
    }
}

I recommend a twapInterval of a few minutes (10-15 minutes) to be resistant to manipulation and also quick to react to changes in the price.

Lines of code

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

Vulnerability details

Impact

The Reth derivative contract handles deposits by checking if ETH can be deposited directly to the RocketDepositPool. If this is not possible, deposits are handled by swapping ETH for rETH on Uniswap.

The checks whether a deposit to the RocketDepositPool is possible are implemented in the `Reth.poolCanDeposit function.

Essentially there are two checks:

1. Maximum pool size not reached
2. Deposit must at least be the minimum deposit amount

The issue is that there is a check missing: That the RocketDepositPool has deposits enabled.

This can cause the SafEth.stake function to be blocked so users cannot stake anymore and thereby cannot enter the Asymmetry protocol.

Proof of Concept

Let's have a look at the RocketDepositPool.deposit function:

Link

function deposit() override external payable onlyThisLatestContract {
    // Check deposit settings
    RocketDAOProtocolSettingsDepositInterface rocketDAOProtocolSettingsDeposit = RocketDAOProtocolSettingsDepositInterface(getContractAddress("rocketDAOProtocolSettingsDeposit"));
    require(rocketDAOProtocolSettingsDeposit.getDepositEnabled(), "Deposits into Rocket Pool are currently disabled");
    require(msg.value >= rocketDAOProtocolSettingsDeposit.getMinimumDeposit(), "The deposited amount is less than the minimum deposit size");
    RocketVaultInterface rocketVault = RocketVaultInterface(getContractAddress("rocketVault"));
    require(rocketVault.balanceOf("rocketDepositPool").add(msg.value) <= rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize(), "The deposit pool size after depositing exceeds the maximum size");
    // Calculate deposit fee
    uint256 depositFee = msg.value.mul(rocketDAOProtocolSettingsDeposit.getDepositFee()).div(calcBase);
    uint256 depositNet = msg.value.sub(depositFee);
    // Mint rETH to user account
    RocketTokenRETHInterface rocketTokenRETH = RocketTokenRETHInterface(getContractAddress("rocketTokenRETH"));
    rocketTokenRETH.mint(depositNet, msg.sender);
    // Emit deposit received event
    emit DepositReceived(msg.sender, msg.value, block.timestamp);
    // Process deposit
    processDeposit(rocketVault, rocketDAOProtocolSettingsDeposit);
}

You can see that there is the following check:
require(rocketDAOProtocolSettingsDeposit.getDepositEnabled(), "Deposits into Rocket Pool are currently disabled")

This check is not replicated in the Reth.poolCanDeposit function. So if RocketPool deposits are disabled the Asymmetry protocol still tries to deposit to the RocketDepositPool and the whole deposit fails.

This blocks the SafEth.stake function which is critical functionality since it is the entry point into the Asymmetry protocol.

Tools Used

VSCode

Recommended Mitigation Steps

The Reth.poolCanDeposit function should include a check that getDepositEnabled() == true:

diff --git a/contracts/SafEth/derivatives/Reth.sol b/contracts/SafEth/derivatives/Reth.sol
index b6e0694..92fd35a 100644
--- a/contracts/SafEth/derivatives/Reth.sol
+++ b/contracts/SafEth/derivatives/Reth.sol
@@ -146,7 +146,8 @@ contract Reth is IDerivative, Initializable, OwnableUpgradeable {
         return
             rocketDepositPool.getBalance() + _amount <=
             rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize() &&
-            _amount >= rocketDAOProtocolSettingsDeposit.getMinimumDeposit();
+            _amount >= rocketDAOProtocolSettingsDeposit.getMinimumDeposit() && 
+            rocketDAOProtocolSettingsDeposit.getDepositEnabled();
     }

Lines of code

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

Vulnerability details

Impact

When users stake their ETH in the Asymmetry protocol this downstream calls the deposit function for the different derivatives.

When the WstEth.deposit function is called the ETH is staked with the Lido protocol.

The issue is that the Lido protocol employs a rate-limiting mechanism to limit the amount of ETH that can be staked within a 24 hour period.

To undertstand that this is an issue we must understand that for the Rocketpool derivative there also exist limitations for staking. In the case of Rocketpool, the Asymmetry protocol will check if these conditions apply and if so, get the rETH from Uniswap (Link).

So we can see that it is intended for the Asymmetry protocol to allow staking in their protocol even when the downstream derivative protocols have restricted staking.

Now it is clear that the Lido rate-limiting is an issue. The Lido WstEth derivative should check if the rate-limiting applies. And it if does, the stETH should be aquired via the Curve pool and then wrapped to get wstETH.

Proof of Concept

The WstEth.deposit function contains the following line:

Link

(bool sent, ) = WST_ETH.call{value: msg.value}("");

This calls this function in the Lido protocol then:
Link

receive() external payable {
    uint256 shares = stETH.submit{value: msg.value}(address(0));
    _mint(msg.sender, shares);
}

As you can see this calls the submit function which in turn calls the _submit function:
Link

function submit(address _referral) external payable returns (uint256) {
    return _submit(_referral);
}

The _submit function contains the code that we are interested in:
Link

function _submit(address _referral) internal returns (uint256) {
    require(msg.value != 0, "ZERO_DEPOSIT");


    StakeLimitState.Data memory stakeLimitData = STAKING_STATE_POSITION.getStorageStakeLimitStruct();
    require(!stakeLimitData.isStakingPaused(), "STAKING_PAUSED");


    if (stakeLimitData.isStakingLimitSet()) {
        uint256 currentStakeLimit = stakeLimitData.calculateCurrentStakeLimit();


        require(msg.value <= currentStakeLimit, "STAKE_LIMIT");


        STAKING_STATE_POSITION.setStorageStakeLimitStruct(
            stakeLimitData.updatePrevStakeLimit(currentStakeLimit - msg.value)
        );
    }


    uint256 sharesAmount = getSharesByPooledEth(msg.value);
    if (sharesAmount == 0) {
        // totalControlledEther is 0: either the first-ever deposit or complete slashing
        // assume that shares correspond to Ether 1-to-1
        sharesAmount = msg.value;
    }


    _mintShares(msg.sender, sharesAmount);


    BUFFERED_ETHER_POSITION.setStorageUint256(_getBufferedEther().add(msg.value));
    emit Submitted(msg.sender, msg.value, _referral);


    _emitTransferAfterMintingShares(msg.sender, sharesAmount);
    return sharesAmount;
}

This contains the check for the staking limit:

if (stakeLimitData.isStakingLimitSet()) {
    uint256 currentStakeLimit = stakeLimitData.calculateCurrentStakeLimit();


    require(msg.value <= currentStakeLimit, "STAKE_LIMIT");


    STAKING_STATE_POSITION.setStorageStakeLimitStruct(
        stakeLimitData.updatePrevStakeLimit(currentStakeLimit - msg.value)
    );
}

This is a 24 hour rate limit and if it is reached the Asymmetry protocol staking functionality is blocked. Also the rebalanceToWeights function is blocked since it requires making deposits as well.

You can see in the following article from Cointelegraph that the staking limit has even recently been activated and staking was halted (https://cointelegraph.com/news/lido-finance-activates-staking-rate-limit-after-more-than-150-000-eth-staked)

So this scenario is definitely one that occurs.

Tools Used

VSCode

Recommended Mitigation Steps

In the WstEth.deposit function it should be checked if the amount of ETH (msg.value) can be deposited without triggering the staking limit.

You can use the getCurrentStakeLimit function (Link) for this purpose and check if msg.value <= getCurrentStakeLimit().

If the stake limit does apply, i.e. msg.value > getCurrentStakeLimit(), you should exchange the ETH for stETH via the Curve pool and then wrap it to get wstETH via the wrap function from the Lido protocol (Link).

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/SafEth.sol#L108-L129 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L138-L155

Vulnerability details

Impact

All three main operations of the Asymmetry protocol (SafEth.stake, SafEth.unstake, SafEth.rebalanceToWeights) involve the interaction with derivative contracts.

One of the derivatives is WstEth.

The WstEth contract deals with stETH in its wrapped form which is wstETH. The issue is that interactions (for our purposes minting and transferring) with stETH can be paused.

This causes the above functionality (stake, unstake, rebalance) to be blocked. So users cannot unstake from the Asymmetry protocol which is the most important of all 3 functionalities.

Proof of Concept

The contract that contains the pause functionality is the StETH contract from the Lido protocol.

When the Asymmetry protocol performs a stake operation then this will downstream cause the StETH._mintShares function to be executed which reverts when the contract is paused (see the whenNotStopped modifier):

Link

    function _mintShares(address _recipient, uint256 _sharesAmount) internal whenNotStopped returns (uint256 newTotalShares)

When the Asymmetry protocol performs a unstake or rebalanceToWeights operation (which swaps stETH in a Curve pool) this downstream executes the StETH._transferShares function which also reverts when StETH is paused:

Link

function _transferShares(address _sender, address _recipient, uint256 _sharesAmount) internal whenNotStopped {
    require(_sender != address(0), "TRANSFER_FROM_THE_ZERO_ADDRESS");
    require(_recipient != address(0), "TRANSFER_TO_THE_ZERO_ADDRESS");


    uint256 currentSenderShares = shares[_sender];
    require(_sharesAmount <= currentSenderShares, "TRANSFER_AMOUNT_EXCEEDS_BALANCE");


    shares[_sender] = currentSenderShares.sub(_sharesAmount);
    shares[_recipient] = shares[_recipient].add(_sharesAmount);
}

As we can see when StETH is paused the Asymmetry protocol is completely blocked from performing any meaningful operations.

Tools Used

VSCode

Recommended Mitigation Steps

It is hard to mitigate the issue that stETH transfers can be paused. This is because if they are paused what should happen with a unstake or rebalanceToWeights function call?

Should stETH be skipped over? This might leave users that don't know about the (possibly temporary) issue with stETH with less funds because they only withdraw from the remaining derivatives.

I cannot suggest a good solution here. At the time of evaluating all the other issues and possibly refactoring the protocol it might be possible for the sponsor to come up with a good solution.

Lines of code

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

Vulnerability details

Impact

The SafEth.unstake function iterates over all derivatives and withdraws the fair share of each of them (converts it to ETH and sends it to the user).

The issue is that when any of the derivatives (which cannot be removed) misbehaves, the user is unable to get his share of all the other derivatives. This means the availability of the Asymmetry protocol is severly impacted.

Proof of Concept

Let's look at the SafEth.unstake function.

It iterates over all the derivatives that have ever been added to the protocol:

Link

for (uint256 i = 0; i < derivativeCount; i++) {
    // withdraw a percentage of each asset based on the amount of safETH
    uint256 derivativeAmount = (derivatives[i].balance() *
        _safEthAmount) / safEthTotalSupply;
    if (derivativeAmount == 0) continue; // if derivative empty ignore
    derivatives[i].withdraw(derivativeAmount);
}

And if the Asymmetry protocol still holds some small amount of them they are still tried to be withdrawn.

The withdrawal (and also even querying the balance) calls external functions of the respective derivative protocol and so if there is any issue with it no unstaking will be possible.

Tools Used

VSCode

Recommended Mitigation Steps

It should be possible for the users to unstake their ETH at any time, possibly at a loss.

So I suggest that there is some kind of alternative emergency unstake function and users can supply to it the derivatives that they want to unstake. Thereby if one of the derivatives misbehaves users may choose to unstake the others anytime they want (of course thereby losing the derivative that blocks the others).

Asymmetry Low Risk and Non-Critical Issues

Summary

[L-01] Use fixed compiler version

All in scope contracts use ^0.8.13 as compiler version.
They should use a fixed version, i.e. 0.8.13, to make sure the contracts are always compiled with the intended version.

[L-02] Wrong index emitted in DerivativeAdded event

The DerivativeAdded event is defined as this:
Link

event DerivativeAdded(
    address indexed contractAddress,
    uint weight,
    uint index
);

So the third parameter is the index of the new derivative in the derivatives mapping.

The addDerivative function that emits this event does it like this:
Link

function addDerivative(
    address _contractAddress,
    uint256 _weight
) external onlyOwner {
    derivatives[derivativeCount] = IDerivative(_contractAddress);
    weights[derivativeCount] = _weight;
    // @audit increment happens
    derivativeCount++;


    uint256 localTotalWeight = 0;
    for (uint256 i = 0; i < derivativeCount; i++)
        localTotalWeight += weights[i];
    totalWeight = localTotalWeight;
    // @audit should be derivativeCount - 1
    emit DerivativeAdded(_contractAddress, _weight, derivativeCount);
}

The issue is that derivativeCount is not the correct index.
The correct index is derivativeCount - 1 since derivativeCount is incremented in the function.

The impact of this is that any off-chain components that process this event will misbehave.

This can possibly result in the adjustWeight function being called with the wrong index.

[L-03] OwnableUpgradeable: Does not implement 2-Step-Process for transferring ownership

SafEth inherits from openzeppelin's OwnableUpgradeable contract.
This contract does not implement a 2-Step-Process for transferring ownership.
So ownership of the contract can easily be lost when making a mistake when transferring ownership.

Consider using the OwnableUpgradeable2Step contract (https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/access/Ownable2StepUpgradeable.sol) instead.

Note that I did not flag this as an issue for the derivative contracts. The derivative contracts are only ever owned by a single SafEth contract and ownership will not be transferred. So it is ok to use the regular OwnableUpgradeable contract for the derivatives.

[L-04] SafEth.stake: check ethAmount for zero instead of weight

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

The SafEth.stake function skips a derivative if weight == 0.

However this can introduce an issue if weight is really small.

weight can have a very small non-zero value which causes ethAmount to be zero:
uint256 ethAmount = (msg.value * weight) / totalWeight;

E.g. 0.5 ETH * 1 / 1e18 = 0.

Some derivatives will revert when trying to deposit a zero amount.

Therefore it is safer to check ethAmount for zero instead of weight.

diff --git a/contracts/SafEth/SafEth.sol b/contracts/SafEth/SafEth.sol
index ebadb4b..56afefb 100644
--- a/contracts/SafEth/SafEth.sol
+++ b/contracts/SafEth/SafEth.sol
@@ -84,9 +84,8 @@ contract SafEth is
         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;
-
+            if (ethAmount == 0) continue;
             // This is slightly less than ethAmount because slippage
             uint256 depositAmount = derivative.deposit{value: ethAmount}();
             uint derivativeReceivedEthValue = (derivative.ethPerDerivative(

[L-05] Fixed slippage setting is inefficient because traded amounts can vary

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

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

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

All three derivative contracts (Reth.sol, SfrxEth.sol and WstEth.sol) use a fixed slippage percentage. This means that while the owner can change the slippage for each derivative contract, the slippage setting for a given contract is the same whether a small amount of funds is traded or a big amount of funds is traded.

This means that the slippage percentage must be chosen big enough such that the maximum amount of funds possible can be traded (after talking to the sponsor this is in the range of 100 to 1000 ETH).

(The only functionality that trades more funds is rebalancing. But the owner can pause staking, increase the slippage percentage, rebalance, decrease the slippage percentage and unpause staking again. So there is no need to set the slippage percentage so high to allow rebalancing all of the time.)

Obviously this slippage percentage is not the best percentage when a small amount is traded.

I estimate this to be of "Low" severity because currently the liquidity in all pools that the protocol integrates with is sufficient such that the price moves only a little even with the maximum amount of funds that will be traded.

However the liquidity may dry out and then a single slippage percentage offers no slippage protection when small amounts are traded.

Therefore it might be beneficial to let users choose the slippage percentage for themselves.

[L-06] Reth.ethPerDerivative function should always use ETH value from Rocketpool instead of pool price

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

The Reth.ethPerDerivative function currently checks if deposits to Rocketpool are possible and calculates the ethPerDerivative value depending on this.

The ethPerDerivative value should always be calculated via the getEthValue function from Rocketpool and never via the pool price.

This is because withdrawals are currently always handled through Rocketpool.

So the value of the rETH (how many ETH it is worth upon withdrawal) is determined by Rocketpool not the Curve pool.

Fix:

diff --git a/contracts/SafEth/derivatives/Reth.sol b/contracts/SafEth/derivatives/Reth.sol
index b6e0694..79bbd8c 100644
--- a/contracts/SafEth/derivatives/Reth.sol
+++ b/contracts/SafEth/derivatives/Reth.sol
@@ -209,10 +209,7 @@ contract Reth is IDerivative, Initializable, OwnableUpgradeable {
         @param _amount - amount to check for ETH price
      */
     function ethPerDerivative(uint256 _amount) public view returns (uint256) {
-        if (poolCanDeposit(_amount))
-            return
-                RocketTokenRETHInterface(rethAddress()).getEthValue(10 ** 18);
-        else return (poolPrice() * 10 ** 18) / (10 ** 18);
+        return RocketTokenRETHInterface(rethAddress()).getEthValue(10 ** 18);
     }

[N-01] Remove unnecessary imports

Unnecessary imports should be removed from the source files to improve readability.

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

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

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

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

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

[N-02] SafEth contract receive function can be dangerous for users

The SafEth contract implements a receive function:

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

This function is needed for the derivative contracts to send ETH which is then sent along further to a user that wants to withdraw.

The issue is that it's easy for a user to send the ETH directly to the SafEth contract instead of using the SafEth.stake function. This would cause a complete loss of funds.

This is a mistake by the user but it might still be beneficial to check within the receive function that msg.sender is one of the derivative contracts.

Thereby a user can only send ETH to the SafEth contract by calling the SafEth.stake function which is the correct function to call.

[N-03] SafEth: Use address(this).balance instead of calculating difference

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

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

The SafEth contract does not hold any ETH across transactions. So it does not need to calculate ethAmountAfter - ethAmountBefore.

It can just use address(this).balance.

This simplifies the Code, makes it more readable and also saves a bit of Gas.

Fix:

diff --git a/contracts/SafEth/SafEth.sol b/contracts/SafEth/SafEth.sol
index ebadb4b..5a2286f 100644
--- a/contracts/SafEth/SafEth.sol
+++ b/contracts/SafEth/SafEth.sol
@@ -108,7 +108,6 @@ contract SafEth is
     function unstake(uint256 _safEthAmount) external {
         require(pauseUnstaking == false, "unstaking is paused");
         uint256 safEthTotalSupply = totalSupply();
-        uint256 ethAmountBefore = address(this).balance;
 
         for (uint256 i = 0; i < derivativeCount; i++) {
             // withdraw a percentage of each asset based on the amount of safETH
@@ -118,8 +117,7 @@ contract SafEth is
             derivatives[i].withdraw(derivativeAmount);
         }
         _burn(msg.sender, _safEthAmount);
-        uint256 ethAmountAfter = address(this).balance;
-        uint256 ethAmountToWithdraw = ethAmountAfter - ethAmountBefore;
+        uint256 ethAmountToWithdraw = address(this).balance;
         // solhint-disable-next-line
         (bool sent, ) = address(msg.sender).call{value: ethAmountToWithdraw}(
             ""
@@ -136,13 +134,11 @@ contract SafEth is
         @dev - Probably not going to be used often, if at all
     */
     function rebalanceToWeights() external onlyOwner {
-        uint256 ethAmountBefore = address(this).balance;
         for (uint i = 0; i < derivativeCount; i++) {
             if (derivatives[i].balance() > 0)
                 derivatives[i].withdraw(derivatives[i].balance());
         }
-        uint256 ethAmountAfter = address(this).balance;
-        uint256 ethAmountToRebalance = ethAmountAfter - ethAmountBefore;
+        uint256 ethAmountToRebalance = address(this).balance;
 
         for (uint i = 0; i < derivativeCount; i++) {
             if (weights[i] == 0 || ethAmountToRebalance == 0) continue;

[N-04] Reth.ethPerDerivative calculation can be simplified

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

The line:

else return (poolPrice() * 10 ** 18) / (10 ** 18);

can be simplified to:

else return poolPrice();