Asymmetry contest - IgorZuk's results

Lines of code

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

Vulnerability details

Impact

There's no way to remove a derivative completely.

Proof of Concept

If a derivative breaks, there's no way to remove it completely. The derivative will forever be called when unstaking, at the very least the balance function, which is enough to block unstaking if it throws. It may also block rebalanceToWeights, which is an important emergency mechanism.

Tools Used

Manual review.

Recommended Mitigation Steps

Add a way to remove a derivative or at least completely stop querying it.

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#L92 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L211 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L86

Vulnerability details

Impact

When staking the amount of minted SafETH may not reflect the share of the unstakeable Ether. Depending on the blockchain state staking and immediate unstaking may cause a loss or gain of ETH at the expense of the SafETH holders.

Proof of Concept

In https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L73 the currently unstakeable Ether is calculated as derivative.ethPerDerivative(derivative.balance()) * derivative.balance(). In https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L92 the newly added unstakeable Ether is calculated as derivative.ethPerDerivative(depositAmount) * depositAmount. The goal of this calculation is to make the staking user's share of the total SafETH supply match the share of unstakeable ETH that became available. This is good because it means that one user staking doesn't affect the other user's unstakeable ETH. In order for this to work each derivative's ethPerDerivative should return the derivative price while withdrawing ETH. For example the SfrxEth derivative does this correctly, it simulates withdrawal of ETH.

The RETH derivative's implementation of ethPerDerivative in https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L211 simulates not withdrawal but deposit of ETH. Its logic has a path querying RETH price on Uniswap, which doesn't match the withdraw logic, it never uses Uniswap, instead it always burns RETH to get ETH. RETH's ethPerDerivative in the current form can't be used to accurately calculate the withdrawable amount of ETH.

The WstETH derivative has a similar problem, its implementation of ethPerDerivative in https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L86 simulates deposit of ETH. It only checks the relation between WStETH and StETH, which matches the depositing logic, but doesn't take into account that withdrawal makes an extra step of selling StETH for ETH, and they may not have a 1:1 rate.

Tools Used

Manual review.

Recommended Mitigation Steps

Make RETH and WstETH ethPerDerivative logic simulate the withdrawal of ETH.

Lines of code

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

Vulnerability details

Impact

The attacker can perform a sandwich attack and enforce an arbitrary RETH purchase rate when the user deposits when stake or rebalanceToWeights are called.

Proof of Concept

For the attack to work SafETH derivative for RETH must be in the mode of buying on Uniswap (poolCanDeposit(amt) == false). With sufficient capital the attacker can impose an arbitrary price of RETH on Uniswap by performing a sandwich attack.

• The attacker buys as much RETH for ETH on Uniswap as possible inflating RETH price.
• In the next transaction, the user then runs deposit on RETH derivative which buys very little RETH for a lot of ETH inflating RETH price even more.
• In the next transaction, the attacker sells RETH for more ETH than they put in initially effectively earning the user's loss.

This is possible because when the RETH derivative buys RETH, it calculates the maximum slippage based on the current price of RETH with no consideration for the historical rates.

Tools Used

Manual review.

Recommended Mitigation Steps

In function poolPrice use the Uniswap's observe mechanism to get the rate more difficult. Uniswap provides a library with the helpers to make it easier with consult and getQuoteAtTick: https://github.com/Uniswap/v3-periphery/blob/main/contracts/libraries/OracleLibrary.sol

Lines of code

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

Vulnerability details

Impact

Moving ETH between derivatives is infeasible for anything other than serious emergencies due to the cost of the operation.

Proof of Concept

SafETH distributes ETH among derivatives using weights when the users are staking. The owner of the protocol can change the weights, but it doesn't affect the already staked funds. In order to update the staked funds the owner must call rebalanceToWeights which unstakes all assets and then stakes them using the current weight configuration. This is very costly because both unstaking and staking may involve exchanging tokens on DEXes, which costs a percentage of the tokens in fees and may involve trading volumes big enough in relation to liquidity to cause large slippage further increasing the cost. Because staking ETH usually has no more than a few % of APY, a single rebalanceToWeights may spend gains from months of staking or even cause losses. This means that the owners will be calling this function as rarely as possible which will make the protocol miss on higher yield derivatives while keeping funds staked in poorly performing protocols. This also means that any emergency withdrawal from one of the derivatives will be a very difficult decision, on one hand there will be risk of losing the funds by not withdrawing, but on the other will be losing funds on fees. This may lead to a passive approach to risks, so even if one of the staking protocol behaves suspiciously, funds may not be withdrawn unless it's too late.

Tools Used

Manual review.

Recommended Mitigation Steps

Add tools for manual stake rebalancing allowing moving only funds that actually need to be moved and potentially over multiple transactions to avoid large slippage. Consider implementing direct exchange between derivatives' tokens without converting them to ETH first. Consider an option to automatically rebalance when changing weights, but only the assets affected by the change.

Pack global storage variables of SafETH

The current storage takes 5 slots:

bool public pauseStaking;
bool public pauseUnstaking;
uint256 public derivativeCount;
uint256 public totalWeight;
uint256 public minAmount;
uint256 public maxAmount;

This is excessive, the types can be much smaller:

• derivativeCount can be uint16 there won't be more than 65 thousand derivatives
• totalWeight can be uint32, the weights scale of 4 billion should be accurate enough
• minAmount and maxAmount can be uint96, the total supply of ETH in the ecosystem is a number that can fit in 87 bits, using 96 is more than safe.

The optimized storage fits exactly in 256 bits and takes only 1 slot:

bool public pauseStaking;
bool public pauseUnstaking;
uint16 public derivativeCount;
uint32 public totalWeight;
uint96 public minAmount;
uint96 public maxAmount;

If it's necessary, the storage can be reduced by 1 more byte if pause variables are merged into a bitmap.

Pack per-derivative storage variables of SafETH

Each derivative takes at least 3 slots:

mapping(uint256 => IDerivative) public derivatives; // derivatives in the system
mapping(uint256 => uint256) public weights; // weights for each derivative

The third slot is the maxSlippage variable stored by each derivative.

• weights can be reduced to uint32 as mentioned in the global storage packing finding
• maxSlippage can be reduced to uint64, because it only deals with numbers on a scale from 0 (no slippage allowed) to 10^18 (any slippage allowed), which needs just 60 bits. Going further this value doesn't need to be stored in each derivative separately, it can be passed in deposit and withdraw functions. It's more flexible this way, keeps the configuration in one place, simplifies the derivatives implementation and makes events cleaner, currently derivatives don't emit anything even though their internal state is changed.

The optimized per-derivative storage fits exactly in 256 bits and takes 1 slot:

struct DerivativeStorage {
    IDerivative derivative;
    uint32 weight;
    uint64 maxSlippage;
}
mapping(uint256 => DerivativeStorage) public derivatives;

Cache the current balance in stake

In the current implementation of stake in https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L73 derivatives[i].balance() is called twice in a row, but its result could be stored in a local variable and reused.

uint256 balance = derivatives[i].balance();
underlyingValue += (derivatives[i].ethPerDerivative(balance) * balance) / 10 ** 18;