Asymmetry contest - 0xTraub's results

Lines of code

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

Vulnerability details

Impact

In the Reth.sol derivative contract, if poolCanDeposit returns false, then when ethPerDerivative() is called, it will use the current value of the UniswapV3 pool instead of the Rocketpool Price. The code checks the current sqrtPriceX96 from the contract storage, which can be easily manipulated, allowing a malicious user to dilute a staker's received tokens. Knowing ahead of time that the poolCanDeposit will return false, can manipulate the price of the pool to make Reth worth significantly more than ETH by buying up all the RETH with a flash loan. Given as the pool only has ~$5M in liquidity and most of the liquidity is concentrated in a small range, this is not very hard to accomplish. This will make ethPerDerivative return a significantly higher value than expected. As a result a user would receive less tokens than expected, because their deposit amount would represent a smaller fraction than normal of ownership of underlying tokens. These funds when withdrawn will return a smaller amount of tokens than originally deposited.

Proof of Concept

1. Alice is going to deposit 1 Eth into stake(). There is 10 eth total. For simplicity purposes let's say 100% of weights are in Reth. poolCanDeposit() will return false, so the Uniswap Oracle is used instead of Rocketpool pricing functions.

2. Bob frontruns Alice. First he buys up a large amount of RETH liquidity from the pool (using a flash loan), making the price of RETH increase. The value of ethPerDerivative now increased to >=2:1

3. The expected underlyingValue is now 20 eth, even though 10 eth was originally deposited.

4. Alice still goes to deposit her 1 eth. Since she is swapping at the rate just created, it will still be worth 1 eth. However, it's compared against a balance() of 20 eth. Alice is minted tokens representing 1/20 of the supply instead of the normal 1/10. When the pool returns to normal The balance will be worth 11 eth.

5. If alice redeems those shares it will only be 0.55 eth (1/20 * 11), instead of the 1 eth she deposited. The remaining .45 eth will go to the holders, in this case the attacker.

6. Attacker redeems their shares for profit.

Tools Used

VS-Code Foundry

Recommended Mitigation Steps

Use a Uniswap V3 Time-Weighted Price Oracle. This makes it significantly harder for the attacker to manipulate the price of the Uniswap V3 Pool. Also consider using the curve pool for RETH/ETH since it contains more liquidity and the stableswap invariant makes it harder to effect the price as it moves out of normal range.

https://blog.uniswap.org/uniswap-v3-oracles

--- SfrxEth.sol --- Line 79 -> uint256 minOut = (((ethPerDerivative(_amount) * _amount) / 10 ** 18) * (10 ** 18 - maxSlippage)) / 10 ** 18;

The maxSlippage parameter can be set by owner. there is no check for whether maxSlippage is <= 1e18. If it is = then the entire equation resolves to minOut = 0. Since this is used in the curve swap, a minAmountZero allows for trade funds to be withdrawn to be stolen as there's no infinite slippage. If it > then there will be an underflow error which would prevent users from withdrawing. The same issue can be found for the StEthEthPool trade on WstEth.sol line 61 and

--- SafETH.sol ---

1. SafETH.sol is the owner of all the derivatives contracts. It does not have a method to renounce ownership should it ever need to be deprecated. Example: If the original SafETH.sol is ever rendered unusable, such as from a storage corruption problem with the Proxy contract, users would not be able to withdraw from the derivatives contract since it is the only one able to do withdrawals. The funds in the derivatives contracts would be locked forever. The SafETH file should include an onlyOwner function in which SafETH.sol transfers ownership of the derivatives contract to somewhere new. Consider using a roll based system where admins can permit different contracts deposit/withdraw which allows for further upgradability with multiple versions of controlled deposit contracts.

2. adjustWeight does not include a call to rebalanceToWeights() after the weights are adjusted. If a user backruns a change in weights without adjusting the balance then the balance() of a derivative may not match its expected value with weight. rebalanceToWeights() should be called automatically at the end after weights are adjusted.

3. tag nonReentrant onto the stake and unstake method for safety

--- Reth.sol ---

1. Consider using the CRV pool for RETH/ETH rather than the Uniswap V3 pool. The stableswap invariant makes it harder to engage in oracle manipulation and results in less slippage at high volumes. It also has ~$2M more in liquidity then the Uniswap Pool resulting in a lower price impact

--- All derivatives contracts ---

1. All of the derivatives contracts are OwnableUpgradable. However, ownableUpgradable does not set the owner of the contract without an explicit call. It requires you to call __Ownable_init() to transfer ownership of the contract to msg.sender. After the constructor there is no ownership set. Similasrly, the initializer modifier does not do a check that the caller is authorized to do so. As a result anyone can backrun the contract deployment and call initialize(address _owner) and make themselves the owner, hijacking the contract. This can be resolved by calling __Ownable_init() in the constructor to set deployer as owner, and then adding the onlyOwner modifier to the initialize() function.

2. withdraw should use address(msg.sender).transfer() rather than call. The SafEth method is intended to use receive() with no function body and has no fallback function. Since only 2100 gas is forwarded it can prevent potential reentrancy exploits from minimal code execution, and the call to withdraw() will immediately return to unstake() afterwards