Asymmetry contest - koxuan's results

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L72-L75 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L211-L216 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L146-L149

Vulnerability details

Impact

When user stake in SafETH, ETH will be deposited into RETH. Due to an incorrect calculation for underlyingValue from ethForDerivative for RETH, user can overpay or underpay, causing loss of funds to either user or SafETH.

Proof of Concept

In stake for SafETH, underlyingValue of each Derivative, including RETH is derived from ethPerDerivative which gets the Derivative worth in terms of ETH.

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

However for ethPerDerivative in RETH, notice that poolCanDeposit is called with the derivative balance, the amount staked by SafETH in RETH. This is incorrect as it should be the amount of ETH that is staked by user into RETH for the stake call.

    function ethPerDerivative(uint256 _amount) public view returns (uint256) {
        if (poolCanDeposit(_amount))
            return
                RocketTokenRETHInterface(rethAddress()).getEthValue(10 ** 18);
        else return (poolPrice() * 10 ** 18) / (10 ** 18);
    }

In poolCanDeposit, a lower or higher amount compared to user staked ETH into RETH can cause the incorrect pool price to be used, either Rocket Pool or Rocket Token UniV3Pool, causing user to either underpay or overpay ETH when staking in RETH.

        return
            rocketDepositPool.getBalance() + _amount <=
            rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize() &&
            _amount >= rocketDAOProtocolSettingsDeposit.getMinimumDeposit();

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend using user staked amount in RETH when deriving underlyingValue for RETH.

Lines of code

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

Vulnerability details

Impact

In SafETH, unstake will withdraw ETH from all derivatives for user. There is no removeDerivative function. In the event that any derivative were to fail, all funds in SafETH will be stuck as no stakers can unstake in SafETH.

Proof of Concept

In unstake of SafETH, notice that every derivative will be withdrawn to add up to the user specified amount of ETH. In the event that any of the derivative fails, all funds in SafETH will be stuck.

        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;
            if (derivativeAmount == 0) continue; // if derivative empty ignore
            derivatives[i].withdraw(derivativeAmount);
        }

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend implementing a removeDerivative function to remove any derivative that is failing to prevent all funds in SafETH to be stuck for stakers.

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L72-L75 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L111-L117

Vulnerability details

Impact

When staking , ethPerDerivative for SfrxETH is part of the sum that determines the price of the safETH tokens. However, due to an incorrect computation in ethPerDerivative, the returned sum will be higher and hence price of safETH tokens will be higher than it should be, causing loss of funds to user as they receive lesser tokens in return.

Proof of Concept

In staking of safETH, each derivative will call ethPerDerivative to get the value of the derivative in ETH value. SfrxETH is on of the derivative.

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

However, in SfrxETH, notice return ((10 ** 18 * frxAmount) / IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle());. This is incorrect as price_oracle returns how much 1 frxETH is worth in terms of ETH. This can be confirmed by going to https://curve.fi/#/ethereum/swap and seeing the exchange rate of 1 frxETH to ETH. See price_oracle return value.

Even though frxETH currently worths less than ETH, the computation will cause ethPerDerivative for SfrxETH to be more than frxETH amount. Even if frxETH suddenly becomes worth more than ETH, the problem will flip to become ethPerDerivative for SfrxETH is less than frxETH amount.

    function ethPerDerivative(uint256 _amount) public view returns (uint256) {
        uint256 frxAmount = IsFrxEth(SFRX_ETH_ADDRESS).convertToAssets(
            10 ** 18
        );
        return ((10 ** 18 * frxAmount) /
            IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle());
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend using get_virtual_price that gets the worth of 1 ETH in terms of frxETH. See get_virtual_price return value.

    return ((10 ** 18 * frxAmount) /
            (10**18/IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).get_virtual_price()));

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L86-L88 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L72-L75 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L91-L95

Vulnerability details

Impact

In WstEth ethPerDerivative, WstETH.getStETHByWstETH is used to derive the derivative worth in terms of stETH. This is incorrect as the current price of stETH in terms of ETH fluctuates between 0.98 to 1.03 around exchanges, and depegging can always take place which causes the incorrect ETH value of WstEth to be reported. Using stETH as a 1 is to 1 equivalence to ETH will cause user to either overpay or underpay when staking, resulting in loss of funds to either user or SafETH.

Proof of Concept

In ethPerDerivative for WstEth, notice WstETH.getStETHByWstETH is used to derive the derivative worth in terms of stETH.

    function ethPerDerivative(uint256 _amount) public view returns (uint256) {
        return IWStETH(WST_ETH).getStETHByWstETH(10 ** 18);
    }

When staking, the incorrect underlying value will be derived for WstEeth.

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

Same for user staked amount of ETH in RETH.

            uint256 depositAmount = derivative.deposit{value: ethAmount}();
            uint derivativeReceivedEthValue = (derivative.ethPerDerivative(
                depositAmount
            ) * depositAmount) / 10 ** 18;
            totalStakeValueEth += derivativeReceivedEthValue;

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend using Chainlink oracle to derive the price for stETH to ETH and computing the total ETH value of WstEth using the oracle price.

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L70-L81 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L211-L216 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L228-L242

Vulnerability details

Impact

When staking in Rocket Pool and Rocket Pool cannot deposit, UniV3Pool will be used to swap ETH to RETH. However, the ethPerDerivative uses PoolPrice, which uses UniV3Pool.slot0 which is susceptible to price manipulation with flash loan attacks. An attacker can crash Rocket Tokens UniV3Pool with flash loan and gain more SafETH Tokens before swapping back and returning the flash loan, allowing attacker to steal from SafETH.

Proof of Concept

In stake of SafETH, ethPerDerivative is used to get the eth amount worth of each derivative. underlyingValue is later divided by total supply to get the price of SafETH token.

        // Getting underlying value in terms of ETH for each derivative
        for (uint i = 0; i < derivativeCount; i++)
            underlyingValue +=
                (derivatives[i].ethPerDerivative(derivatives[i].balance()) *
                    derivatives[i].balance()) /
                10 ** 18;


        uint256 totalSupply = totalSupply();
        uint256 preDepositPrice; // Price of safETH in regards to ETH
        if (totalSupply == 0)
            preDepositPrice = 10 ** 18; // initializes with a price of 1
        else preDepositPrice = (10 ** 18 * underlyingValue) / totalSupply;

When we look into ethPerDerivative for Reth, if Rocket Pool cannot be deposited, poolPrice() will be used to derive the eth worth of Rocket Pool Derivative.

    function ethPerDerivative(uint256 _amount) public view returns (uint256) {
        if (poolCanDeposit(_amount))
            return
                RocketTokenRETHInterface(rethAddress()).getEthValue(10 ** 18);
        else return (poolPrice() * 10 ** 18) / (10 ** 18);
    }

The issue comes in poolPrice, notice that UniV3Pool.slot0 is used to derive the price for Rocket Tokens. This is susceptible to price manipulation by attackers as described.

1. Attacker flash loans a huge amount of Rocket Tokens, selling them in UniV3Pool of Rocket Tokens in exchange for ETH, crashing the price of it in the process.

2. Attacker uses the swapped ETH to stake in SafETH. Since prices of RETH have been crashed, underlyingValue will be lower. Price of SafETH token will hence be lower. User's ETH can exchange more SafETH and hence more will be minted. Note that staking ETH will rebalance the Rocket Token UniV3Pool.

3. SafETH tokens can be unstaked and since staking ETH in step 2 will rebalance back UniV3Pool, user will gain more ETH. It can be argued that only a proportion will be deposited into RETH but the distribution is not known and hence a large proportion can deposited into Reth. Another way to gain is that SafETH tokens can be exchanged for ETH through liquidity pools. ETH can be returned to flash loan and attacker can gain the remaining ETH that they stole from SafETH.

    function poolPrice() private view returns (uint256) {
        address rocketTokenRETHAddress = RocketStorageInterface(
            ROCKET_STORAGE_ADDRESS
        ).getAddress(
                keccak256(
                    abi.encodePacked("contract.address", "rocketTokenRETH")
                )
            );
        IUniswapV3Factory factory = IUniswapV3Factory(UNI_V3_FACTORY);
        IUniswapV3Pool pool = IUniswapV3Pool(
            factory.getPool(rocketTokenRETHAddress, W_ETH_ADDRESS, 500)
        );
        (uint160 sqrtPriceX96, , , , , , ) = pool.slot0();
        return (sqrtPriceX96 * (uint(sqrtPriceX96)) * (1e18)) >> (96 * 2);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend using twap oracle to get the price instead of UniV3Pool.slot0.

Lines of code

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

Vulnerability details

Impact

When withdrawing from SfrxEth, slippage control is put in place when swapping sFrxEth for Eth. However, due to division before multiplication when calculating minOut, slippage control can be lower than intended, causing slippage that is more than expected.

Proof of Concept

In withdraw of SfrxEth minOut act as the slippage control. Notice that multiplication before division is done which can cause minOut to be lower than intended. This can cause slippage to be more than expected.

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


        IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).exchange(
            1,
            0,
            frxEthBalance,
            minOut
        );

Note: This also applies to deposit in Reth.

            uint256 minOut = ((((rethPerEth * msg.value) / 10 ** 18) *
                ((10 ** 18 - maxSlippage))) / 10 ** 18);

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend multiplying before dividing.

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

Lines of code

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

Vulnerability details

Impact

When staking, msg.value will be divided based on the weights of each derivative before depositing into them. However, due to a rounding down error, there will be leftover ETH in SafEth which will not be refunded, causing loss of funds to user.

Proof of Concept

In stake, the amount to deposit into derivative (ethAmount) is computed with uint256 ethAmount = (msg.value * weight) / totalWeight;. Notice that rounding down error will cause leftovers in SafETH. There is no refund mechanism implemented to return user their leftovers, causing loss of funds to user.

            uint256 ethAmount = (msg.value * weight) / totalWeight;


            // This is slightly less than ethAmount because slippage
            uint256 depositAmount = derivative.deposit{value: ethAmount}();

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend sending back leftovers to users.

        uint256 ethAmountToRefund = address(this).balance;
        (bool sent, ) = address(msg.sender).call{value: ethAmountToRefund}(
            ""
        );
        require(sent, "Failed to refund Ether");