Asymmetry contest - igingu's results

Lines of code

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

Vulnerability details

The attack vector and impact is similar to Trail of Bits Yearn Finance Audit - 3. Division rounding may affect issuance of shares, where users may not receive any shares when staking, if the total asset amount has been manipulated through a large “donation”.

Impact

The first staker can make it so that a few units of SafeEth are worth 1Eth of underlying value, effectively making most subsequent stakings mint 0 units of SafeEth to victims.

This would not only make stakers sometimes completely use their funds (because they would be minted 0 SafEth units), but attacker could also withdraw the underlying value added by the victims, since the total number of SafEth shares didn't change and attacker holds all of them.

Proof of Concept

• attacker stakes 3 WEI. Since it is the first stake, preDepositPrice is 10 ** 18
• the 3 WEI are deposited in the staking pools, and now the totalStakedValueEth in the system is 3 WEI.
• attacker is minted (3e18) / 1e18 shares, so 3 shares.
• attacker directly sends 1 REth to Assymetry's Reth derivative contract. Now the underlying value in the system will be 1 ETH and 3 WEI from before.
• victim stakes 0.3 ETH. preDepositPrice will be computed as: (1e18 * (1e18 + 3)) / 3, which would be ~ 0.33 ETH * 1e18 .
• totalStakeValue will be ~ 0.3 ETH
• mintAmount will be ((~ 0.3 ETH * 1e18) / 0.33 ETH * 1e18 ), which would round down to 0 units of SafEth.
• now there are still 3 units of SafEth minted, but which now underly 1Reth + 0.3 ETH (from victim).
• if attacker unstakes their 3 units of shares, they will withdraw ~ 1.3 ETH, with an initial investment of 1 ETH.

Recommended Mitigation Steps

If Assymetry Finance notices this attack happening, they can immediately pause staking, blocking users from becoming victims, and also pause unstaking, blocking the attacker from unstaking their shares and getting back their 1 Reth investment.

An easy way to mitigate the vulnerability would be to require(mintAmount != 0, "Can not mint 0 shares").

Another way would be taking Uniswap example: Uniswap V2 solved this problem by sending the first 1000 LP tokens to the zero address. The same can be done in this case, send the first 1000 SafEth units to the zero address to enable share dilution.

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L73 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L95

Vulnerability details

I will put below all my concerns about how underlyingValue is computed and how totalStakedEthValue is computed. I think this should amount to a medium vulnerability, as the behavior is not consistent and can lead to many different errors in calculating correct amount of shares to mint to users. Developers should settle on which calculation they want to use, and have it consistent accross the derivatives.

underlyingValue is a number that represents how much Eth underlies all the derivatives in the system.

totalStakeValueEth is a number that represents how much Eth underlies the new derivatives that have resulted from user staking msg.value Eth.

Both underlyingValue and totalStakeValueEth are computed based on the ethPerDerivative function.

Reth

For Reth, ethPerDerivative is either the spot price of Uniswap V3 rEth/wEth pool, or RocketToken Eth value per 1 REth token.

Why is this wrong?

When computing underlyingValue, we want to see how much Eth underlies Reth in the system, we do not want to deposit any Eth yet, so we do not care if poolCanDeposit(RethDerivative.balance()) => we can always take rEth/Eth price from Rocket Pool.

When computing totalStakeValueEth, it would make more sense to get the ethPerDerivative amount before the deposit, or to change the ethPerDerivative function so that it goes on the same if branch: we deposit the Eth, and poolCanDeposit returns true, so we deposit using the conversion in RocketToken.getEthValue(); we get the ethPerDerivative conversion rate now, to compute totalStakeValueEth, and poolCanDeposit might return false, so we get the conversion in poolPrice(), which is different than the conversion that was used when depositing. This will either mint less or more shares to staker.

Recommendation:

If when depositing Eth in exchange for rEth, the RocketToken.getEthValue() computation was used, make sure that is also used to compute totalStakeValueEth.

WstEth and SfrxEth

WstEth and SfrxEth are similar tokens in regards to their relationships with stEth and frxEth, and Eth.

1 stEth should currently be pegged to Eth (by arbitrage), and 1 wstEth is worth (X >= 1) stEth.

1 frxEth should currently be pegged to Eth (by arbitrage), and 1 sfrxEth is worth (X >= 1) frxEth.

Why is this wrong?

However, the ethPerDerivative computation for these is different.

For WstEth, developers compute the amount of Eth by first getting the amount of stEth for 1 WstEth, and then returning that, assuming 1 stEth is equal to 1 Eth.

For SfrxEth, developers compute the amount of Eth by first getting the amount of frxEth for 1 SfrxEth, and then multiplying that by the frxEth/Eth Curve Liquidity pool price.

Recommendation

I think both tokens should either do the algorithm for WstEth, or do the algorithm for SfrxEth, since the relations between them are the same (1 wstEth is many stEth, 1 stEth is 1 Eth -- 1 sfrxEth is many frxEth, 1 frxEth is 1 Eth).

Also, for all 3 derivatives, it would also be a possibility to just have the totalStakeValueEth as msg.value, as the total amount of Eth that was staked is exactly msg.value, irrespective of the derivatives now being exchangable for less, because of slippage and trading fees.

Lines of code

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

Vulnerability details

Withdrawing wstEth incorrectly computes the minAmount given the maxSlippage. It assumes a 1-1 stEth to Eth conversion, while for SfrxEth and Reth, it computes the minAmount using the ethPerDerivative price that will be used during the exchange.

Impact

The more subunitary stEth/Eth is, the more unstaking calls will revert since minOut would be over the exchange price even without including the maxSlippage into the calculation. This would block user funds.

Example

Imagine stEth/Eth = 99e16. Assuming maxSlippage of 1%, and stEthBalance of 1, minOut would be 99e16 as well, which is exactly the stEth/Eth price, allowing for no actual slippage in the Curve Pool.

If minOut would have been calculated using actual exchange price (99e16), it would have been 9.801e+17, actually allowing for 1% slippage.

Recommended Mitigation Steps

contracts/SafEth/derivatives/WstEth.sol#L60
    uint256 minOut = (stEthBal * (10 ** 18 - maxSlippage)) / 10 ** 18;
    -> 
    uint256 minOut = ((stEthBal * (ethPerDerivative(stEthBal)) / 10 ** 18) *
            (10 ** 18 - maxSlippage)) / 10 ** 18;

Lines of code

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

Vulnerability details

When unstaking SafEth shares, Reth, SfrxEth and respectively WstEth are transformed back into Eth. For sfrxEth and wstEth, this transformation is done by first unwrapping the tokens (sfrxEth to frxEth, wstEth to stEth), and then trading frxEth and stEth to Eth, using a liquidity pool.

The above is done because actually unstaking Eth from a validator is not currently possible, so the only way to recover your Eth is by trading your staking tokens for Eth, on markets like curve or Uniswap.

Rocket Pool does implement such functionality, where a user can burn their rEth and receive Eth back. However, this is only possible when there is Eth that other stakers have deposited, which hasn't been used by a node operator to create a validator, or Eth was returned by a node operator after they exited one of their validators:

NOTE (from the RocketPool Staking documentation)

Trading rETH back for ETH directly with Rocket Pool 
is only possible when the staking pool has enough ETH in it to handle your trade. 
ETH in this pool comes from two sources:

ETH that other stakers have deposited, which hasn't 
been used by a node operator to create a new validator yet

ETH that was returned by a node operator after they exited one 
of their validators and received their rewards from the 
Beacon Chain (note that this is not possible until after the 
ETH1-ETH2 Merge occurs and withdrawals are enabled)

It's possible that if node operators have put all of the staking pool 
to work on the Beacon chain, then the liquidity pool won't have enough 
balance to cover your unstaking. In this scenario, you may find other 
ways to trade your rETH back to ETH (such as a decentralized exchange 
like Uniswap (opens new window)) - though they will likely come with a small premium.

The above means that you might only be able to burn a certain portion of rEth, sometimes, and other times it will revert, as there will be no unused Eth waiting to be staked in the Rocket Pool, or the unused Eth will not be enough.

Impact

Unstaking DoS

Unstaking SafEth tokens is guaranteed to often revert and block user funds, since most of the time there will not be enough leftover Eth in the Rocket Pool to account for burning the rEth in the Reth derivatives contract.

This would mean blocking user funds (not only rEth, but also sfrxEth and wstEth holdings), until each user has the luck of unstaking just enough amount of SafEth tokens, regularly, until they manage to get out all their underlying Eth. Not needed to say, the above would be a very tedious process, which could go on for weeks or months, depending of how fast leftover Eth is used to create new validators.

rebalanceToWeights DoS

Aside from staking, the rebalanceToWeights function could forever be DoS, since it doesn't only withdraw funds of one user (like unstake), but withdraws all Reth there is, from the RocketPool => it is more likely that Rocket Pool will not have enough leftover Eth to cover such big withdrawal. This in turn would DoS the removal of a derivative (changing weight to 0): one could change the weight to 0, but wouldn't be able to pull all funds from Reth and redistribute them to other two derivatives.

Proof of Concept

Reth.withdraw() function

RocketTokenRETH.burn() function

Rocket Pool Staking documentation

Recommended Mitigation Steps

Steps should be taken so that in case RocketTokenRETHInterface(rethAddress()).burn(amount); reverts, Reth smart contract would trade the rEth against wEth on Uniswap or directly Eth on Curve, instead of trying to unstake it with the Rocket Pool.

Lines of code

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

Vulnerability details

The ethPerDerivative function is used to compute the underlying value of all the derivatives currently in the system. A staker will receive an amount of shares equal to the percentage of their staked ether, over the whole underlying value in the system:

• As rEth/wEth price becomes smaller, underlyingValue becomes smaller
• As underlyingValue becomes smaller, preDepositPrice becomes smaller
• As preDepositPrice becomes smaller, number of minted shares becomes bigger

Reth.sol's ethPerDerivative function computes rEth/wEth price by querying Uniswap V3, but instead of getting the time-weighted-average-price over say, the last 30 minutes, it gets the spot price (the last observed price), which is much easier to manipulate and has been a vulnerability in many DeFi systems.

Impact

If Uniswap V3 rEth/wEth price can be manipulated, the underlying value of all the derivatives will be smaller, which will result in an attacking staker receiving more shares by depositing the same amount of Eth.

Proof of Concept

• attacker swaps a considerable amount of rEth in the rEth/wEth Uniswap V3 pool, causing the rEth price to drop
• attacker stakes a considerable amount of Eth
• during SafEth.stake() function, the system will report a smaller underlyingValue, as it is based on rEth/wEth Uniswap V3 pool spot price, and attacker will receive more SafEth shares than they were supposed to
• as spot price is arbitraged back up, to the non-manipulated value, attacker can unstake their shares to be left with more Eth than before

Recommended Mitigation Steps

An attacker manipulating price will not be able to make the oracle report a bad price for a long time, and since TWAP is weighted on time buckets, it won't impact TWAP in a significant way.

Uniswap article computing necessary value to significantly impact TWAP price

Compute Reth poolPrice() by using TWAP calculation from Uniswap V3, which greatly increases the required value for an attacking swap, as compared to spot price.

An article explaining more about what TWAP is, why it is better, with code example showing how to get it from Uniswap Oracle on-chain