Asymmetry contest - yac's results

Lines of code

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

Vulnerability details

Impact

When a user deposits after an inflation attack, the user could receive zero SafEth tokens because of how solidity rounds down non-integer values. This means that the user can lose all of the ETH that they deposit because zero SafEth tokens are minted for their deposit.

Proof of Concept

The core problem here is that it is possible to pass a mintAmount of zero to the OpenZeppelin _mint() function. In comparison, the solmate ERC4626 minting logic will purposefully revert if zero shares will be minted.

Assume the following scenario:

1. The Asymmetry protocol is deployed to mainnet
2. The first user wants to deposit 0.5 ETH, the minAmount value, into the protocol. The user's transaction is submitted to the mempool.
3. The attacker observes this transaction and frontruns it with several actions:
   1. The attacker calls stake() with 0.5 ETH into asymmetry and receives slightly less than 0.5 ETH in SafETH in return (actually 5 * 10^17 considering it is a ERC20 with 18 decimals)
   2. The attacker calls unstake() to withdraw all of their SafETH tokens except for 1 wei
   3. The attacker airdrops 1 ETH (actually 10^18 for ERC20 tokens with 18 decimals) to each derivative contract (Reth.sol, SfrxEth.sol, WstEth.sol), which will push the value of preDepositPrice to 10 ^ 36. preDepositPrice is now 3 * 10^18.
   4. Now if a user deposits less than 1 ETH, the user will receive no SafEth tokens. This is because when the user calls stake(), the value of totalStakeValueEth is X and preDepositPrice is X. If the attacker airdrop 10 ETH per derivative then everything deposited under 10 ETH won't mint anything.

Tools Used

Manual analysis

Recommended Mitigation Steps

Two fixes should be made:

• Before the first deposit is made, a small number of SafEth should be sent to the zero address. This will prevent the inflation attack.
• Make sure minting zero shares reverts.

Some related information and discussion on inflation attacks, though specific to ERC4626, is found in the OZ repo.

Lines of code

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

Vulnerability details

Impact

poolPrice() in Reth.sol uses the Uniswap pool spot price as an oracle. This makes it vulnerable to manipulation from flash loans.

Proof of Concept

The Reth.poolprice() function should return the expected exchange rate between rETH and ETH. But the return value can be manipulated because it is using the spot price of the Uniswap pool. The liquidity of this pool is only $5 million, so flashloans that frontrun a SafEth.stake() call can easily alter the spot price of the pool. The minAmount value calculated by Reth relies on the spot price too, so it does not help prevent the protocol from swapping to rETH at a very bad rate that is manipulated by frontrunning.

Tools Used

Manual analysis

Recommended Mitigation Steps

Do not use Uniswap pool spot price for a price oracle. Instead, use Chainlink or another source of price data that cannot easily be manipulated.

Lines of code

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

Vulnerability details

Impact

rebalanceToWeights() in SafEth.sol works by withdrawing all derivative positions and depositing all positions with the new weights. This causes greater loss of value than is necessary.

In the case of rETH, if rETH cannot be deposited into the RocketPool contract directly, WETH will be swapped for rETH using Uniswap. The fee of the pool is 500 or 0.05% (specified here). If the entire position is withdrawn and redeposited in the exact same amount, this means a 0.1% fee is paid even when there is no change in the Uniswap position.

In addition to Uniswap fees, the default slippage tolerance of 1% may give MEV bots an arbitrage opportunity, resulting in additional loss of value for every rebalance. Each derivative has a maxSlippage state variable, so during a rebalance it is possible that a 1% loss of value for each derivative could take place. For n derivatives, that's n% of the total ETH staked that could be lost (i.e. up to 3% loss with 3 derivatives). A brief test of Uniswap v3 showed an autoslippage value of 0.1-0.21% for a swap between 1 ETH and 100 ETH of value, which is significantly less than the hardcoded 1% value, increasing the risk of value loss from arbitrage.

Proof of Concept

The core problem is that all derivative balances are withdrawn and redeposited in rebalanceToWeights(), instead of only the balances that need to be moved to a new derivative.

There are two ways value can be lost during this process.

1. Uniswap fees of 0.05%

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

2. Slippage values that allows MEV both value extraction (default of 1%)

• rETH slippage: https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L173-L174
• SfrxEth slippage: https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L74-L75
• WstEth slippage: https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L60

Tools Used

Manual analysis

Recommended Mitigation Steps

rebalanceToWeights() should not withdraw and redeposit all positions. Instead, the rebalancing should only withdraw and deposit the exact value than needs changing. For example, if there is 1 ether of rETH than needs to be converted to wstETH, then only 1 ETH should be withdrawn from Reth.sol and deposited with WestEth.sol. Moving less value will result in less fees paid to Uniswap if Reth.swapExactInputSingleHop() is called and also reduce the risk of frontrunning that arbitrages the maxSlippage amount.

In order to reduce the value moved during rebalancing, the SafEth.sol contract should do the following steps:

1. calculate the new ethAmount for each derivative with the new weights
2. calculate the ethDelta between the ethAmount and the current ethAmount for every derivative
3. Increase or decrease the ether invested in each derivative based on whether ethDelta is greater than zero or less than zero. If ethDelta is zero, no action is needed for this derivative.

Lines of code

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

Vulnerability details

Impact

rETH is acquired using the Uniswap rETH/WETH pool. This solution has higher fees and lower liquidity than alternatives, which results in more lost user value than other solutions.

The Uniswap rETH/WETH pool that is used in Reth.sol to make swaps has a liquidity of $5 million. In comparison, the Balancer rETH/WETH pool has a liquidity of $80 million. Even the Curve rETH/WETH pool has a liquidity of $8 million. The greater liquidity should normally offer lower slippage to users. In addition, the fees to swap with the Balancer pool are only 0.04% compared to Uniswap's 0.05%. Even the Curve pool offers a lower fee than Uniswap with just a 0.037% fee. This Dune Analytics dashboard shows that Balancer is where the majority of rETH swaps happen by volume.

One solution to finding the best swap path for rETH is to use RocketPool's RocketSwapRouter.sol contract swapTo() function. When users visit the RocketPool frontend to swap ETH for rETH, this is the function that RocketPool calls for the user. RocketSwapRouter.sol automatically determines the best way to split the swap between Balancer and Uniswap pools.

Proof of Concept

Pools that can be used for rETH/WETH swapping:

• Uniswap rETH/WETH pool: $5 million in liquidity
• Balancer rETH/WETH pool
• Curve Finance rETH/ETH pool: $8 million in liquidity

Line where Reth.sol swaps WETH for rETH with the Uniswap rETH/WETH pool.

Tools Used

Etherscan, Dune Analytics

Recommended Mitigation Steps

The best solution is to use the same flow as RocketPool's frontend UI and to call swapTo() in RocketSwapRouter.sol. An alternative is to modify Reth.sol to use the Balancer rETH/ETH pool for swapping instead of Uniswap's rETH/WETH pool to better conserve user value by reducing swap fees and reducing slippage costs.

Lines of code

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

Vulnerability details

Impact

When a user calls stake(), the calculation to determine the amount of ETH to convert to each derivative rounds down and can leave some ETH dust in the SafEth contract. This ETH is trapped and cannot be retrieved, resulting in users slowly losing value as stake() keeps getting called.

Proof of Concept

The division operation in this line rounds down. If there are 3 derivatives and the division rounds down for all of them, 3 wei will get trapped in the SafEth contract every time the function is called. https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L88

Tools Used

Manual analysis

Recommended Mitigation Steps

Keep address(this).balance == 0 at the end of stake() and unstake() by sending address(this).balance to msg.sender.

address(msg.sender).call{value: address(this).balance}

In unstake(), this line can be replaced by sending address(this).balance instead.

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L173-L174 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L74-L75 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L60

Vulnerability details

Impact

There is a hardcoded slippage value for each derivative which can be changed by the owner using setMaxSlippage(). Because the slippage is not user-specified, if the slippage tolerance is not met, the protocol is unusable. The hardcoded slippage will result in the stake() operation reverting if even one of the many derivatives cannot be exchanged within the slippage tolerance. Third parties can alter the exchange rate and cause the rate to be outside the slippage tolerance, preventing users from calling stake().

Proof of Concept

If the slippage tolerance is not met during the derivative.deposit() call, the entire stake() operation will revert.

Every derivative has a slippage tolerance:

• rETH slippage
• SfrxEth slippage
• WstEth slippage

The simplest case of this denial of service is in Reth.sol because the Uniswap pool can be manipulated so that the price of rETH in the Uniswap pool drops below the rate calculated by minOut.

Tools Used

Manual analysis

Recommended Mitigation Steps

Allow users to optionally specify the slippage when calling stake(), but use the owner-controlled maxSlippage if the user chooses not to specify a slippage tolerance.

1. Use storage gaps in all upgradeable contracts

Use storage gaps like all OZ upgradeable contracts avoid the possibility of future storage collisions with mappings and dynamic arrays that are allocated to low numbered storage slots. Although the odds of such a collision are low, it is better to protect against it by reserving storage slots than to expect there are never any problems, due to the severity of the consequences if there is a collision. Some OZ best practices are already applied in other places, so use the other best practices to avoid future storage collisions.

2. Reentrancy protection improvements for stake() or unstake()

stake() and unstake() in SafEth.sol have no reentrancy protection. At a minimum, it is recommended to move the line with _burn(msg.sender, _safEthAmount); earlier in unstake(), because the function calls external contracts in the for loop (albeit contracts written by the Assymetry team). Detected with this slither detector.

3. Use Uniswap TransferHelper.sol safeApprove()

Uniswap has a contract intended to help with Uniswap interactions. Use safeApprove() from this contract instead of approve() before the Uniswap interaction in Reth.sol.

4. Missing events for critical operations

Several critical operations do not trigger events in Reth.sol, SfrxEth.sol and WstEth.sol. As a result, it is difficult to check the behavior of the contracts. Without events, users and blockchain-monitoring systems cannot easily detect suspicious behavior.

5. Incorrect comment

This comment's description is inaccurate and was accidentally copied from a different function.

6. No timelock on addDerivative() lets owner control user value

addDerivative() allows the SafEth contract owner to add ETH derivatives immediately. This can be followed by a call to rebalanceToWeights() which will rebalance the position of all existing SafEth holders to hold some of the added derivative. The lack of a timelock causes two problems:

1. The owner could rug the project by creating a fake ETH derivative, adding it to SafEth with addDerivative() with a large weight that would cause most of the ETH to be sent to this new derivative, and therefore steal user value.
2. If a new ETH derivative is added with addDerivative(), some existing SafEth holders may not want exposure to this new ETH derivative. Using a timelock allows users time to withdraw their position if they do not want this exposure to the new derivative. There is an additional risk that the new derivative contract, the equivalent of Reth.sol, WstEth.sol, or SfrxEth.sol, will have a bug that could put user funds at risk. Using a timelock gives users time to review the new code that will soon control their funds to make sure they trust the new code.

1. Cache derivatives[i].balance() in rebalanceToWeights()

The derivatives[i].balance() external function is called twice in rebalanceToWeights() when the if statement is entered. Caching the value locally saves gas. Hardhat gas estimates show this caching saves 8154 gas for rebalanceToWeights().

+ uint256 derivativeBalance = derivatives[i].balance();
- if (derivatives[i].balance() > 0)
+ if (derivativeBalance > 0)
- 	derivatives[i].withdraw(derivatives[i].balance());
+ 	derivatives[i].withdraw(derivativeBalance);

2. Cache derivatives[i] in stake()

Caching derivatives[i] to a local variable in stake() in this for loop can save 537 gas.

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

3. Cache derivatives[i].balance() in stake()

Caching derivatives[i].balance() to a local variable in stake() in this for loop can save 8145 gas. When this for loop is modified so derivatives[i] and derivatives[i].balance() are cached, 8592 gas can be saved.

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

4. Avoid duplicate checks of a constant value

ethAmountToRebalance in rebalanceToWeights() is not modified in the for statement so checking if this value is zero should be moved outside of the for loop to avoid duplicate checks of the same value. This reduces the contract size by 2 bytes and hardhat gas estimates show this saving 55 gas in the maximum gas scenario.

+ if (ethAmountToRebalance != 0) {
	for (uint i = 0; i < derivativeCount; i++) {
-		if (weights[i] == 0 || ethAmountToRebalance == 0) continue;
+		if (weights[i] == 0) continue;

5. Move logic Reth.deposit() logic inside for loop

The variables rocketDepositPoolAddress and rocketDepositPool are only used in the else block of Reth.deposit(). Calculating these values is unnecessary in the if block, and therefore wastes unnecessary gas. Move these lines from outside the if/else block into the else block.

		// Per RocketPool Docs query addresses each time it is used
        address rocketDepositPoolAddress = RocketStorageInterface(
            ROCKET_STORAGE_ADDRESS
        ).getAddress(
                keccak256(
                    abi.encodePacked("contract.address", "rocketDepositPool")
                )
            );

        RocketDepositPoolInterface rocketDepositPool = RocketDepositPoolInterface(
                rocketDepositPoolAddress
            );