Asymmetry contest - Tricko's results

Lines of code

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

Vulnerability details

Impact

First minter can artificially inflate preDepositPrice, enabling him to steal funds from small deposits.

Proof of Concept

For simplicity consider that wstETH value is equal to ETH and Bob manages to be the first minter on the recently deployed SafEth contract. The attack consist on the following steps.

1. Bob stakes the minimum amount of ETH and receives X safETH tokens
2. Bob unstakes X-1 safETH tokens, leaving totalSupply() == 1
3. Bob then directly transfer 10^20 wstETH to the wstEth derivative contact After this series of actions, the final state of the SafEth contract is totalSupply() == 1, underlyingValue = 1e20, therefore artificially inflating the preDepositPrice. Under these circumstances, any user who attempts to stake a small amount of ETH will have their mintAmount rounded down to zero, as shown in the code snippet below

uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;
_mint(msg.sender, mintAmount);

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

Besides hindering small stakers from using the protocol, Bob is effectively stealing those user funds as they will deposit ETH, but don't receive safETH tokens in return. Subsequently, Bob can cash out his stake for a profit. Due to the ease with which this attack can be executed and the severity of its implications for future stakers, I consider it to be a high severity issue.

Tools Used

Manual Review

Recommended Mitigation Steps

Please consider minting small fraction of the first mint to address(0). This increase the funds necessary for the attacker to perform this exploit, as shown by the Uniswap team.

Also consider checking if mintAmount > 0 and reverting otherwise, as shown in the diff below, thus preventing users from losing all their ETH deposited and receiving no safETH in return.

diff --git a/SafEth.orig.sol b/SafEth.sol
index d5bbd84..b0009b5 100644
--- a/SafEth.orig.sol
+++ b/SafEth.sol
@@ -97,6 +97,7 @@ contract SafEth is
             totalStakeValueEth += derivativeReceivedEthValue; //total value user has staked in this call
         }
         uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;
+        require(mintAmount > 0, "Mint amount must be greater than zero");
         _mint(msg.sender, mintAmount);
         emit Staked(msg.sender, msg.value, mintAmount);
     }

Lines of code

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

Vulnerability details

Impact

On two places in the WstEth contract, an implicit 1:1 relationship between ETH and stETH is defined, leading to loss of user funds on unexpected market conditions.

Proof of Concept

First on the withdraw function, as shown in the snippet below, where the minOut value is calculated as (stEthBal * (10 ** 18 - maxSlippage)) / 10 ** 18, defining the minOut in function of stETH value, not ETH.

function withdraw(uint256 _amount) external onlyOwner {
    IWStETH(WST_ETH).unwrap(_amount);
    uint256 stEthBal = IERC20(STETH_TOKEN).balanceOf(address(this));
    IERC20(STETH_TOKEN).approve(LIDO_CRV_POOL, stEthBal);
    uint256 minOut = (stEthBal * (10 ** 18 - maxSlippage)) / 10 ** 18;
    IStEthEthPool(LIDO_CRV_POOL).exchange(1, 0, stEthBal, minOut);
    // solhint-disable-next-line
    (bool sent, ) = address(msg.sender).call{value: address(this).balance}(
        ""
    );
    require(sent, "Failed to send Ether");
}

And also on ethPerDerivative, where instead of ETH/wstETH , it actually returns the stETH/wstETH ratio.

function ethPerDerivative(uint256 _amount) public view returns (uint256) {
    return IWStETH(WST_ETH).getStETHByWstETH(10 ** 18);
}

In both cases, the values are returned in terms of stETH instead of ETH, which establishes an implicit 1:1 relationship between ETH and stETH. Despite the historical swap rate being near 1, there have been instances when this correlation did not hold, such as on June 10, 2022, following the Luna collapse, when stETH/ETH was trading at 0.94 on multiple DEXes. Recent events, such as the USDC depeg, indicate that hardcoding these relationships is unsafe since they may break during extreme market conditions, resulting in potential exploits.

To illustrate, let's consider a scenario where the stETH/ETH pair is trading at 0.94. Because the value used in ethPerDerivative is hardcoded to 1, users who stake their funds will receive less safETH than they should. This is because underlyingValue would be above its actual value, resulting in a loss of funds for the protocol users.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using an price oracle to define the stETH/ETH instead of hardcoding it to 1, similarly to what is done for the FRX/ETH relationship in SfrxEth.sol.

Lines of code

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

Vulnerability details

Impact

Lido has established daily staking limits as a safeguard against unexpected market conditions. At present, the daily staking limit is set at 150000 ether, but Lido retains the right to alter this limit in the future.

If the Asymmetry protocol expands to the point where the amount of ETH held in Lido derivatives exceeds the limit, any attempts to rebalance using the rebalanceToWeights() function will revert, and the rebalancing mechanism will be blocked until the weights are adjusted to bring the deposited ETH below the limit.

It is worth noting that the protocol owners will need to continuously adjust the weights to accommodate the growing ETH balance on Lido resulting from new Asymmetry users' deposits. However, since the developers have indicated that rebalanceToWeights() will not be used often, this issue can be classified as medium severity.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a hard limit on the amount of ETH deposited to the Lido’s derivatives and allocate that excess ETH to the others derivatives.

Lines of code

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

Vulnerability details

Impact

During rebalancing all the ETH staked on the derivatives are withdrawn to be re-deposited. Besides rETH, all the others derivatives' withdraw process are done through a single DEX swap per derivative. Therefore the higher the balance on the derivatives, less efficient those swaps will be and more user funds will be lost to slippage and price impact.

Pairs like stETH/ETH have massive pools containing on the order of billions of dollars worth of tokens. But the Curve FRX/ETH pool used by the SfrxEth contract is orders of magnitude smaller (At the time of this writing it held 33,375 ETH and 38,868 Frax), thus being significantly more affected by the swap size.

This means, that if the Asymmetry protocol grows high enough, rebalances will keep incurring high costs to the funds stored in the protocol. Relatively small (on a protocol scale) rebalances will cause significant price impacts, especially in smaller pools like FRX/ETH.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding new methods to do partial rebalancing, this way if needed the protocol can schedule smaller rebalances over days or weeks, minimizing the price impact and consequently leading to smaller losses to users’ funds.

Lines of code

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

Vulnerability details

Impact

RocketPool rETH tokens have a deposit delay that prevents any user who has recently deposited to transfer or burn tokens. In the past this delay was set to 5760 blocks mined (aprox. 19h, considering one block per 12s). This delay can prevent asymmetry protocol users from unstaking if another user staked recently.

While it's not currently possible due to RocketPool's configuration, any future changes made to this delay by the admins could potentially lead to a denial-of-service attack on the ùnstake() mechanism. This is a major functionality of the asymmetry protocol, and therefore, it should be classified as a high severity issue.

Proof of Concept

Currently, the delay is set to zero, but if RocketPool admins decide to change this value in the future, it could cause issues. Specifically, protocol users staking actions could prevent other users from unstaking for a few hours. Given that many users call the stake function throughout the day, the delay would constantly reset, making the unstaking mechanism unusable. It's important to note that this only occurs when stake() is used through the rocketDepositPool route. If rETH is obtained from the Uniswap pool, the delay is not affected.

A malicious actor can also exploit this to be able to block all unstake calls. Consider the following scenario where the delay was raised again to 5760 blocks. Bob (malicious actor) call stakes() with the minimum amount, consequently triggering deposit to RocketPool and resetting the deposit delay. Alice tries to unstake her funds, but during rETH burn, it fails due to the delay check, reverting the unstake call.

If Bob manages to repeatedly stakes() the minimum amount every 19h (or any other interval less then the deposit delay), all future calls to unstake will revert.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider modifying Reth derivative to obtain rETH only through the UniswapV3 pool, on average users will get less rETH due to the slippage, but will avoid any future issues with the deposit delay mechanism.