Asymmetry contest - 0xepley's results

Lines of code

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

Vulnerability details

Impact

In following line in wstEth: https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L60 we are wrongly assuming that 1sthEth is 1:1 in price with the eth but that is not the case, even currently it is on uniswap as

1 stETH = 1.00185 ETH 

        uint256 minOut = (stEthBal * (10 ** 18 - maxSlippage)) / 10 ** 18;

That is not alot but in future it can vary. So to catter the deposit issue use the mechanism using the ethPerDerivative() function as used in the sfrxEth.sol like following:

        uint256 minOut = (((ethPerDerivative(_amount) * _amount) / 10 ** 18) *(10 ** 18 - maxSlippage)) / 10 ** 18;

at following line: https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L74-L75

Proof of Concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

Calculate it using the ethPerDerivative() function like in SfrxEth.sol

Lines of code

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

Vulnerability details

Impact

In the curve v2 whitepaper

https://classic.curve.fi/files/crypto-pools-paper.pdf

the price oracle mechanism is explained briefly in the “Algorithm for repegging” section. It is reproduced below for convenience. Internally, we have a price oracle given by an exponential moving average (EMA) applied in N-dimensional price space. Suppose that the last reported price is pLast, and the update happened t seconds ago while the half-time of the EMA is T1/2. Then the oracle price p_new is given as

α = 2^(− t / T1/2);
p_new = pLast * (1 - α) + α * p_old // p_old = current price_oracle

Impact

There are no necessary checks are place in code in function:

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

for the staleness check and temperness, due to which in sudden movements, we may end up getting the wrong price.

This can be done by ensuring that the price was updated within a certain limit

// eg. last price update / trade must have been executed within the past hour

uint256 lastPricesTimestamp = ICurveCryptoPool(CURVEPOOL).last_prices_timestamp();
require(block.timestamp - lastPricesTimestamp <= 1 hours, 'stale price');

checking that the last reported price pLast has not deviated too far from the current oracle price p_old.

uint256 lastPrice = ICurveCryptoPool(CURVEPOOL).last_prices();
uint256 oraclePrice = ICurveCryptoPool(CURVEPOOL).price_oracle();
uint256 percentDiff;
// eg. require difference in prices to be within 5%
if (lastPrice > oraclePrice) {
  percentDiff = (lastPrice - oraclePrice) * 1e18 / oraclePrice;
} else {
  percentDiff = (oraclePrice - lastPrice) * 1e18 / oraclePrice;
}
require(percentDiff <= 5e16, 'volatile market');

Tools Used

Manual Review

Recommended Mitigation Steps

Add the above given checks in the function to ensure the best price is received that is not stale or tempered.

Lines of code

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

Vulnerability details

Impact

Price being fetched from uniswap is spot price for the current tick which can be manipulated using the attacks like flashloan attack, rather we should use the TWAP price using the cummulative tick, that is more resistant to flash loan attakcs.

If manipulated price is reveived user can benefit by getting more share in the vault than the real value deposit, and same goes for withdrawl user can withdraw more value than deposited.

Proof of Concept

Historical data is stored as an array of Observations structs. Like their name suggests, Observations store the information observed by the Pool when a trade is executed. Observations are written to memory on a per block basis (if a trade occurs). Using two Observations we can derive the time weighted average price, amongst other things.

First lets look into what each observation instance stores:

struct Observation {
	// the block timestamp of the observation
	uint32 blockTimestamp;
	// the tick accumulator, i.e. tick * time elapsed since the pool was first initialized
	int56 tickCumulative;
	// the seconds per liquidity, i.e. seconds elapsed / max(1, liquidity) since the pool was first initialized
	uint160 secondsPerLiquidityCumulativeX128;
	// whether or not the observation is initialized
	bool initialized;
}

Observation s may be retrieved via the observations method on v3 pools. However, this is not the recommended way to consume oracle data. Instead, prefer observe :

function observe(uint32 [] calldata secondsAgos)
external
view
Copy
returns (int56 [] memory tickCumulatives, uint160 [] memory secondsPerLiquidityCumulativeX128s);

The poolPrice() function can be modified as following to get the twap price, note that passed interval should neither be too small not too large, if too large will introduce the stale price problem

function poolPrice(uint32 twapInterval) private view returns (uint256) {
    //getAddress takes the encoded key
    address rocketTokenRETHAddress = RocketStorageInterface(
        ROCKET_STORAGE_ADDRESS
    ).getAddress(
        keccak256(
            abi.encodePacked("contract.address", "rocketTokenRETH")
        )
    );
    IUniswapV3Factory factory = IUniswapV3Factory(UNI_V3_FACTORY);
    IUniswapV3Pool pool = IUniswapV3Pool(
        // @audit hardcoded fee
        factory.getPool(rocketTokenRETHAddress, W_ETH_ADDRESS, 500)
    );

    uint32[] memory secondsAgos = new uint32[](2);
    secondsAgos[0] = twapInterval;
    secondsAgos[1] = 0;
    (int56[] memory tickCumulatives, ) = pool.observe(secondsAgos);
    int24 tickDifference = int24((tickCumulatives[1] - tickCumulatives[0]) / twapInterval);
    uint160 sqrtPriceX96 = TickMath.getSqrtRatioAtTick(tickDifference);

    return (sqrtPriceX96 * sqrtPriceX96 * 1e18) / FixedPoint96.Q96;
}

Tools Used

Manual Review

Recommended Mitigation Steps

Change the poolPrice() function as recommended above.

Lines of code

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

Vulnerability details

Impact

exactInputSingle(params) function on uniswap router takes in deadline parameter but in code we have declared params as struct as follow:

        ISwapRouter.ExactInputSingleParams memory params = ISwapRouter
            .ExactInputSingleParams({
                tokenIn: _tokenIn,
                tokenOut: _tokenOut,
                fee: _poolFee,
                recipient: address(this),
                amountIn: _amountIn,
                amountOutMinimum: _minOut,
                sqrtPriceLimitX96: 0
            });

But in uniswap docs as we can see the following at : https://docs.uniswap.org/contracts/v3/guides/swaps/single-swaps

   ISwapRouter.ExactInputSingleParams memory params =
            ISwapRouter.ExactInputSingleParams({
                tokenIn: DAI,
                tokenOut: WETH9,
                fee: poolFee,
                recipient: msg.sender,
                deadline: block.timestamp,
                amountIn: amountIn,
                amountOutMinimum: 0,
                sqrtPriceLimitX96: 0
            });

There exist the option to pass the deadline.

As we can see there is no deadline parameter in our code struct. It can caused alot of serious problems:

1.Alice wants to swap eth for safEth 2.The transaction is submitted to the mempool, however, Alice chose a transaction fee that is too low for miners to be interested in including her transaction in a block. The transaction stays pending in the mempool for extended periods, which could be hours, days, weeks, or even longer. 3.When the average gas fee dropped far enough for Alice's transaction to become interesting again for miners to include it, her swap will be executed. In the meantime, the price of ETH could have drastically changed. She will still get 1 ETH but the safEth value of that output might be significantly lower. She has unknowingly performed a bad trade due to the pending transaction she forgot about.

An even worse way this issue can be maliciously exploited is through MEV:

1.The swap transaction is still pending in the mempool. Average fees are still too high for miners to be interested in it. The price of tokens has gone up significantly since the transaction was signed, meaning Alice would receive a lot more ETH when the swap is executed. But that also means that her maximum slippage value (sqrtPriceLimitX96 and minOut ) is outdated and would allow for significant slippage. 2.A MEV bot detects the pending transaction. Since the outdated maximum slippage value now allows for high slippage, the bot sandwiches Alice, resulting in significant profit for the bot and significant loss for Alice.

Also for the same reason it is necessary to provide the user to pass in the slippage to prevent something like this too.

Tools Used

Manual Review

Recommended Mitigation Steps

Pass in the deadline check

Lines of code

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

Vulnerability details

Impact

In the deposit() function of Reth.sol we are using the maxSlippage hardcoded by the operator but instead of this we would have prompted the user to set slippage manually. Similarly in withdraw() function of SfrxEth.sol we are using hardcoded slippage setted by the operator. Also in the withdraw() function of WstETH.sol we are using the pre-definded value of slippage.

The underlying yield tokens price may go down abruptly. If Luna/UST things happen again, users' funds may get locked.

Moreover, the withdrawal of the lidoVault takes a swap from the curve pool. 1 WstEth worth 0.995 ETH at the time of writing.

Given that users' funds would be stuck temporarily while the operator adjust the slippage and it would create panic, investment loss for the users and Reputation loss of the protocol

solidity

    for (uint256 i = 0; i < derivativeCount; i++) {
        // withdraw a percentage of each asset based on the amount of safETH
        uint256 derivativeAmount = (derivatives[i].balance() * _safEthAmount) / safEthTotalSupply; //20*2 => 40/100 = 0.4
        if (derivativeAmount == 0) continue; // if derivative empty ignore
        derivatives[i].withdraw(derivativeAmount);
    }

Moreover as 1 of the protocol will suffer this (https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L174

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

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

and withdraw is in the loop (as shown the code above) so we would have to give high slippage to the other two protocols, which would cause unnecessary fee being dedected from user's investment

Proof of Concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

Make another function in which user will be prompted to add slippage by himself as shown below

solidity function unstake(uint256 _safEthAmount, uint256 maxSlippage) external