Asymmetry contest - rvierdiiev's results

Lines of code

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

Vulnerability details

Impact

This isssue is not about price manipulation. It's about edge case, when while calling SafEth.stake function will receive different Reth.ethPerDerivative amount which will make shares calculations incorrect.

When user stakes then different Reth price can be used for derivative underlying calculation and amount deposited by user.

Proof of Concept

When user wants to deposit into SafEth he can call stake function. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L63-L101

    function stake() external payable {
        require(pauseStaking == false, "staking is paused");
        require(msg.value >= minAmount, "amount too low");
        require(msg.value <= maxAmount, "amount too high");


        uint256 underlyingValue = 0;


        // 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;


        uint256 totalStakeValueEth = 0; // total amount of derivatives worth of ETH in system
        for (uint i = 0; i < derivativeCount; i++) {
            uint256 weight = weights[i];
            IDerivative derivative = derivatives[i];
            if (weight == 0) continue;
            uint256 ethAmount = (msg.value * weight) / totalWeight;


            // This is slightly less than ethAmount because slippage
            uint256 depositAmount = derivative.deposit{value: ethAmount}();
            uint derivativeReceivedEthValue = (derivative.ethPerDerivative(
                depositAmount
            ) * depositAmount) / 10 ** 18;
            totalStakeValueEth += derivativeReceivedEthValue;
        }
        // mintAmount represents a percentage of the total assets in the system
        uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;
        _mint(msg.sender, mintAmount);
        emit Staked(msg.sender, msg.value, mintAmount);
    }

As you can see function calculates underlyingValue which is total eth value that is held by derivative contracts. To calculate underlying amount for derivative we call:

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

This actually multiplies derivative eth price by derivative balance. Later this underlyingValue is used to calculate preDepositPrice of SafEth token. To find eth price of derivative, function calls derivatives[i].ethPerDerivative(derivatives[i].balance()). Let's check this function for Reth. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L211-L216

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

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

    function poolCanDeposit(uint256 _amount) private view returns (bool) {
        address rocketDepositPoolAddress = RocketStorageInterface(
            ROCKET_STORAGE_ADDRESS
        ).getAddress(
                keccak256(
                    abi.encodePacked("contract.address", "rocketDepositPool")
                )
            );
        RocketDepositPoolInterface rocketDepositPool = RocketDepositPoolInterface(
                rocketDepositPoolAddress
            );


        address rocketProtocolSettingsAddress = RocketStorageInterface(
            ROCKET_STORAGE_ADDRESS
        ).getAddress(
                keccak256(
                    abi.encodePacked(
                        "contract.address",
                        "rocketDAOProtocolSettingsDeposit"
                    )
                )
            );
        RocketDAOProtocolSettingsDepositInterface rocketDAOProtocolSettingsDeposit = RocketDAOProtocolSettingsDepositInterface(
                rocketProtocolSettingsAddress
            );


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

As you can see, this function provides price in 2 ways. In case if poolCanDeposit, then it returns price from RocketTokenRETHInterface, otherwise from poolPrice() function. poolCanDeposit function just checks if provided _amount can be deposited to rocket pool(if it's too big or too small). In case when we calculate derivative price for Reth then _amount will be derivatives[i].balance(). And this is important here.

Function SafEth.stake later deposits to derivative contract. In case of Reth, when poolCanDeposit == true then it will deposit to rocket pool directly, otherwise it will swap on uniswap. Then later, after deposit, SafEth.stake calls ethPerDerivative again in order to calculate how much eth were deposited to the derivative contract.

So, it's possible that in one SafEth.stake call, Reth.ethPerDerivative function will use poolPrice() to calculate total underlying value and then Reth.ethPerDerivative function will use RocketTokenRETHInterface to get price.

Example. 1.derivatives[i].balance() for reth is 50_000. 2.rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize() is 160_000 and rocketDepositPool.getBalance() is 120_000. 3.user wants to stake 10_000 eth. 4.when calculating underlying of reth, then poolCanDeposit will retun false, as 50_000 will be provided as amount(120_000 + 50_000 > 160_000), so uniswap pool will be used to get the price. 5.but deposit will come directly to the rocket pool and to calculate amount of eth that was deposited, Reth.ethPerDerivative(10_000) will be called. 120_000 + 10_000 > 160_000, so poolCanDeposit will be true and price will be fetched from RocketTokenRETHInterface. 6.As result different price was used to calculated underlying amount of derivative and eth amount deposited to the rocket pool, because of that SafEth token amount calculation is incorrect.

I believe that main logic here was to use uniswap price in case, when rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize() limit is already reached. But because underlying amount for derivative is calculated with it's total balance, instead of amount that user will deposit to derivative contract, different prices are taken.

Also similar thing with different prices can happen if _amount that user will deposit to Reth is less than rocketDAOProtocolSettingsDeposit.getMinimumDeposit(), but poolCanDeposit(derivatives[i].balance())==true. Then for underlying calculations, RocketTokenRETHInterface price will be used and for deposited amount calculation, uniswap price will be taken.

Recommended Mitigation Steps

I guess, that you need to calculate underlying for derivative contract like this:

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

Here, we provide (msg.value * weights[i]) / totalWeight as amount that user will provide to rocket pool.

Lines of code

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

Vulnerability details

Impact

If any underlying protocol of derivative will be paused/hacked then unstaking will be blocked, because unstake loops through all derivatives in order to withdraw and there is no ability to remove derivative.

Proof of Concept

In case if unstake is called, then function will loop though all derivatives that has balance in order to withdraw part of funds from it. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L108-L129

    function unstake(uint256 _safEthAmount) external {
        require(pauseUnstaking == false, "unstaking is paused");
        uint256 safEthTotalSupply = totalSupply();
        uint256 ethAmountBefore = address(this).balance;


        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);
        }
        _burn(msg.sender, _safEthAmount);
        uint256 ethAmountAfter = address(this).balance;
        uint256 ethAmountToWithdraw = ethAmountAfter - ethAmountBefore;
        // solhint-disable-next-line
        (bool sent, ) = address(msg.sender).call{value: ethAmountToWithdraw}(
            ""
        );
        require(sent, "Failed to send Ether");
        emit Unstaked(msg.sender, ethAmountToWithdraw, _safEthAmount);
    }

In case if any of underlying protocols of derivative will be paused or hacked, so withdraw will revert, then all unstale function will revert and noone will be able to unstake.

The problem here is that protocol doesn't have ability to remove derivative contract. It has ability to change weight to 0, but it's not enough.

Tools Used

VsCode

Recommended Mitigation Steps

Add ability to remove derivative contract.

Lines of code

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

Vulnerability details

Tools Used

VsCode

Recommended Mitigation Steps

You need to convert stEth amount that you can receive for wstEth to eth in order to receive correct amount of eth, received for wstEth. You need to use some stEth/eth oracle price here.

Impact

WstEth.withdraw calculates slippage incorrectly. User can loose more than maxSlippage, because slippage amount is calculated for stEth instead of eth.

Proof of Concept

WstEth.withdraw function unwraps wstEth to stEth, then swaps this stEth to eth. To control minimum amount of eth, it uses maxSlippage param. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L56-L67

    function withdraw(uint256 _amount) external onlyOwner {
        IWStETH(WST_ETH).unwrap(_amount);
        uint256 stEthBal = IERC20(STETH_TOKEN).balanceOf(address(this));
        IERC20(STETH_TOKEN).approve(LIDO_CRV_POOL, stEthBal);
        uint256 minOut = (stEthBal * (10 ** 18 - maxSlippage)) / 10 ** 18;
        IStEthEthPool(LIDO_CRV_POOL).exchange(1, 0, stEthBal, minOut);
        // solhint-disable-next-line
        (bool sent, ) = address(msg.sender).call{value: address(this).balance}(
            ""
        );
        require(sent, "Failed to send Ether");
    }

This is how minimum amount is calculated: uint256 minOut = (stEthBal * (10 ** 18 - maxSlippage)) / 10 ** 18; The problem is that slippage was calculated using stEthBal, which is amount of stEth that will be swapped. But it should be calculated using stEth/eth price as stEth and eth have different price.

If i want to swap 100 steth with 1% slippage, then minOut == 99 eth will be calculated. But 1 steth costs 1.0025 eth, so i expect to receive 100.25 eth after swap actually. If i receive 99 eth instead of 100.25(because i allowed that), then slippage is bigger and user can lose more funds than he expects with maxSlippage. In this case slippage should be calculated as 100.25 * 0.99 = 99.2475.

Tools Used

VsCode

Recommended Mitigation Steps

To calculate correct minimum amount you need to use stEth/eth price and convert stEth to eth and apply slippage to converted amount.

Lines of code

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

Vulnerability details

Impact

WstEth.ethPerDerivative function returns stEth amount per wstEth, but should return eth amount per wstEth. Because of that underlying amount of WstEth contract is calculated incorrectly, which affects SafEth price.

Proof of Concept

To calculate total amount of eth that is controlled by derivative contracts, SafEth.stake function calls ethPerDerivative function for each derevative and multiplies it by balance of derivative.

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

        for (uint i = 0; i < derivativeCount; i++)
            underlyingValue +=
                (derivatives[i].ethPerDerivative(derivatives[i].balance()) *
                    derivatives[i].balance()) /
                10 ** 18;

ethPerDerivative function should return how much eth can be received for derivative token.

The problem is that WstEth.ethPerDerivative function returns how much stEth can be returned for derivative token. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L86-L88

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

This is incorrect as stEth doesn't have same price as eth. Because of that underlying calculation is broken and price per SafEth token is calculated incorrectly. This will have impact on amount of SafEth tokens that depositor will receive.

Lines of code

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

Vulnerability details

Impact

Reth.ethPerDerivative uses uniswap price, which can be manipulated

Proof of Concept

Reth.ethPerDerivative is used to calculate eth amount that can be received for 1 rocket token. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L211-L216

    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 case if _amount cannot be deposited directly to rocket pool, then poolPrice() function is used to get price from uniswap. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L228-L242

    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);
    }

The problem here is that uniswap price can be manipulated by attacker, so this approach is vulnerable to flash loan attack, where attacker can change rocket token price in the pool, to be smaller, than it is now. Then mint bigger amount of SafEth tokens for himself(using stake function), put rocket token price back in the pool, unstake to receive more eth than he has staked and then repay flashloan.

To avoid this problem you need to use oracle.

Tools Used

VsCode

Recommended Mitigation Steps

You need to use oracle price, that can't be manipulated.

Lines of code

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 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L111-L117

Vulnerability details

Impact

Using of amm price to calculate underlying is incorrect.

Proof of Concept

IDerivative.ethPerDerivative function is used to provide amount of eth that can be received for derivative token in case of withdraw.

We will check this function for Reth and SfrxEth derivatives. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L211-L216

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

Inside Reth contract the price is fetched in 2 ways. First way is to get price from RocketTokenRETHInterface. This is correct, because this is real exchange price that user can receive if he withdraws from rocket pool. This price is increasing during the time. Another way which is used when the pool is full is to get the price from uniswap. Price on uniswap is always changing. Because of that underlying amount will be also changing for derivative. But it's not like that. Underlying amount will be always equal to RocketTokenRETHInterface(rethAddress()).getEthValue(balance()).

Same problems exists inside SfrxEth. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L111-L117

    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());
    }

This function also uses market price, which is volatile. It should use frax alternative of RocketTokenRETHInterface instead.

And only WstEth derivative does it right. But he also needs to convert stEth to eth.

The problem of this bug is that underlying amount will be calculated in different ways every time and this can lead to insolvency of the SafEth. But if you will not use market price, then underlying amount will be always same(for same amount and price).

Tools Used

VsCode

Recommended Mitigation Steps

Don't use market prices for calculation of underlying.

Lines of code

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

Vulnerability details

Impact

Withdrawing from Reth derivative can be blocked, because when someone deposits to rocket pool, than it is allowed to withdraw(burn) only after some amount of blocks have passed. This will block unstake from SafEth contract.

Proof of Concept

To withdraw from rocket pool, Reth.withdraw function is called. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L107-L114

    function withdraw(uint256 amount) external onlyOwner {
        RocketTokenRETHInterface(rethAddress()).burn(amount);
        // solhint-disable-next-line
        (bool sent, ) = address(msg.sender).call{value: address(this).balance}(
            ""
        );
        require(sent, "Failed to send Ether");
    }

This function simply, burns its rocket token.

The problem is that inside rocket pool RocketTokenRETH contract, when burn function will be called, then _beforeTokenTransfer function will be called. https://github.com/rocket-pool/rocketpool/blob/master/contracts/contract/token/RocketTokenRETH.sol#L157-L170

    function _beforeTokenTransfer(address from, address, uint256) internal override {
        // Don't run check if this is a mint transaction
        if (from != address(0)) {
            // Check which block the user's last deposit was
            bytes32 key = keccak256(abi.encodePacked("user.deposit.block", from));
            uint256 lastDepositBlock = getUint(key);
            if (lastDepositBlock > 0) {
                // Ensure enough blocks have passed
                uint256 depositDelay = getUint(keccak256(abi.encodePacked(keccak256("dao.protocol.setting.network"), "network.reth.deposit.delay")));
                uint256 blocksPassed = block.number.sub(lastDepositBlock);
                require(blocksPassed > depositDelay, "Not enough time has passed since deposit");
                // Clear the state as it's no longer necessary to check this until another deposit is made
                deleteUint(key);
            }

As you can see, this function checks, when last deposit of from account occured. And then it checks, that some amount of blocks already passed since last deposit. Otherwise it will revert and will not allow to burn tokens.

So every time, when Reth contract deposits to rocket pool, then lastDepositBlock is updated for it and then withdrawals are blocked for depositDelay amount of blocks. As result, users can't withdraw from all derivatives as unstaking is looping through all derivatives.

Also malicious user can stake each depositDelay amount of blocks in order to block withdrawals.

Tools Used

VsCode

Recommended Mitigation Steps

I don't know how to handle this correctly.