Asymmetry contest - rbserver's results

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/SafEth.sol#L63-L101

Vulnerability details

Impact

The following Reth.ethPerDerivative function returns how much ETH that 10 ** 18 rETH are worth depending on poolCanDeposit(_amount) is true or false, where _amount would be the ETH amount to be deposited.

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

When calling the following SafEth.stake function, underlyingValue += (derivatives[i].ethPerDerivative(derivatives[i].balance()) * derivatives[i].balance()) / 10 ** 18 is executed. If the Reth contract is the corresponding derivative, derivatives[i].balance() is the Reth contract's rETH balance and is not the ETH amount to be deposited. Because the Reth.ethPerDerivative function receives such rETH balance as the input value while expecting the ETH amount to be deposited as the input, the ETH amount that 10 ** 18 rETH are worth returned by derivatives[i].ethPerDerivative(derivatives[i].balance()) can be incorrect. For example, if such rETH balance is 1000e18 that makes poolCanDeposit(_amount) false and the ETH to be deposited is 90e18 that makes poolCanDeposit(_amount) true, calling the Reth.ethPerDerivative function with 1000e18 can return (poolPrice() * 10 ** 18) / (10 ** 18) but actually should return RocketTokenRETHInterface(rethAddress()).getEthValue(10 ** 18).

Similarly, the SafEth.stake function also executes uint derivativeReceivedEthValue = (derivative.ethPerDerivative(depositAmount) * depositAmount) / 10 ** 18. When the Reth contract is the corresponding derivative, depositAmount is the rETH amount minted or swapped from the staked ETH amount and is not the exact ETH amount to be deposited especially because that rETH and ETH are not always one to one. Since the Reth.ethPerDerivative function receives such rETH amount as the input value while expecting the ETH amount to be deposited as the input, the ETH amount that 10 ** 18 rETH are worth returned by derivative.ethPerDerivative(depositAmount) can also be incorrect. For instance, if such minted or swapped rETH amount is 100e18 that makes poolCanDeposit(_amount) false and the ETH to be deposited is 90e18 that makes poolCanDeposit(_amount) true, calling the Reth.ethPerDerivative function with 100e18 can return (poolPrice() * 10 ** 18) / (10 ** 18) but actually should return RocketTokenRETHInterface(rethAddress()).getEthValue(10 ** 18).

When calling the Reth.ethPerDerivative function returns incorrect ETH amount that 10 ** 18 rETH are worth, underlyingValue, preDepositPrice, derivativeReceivedEthValue, and totalStakeValueEth can be incorrect, which can result in an incorrect mintAmount. As a result, the SafEth token amount minted to users can be less than they are entitled to.

Moreover, because depositAmount can be much smaller than derivatives[i].balance(), it is possible that calling derivative.ethPerDerivative(depositAmount) makes poolCanDeposit(_amount) true and calling derivatives[i].ethPerDerivative(derivatives[i].balance()) makes poolCanDeposit(_amount) false. This would cause the SafEth.stake function to depend on different and inconsistent ETH amounts that 10 ** 18 rETH are worth, which can also cause mintAmount to be calculated incorrectly.

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

    function stake() external payable {
        ...
        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);
        ...
    }

Proof of Concept

The following steps can occur for the described scenario.

1. The Reth contract is the only derivative so far.
2. A user calls the SafEth.stake function to stake 9e18 ETH.
3. When executing underlyingValue += (derivatives[i].ethPerDerivative(derivatives[i].balance()) * derivatives[i].balance()) / 10 ** 18, derivatives[i].balance() can be 1000e18 rETH. This can make poolCanDeposit(_amount) false when calling the Reth.ethPerDerivative function.
4. When executing uint derivativeReceivedEthValue = (derivative.ethPerDerivative(depositAmount) * depositAmount) / 10 ** 18, depositAmount can be 10e18 rETH. This can make poolCanDeposit(_amount) true when calling the Reth.ethPerDerivative function.
5. Because poolCanDeposit(_amount) is false in step 3 but true in step 4, calling the Reth.ethPerDerivative function return different and inconsistent ETH amounts that 10 ** 18 rETH are worth in these steps. This eventually causes mintAmount to be calculated incorrectly.
6. If the Reth.ethPerDerivative function can be called with 9e18, which is the ETH amount to be deposited, as its input value in both steps 3 and 4, poolCanDeposit(_amount) would be true and the corresponding ETH amounts that 10 ** 18 rETH are worth would be the same in both steps. The resulting mintAmount would be correct and can be more than such mintAmount in step 5.

Tools Used

VSCode

Recommended Mitigation Steps

The SafEth.stake function can be updated to call the derivative contract's ethPerDerivative function with the ETH amount to be deposited, instead of derivatives[i].balance() and depositAmount, as the input.

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L165-L175 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L108-L129 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L56-L67 https://etherscan.io/address/0xDC24316b9AE028F1497c275EB9192a3Ea0f67022#code#L441

Vulnerability details

Impact

Calling the following SafEth.adjustWeight function can update the weight for an existing derivative to 0. However, there is no way to remove an existing derivative. If the external contracts that an existing derivative depends on malfunction or get hacked, this protocol's functionalities that need to loop through the existing derivatives can behave unexpectedly. Users can fail to unstake and lose their deserved ETH as one of the severest consequences.

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

    function adjustWeight(
        uint256 _derivativeIndex,
        uint256 _weight
    ) external onlyOwner {
        weights[_derivativeIndex] = _weight;
        uint256 localTotalWeight = 0;
        for (uint256 i = 0; i < derivativeCount; i++)
            localTotalWeight += weights[i];
        totalWeight = localTotalWeight;
        emit WeightChange(_derivativeIndex, _weight);
    }

For example, calling the following SafEth.unstake function would loop through all of the existing derivatives and call the corresponding derivative's withdraw function. When the WstEth contract is one of these derivatives, the WstEth.withdraw function would be called, which further calls IStEthEthPool(LIDO_CRV_POOL).exchange(1, 0, stEthBal, minOut). If self.is_killed in the stETH-ETH pool contract corresponding to LIDO_CRV_POOL becomes true, especially after such pool contract becomes compromised or hacked, calling such exchange function would always revert. In this case, calling the SafEth.unstake function reverts even though all other derivatives that are not the WstEth contract are still working fine. Because the SafEth.unstake function is DOS'ed, users cannot unstake and withdraw ETH that they are entitled to.

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

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

https://etherscan.io/address/0xDC24316b9AE028F1497c275EB9192a3Ea0f67022#code#L441

def exchange(i: int128, j: int128, dx: uint256, min_dy: uint256) -> uint256:
    ...
    assert not self.is_killed  # dev: is killed

Proof of Concept

The following steps can occur for the described scenario.

1. The WstEth contract is one of the existing derivatives. For the WstEth contract, the stETH-ETH pool contract corresponding to LIDO_CRV_POOL has been hacked in which its self.is_killed has been set to true.
2. Alice calls the SafEth.unstake function but such function call reverts because calling the stETH-ETH pool contract's exchange function reverts for the WstEth derivative.
3. Although all other derivatives that are not the WstEth contract are still working fine, Alice is unable to unstake. As a result, she cannot withdraw and loses her deserved ETH.

Tools Used

VSCode

Recommended Mitigation Steps

The SafEth contract can be updated to add a function, which would be only callable by the trusted admin, for removing an existing derivative that already malfunctions or is untrusted.

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L111-L117 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L60-L88 https://etherscan.io/address/0xa1F8A6807c402E4A15ef4EBa36528A3FED24E577#code#L516

Vulnerability details

Impact

The following SfrxEth.ethPerDerivative function returns how much frxETH that 10 ** 18 sfrxETH are worth.

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

In the following SfrxEth.withdraw function, since minOut is set to (((ethPerDerivative(_amount) * _amount) / 10 ** 18) * (10 ** 18 - maxSlippage)) / 10 ** 18, minOut is denominated in frxETH instead of ETH. Such minOut is used to call IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).exchange(1, 0, frxEthBalance, minOut) for exchanging frxETH for ETH.

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

    function withdraw(uint256 _amount) external onlyOwner {
        IsFrxEth(SFRX_ETH_ADDRESS).redeem(
            _amount,
            address(this),
            address(this)
        );
        uint256 frxEthBalance = IERC20(FRX_ETH_ADDRESS).balanceOf(
            address(this)
        );
        IsFrxEth(FRX_ETH_ADDRESS).approve(
            FRX_ETH_CRV_POOL_ADDRESS,
            frxEthBalance
        );

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

        IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).exchange(
            1,
            0,
            frxEthBalance,
            minOut
        );
        // solhint-disable-next-line
        (bool sent, ) = address(msg.sender).call{value: address(this).balance}(
            ""
        );
        require(sent, "Failed to send Ether");
    }

For the corresponding frxETH-ETH pool contract's exchange function below, the _min_dy input is the minimum amount of the j input's asset to receive. When calling IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).exchange(1, 0, frxEthBalance, minOut), the asset corresponding to the j input should be ETH and the _min_dy input needs to be the minimum amount of ETH to receive. However, minOut is denominated in frxETH instead of ETH. If 1 frxETH is worth more than 1 ETH at this moment, then _min_dy based on minOut denominated in frxETH is less than _min_dy based on minOut denominated in ETH. Yet, because minOut, which is denominated in frxETH instead of ETH, is used, frxETH can be exchanged to ETH at a very suboptimal exchange rate when IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).exchange(1, 0, frxEthBalance, minOut) is called. As a result, the slippage control becomes incorrect, and users lose ETH when they should not.

https://etherscan.io/address/0xa1F8A6807c402E4A15ef4EBa36528A3FED24E577#code#L516

@payable
@external
@nonreentrant('lock')
def exchange(i: int128, j: int128, _dx: uint256, _min_dy: uint256) -> uint256:
    ...
    @param j Index valie of the coin to recieve
    ...
    @param _min_dy Minimum amount of `j` to receive
    ...
    assert dy >= _min_dy, "Exchange resulted in fewer coins than expected"

    ...

    coin = self.coins[j]
    if coin == 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE:
        raw_call(msg.sender, b"", value=dy)
    else:
        ...
    ...

Proof of Concept

The following steps can occur for the described scenario. 1.The SfrxEth contract is the only derivative so far. 2. Alice calls the SafEth.unstake function, which further calls the SfrxEth.withdraw function. 3. When calling the SfrxEth.withdraw function, minOut is calculated to be 10e18 frxETH. When minOut is 10e18, calling IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).exchange(1, 0, frxEthBalance, minOut) allows 10e18 ETH to be exchanged and eventually sent to Alice. 4. Yet, at this moment, 1 frxETH is worth 1.1 ETH so 10e18 frxETH can be converted to 11e18 ETH. If IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).exchange(1, 0, frxEthBalance, minOut) can be called with minOut being 11e18, such function call should revert if only 10e18 ETH can be exchanged. 5. Because receiving 10e18 ETH is very suboptimal and is a result of the incorrect slippage control, Alice loses ETH when she should not.

Tools Used

VSCode

Recommended Mitigation Steps

The SfrxEth.ethPerDerivative function can be updated to return how much ETH instead of frxETH that 10 ** 18 sfrxETH are worth.

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#L63-L101

Vulnerability details

Impact

The following WstEth.ethPerDerivative function returns how much stETH that 10 ** 18 wstETH are worth.

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

When calling the following SafEth.stake function, uint derivativeReceivedEthValue = (derivative.ethPerDerivative(depositAmount) * depositAmount) / 10 ** 18 is executed. If the WstEth contract is one of the derivatives, then depositAmount is the amount of wstETH that corresponds to the staked ETH amount and derivative.ethPerDerivative(depositAmount) is how much stETH that 10 ** 18 wstETH are worth. Thus, derivativeReceivedEthValue is how much stETH that the staked ETH are worth and is denominated in stETH instead of ETH. If 1 stETH is worth more than 1 ETH, then totalStakeValueEth and mintAmount based on derivativeReceivedEthValue denominated in stETH are less than totalStakeValueEth and mintAmount based on derivativeReceivedEthValue denominated in ETH. Hence, the SafEth amount minted to the user using derivativeReceivedEthValue denominated in stETH is lower than the SafEth amount that could be minted to the user using derivativeReceivedEthValue denominated in ETH. As a result, the SafEth token amount minted to the user is less than she or he is entitled to.

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

    function stake() external payable {
        ...

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

Proof of Concept

The following steps can occur for the described scenario.

1. The WstEth contract is the only derivative so far.
2. Alice calls the SafEth.stake function to stake 10e18 ETH.
3. When calling the SafEth.stake function, depositAmount can be 10e18 wstETH that corresponds to the staked 10e18 ETH.
4. If 0.9 stETH is worth 1 wstETH at this moment, derivative.ethPerDerivative(depositAmount) is 0.9e18 stETH. Then, derivativeReceivedEthValue is evaluated to 0.9e18 * 10e18 / 10 ** 18 = 9e18.
5. When preDepositPrice is 10 ** 18, mintAmount is evaluated to 9e18 * 10 ** 18 / 10 ** 18 = 9e18 so 9e18 SafEth tokens are minted to Alice.
6. However, since 0.9 stETH is worth 1 ETH, derivative.ethPerDerivative(depositAmount) could be converted to 1e18 ETH. Then, derivativeReceivedEthValue could be evaluated to 1e18 * 10e18 / 10 ** 18 = 10e18, and mintAmount could be evaluated to 10e18 * 10 ** 18 / 10 ** 18 = 10e18 so 10e18 SafEth tokens could be minted to Alice.
7. Because 9e18 is less than 10e18, the SafEth token amount minted to Alice is less than she is entitled to.

Tools Used

VSCode

Recommended Mitigation Steps

The WstEth.ethPerDerivative function can be updated to return how much ETH instead of stETH that 10 ** 18 wstETH are worth.

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L56-L67 https://etherscan.io/address/0xDC24316b9AE028F1497c275EB9192a3Ea0f67022#code#L438

Vulnerability details

Impact

In the following WstEth.withdraw function, minOut is denominated in stETH instead of ETH because minOut is set to (stEthBal * (10 ** 18 - maxSlippage)) / 10 ** 18 and is used to call IStEthEthPool(LIDO_CRV_POOL).exchange(1, 0, stEthBal, minOut), which is for exchanging stETH for ETH.

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

As shown below, for the corresponding stETH-ETH pool contract's exchange function, the min_dy input is the minimum amount of the j input's asset to receive; when calling IStEthEthPool(LIDO_CRV_POOL).exchange(1, 0, stEthBal, minOut), the j input's asset should be ETH so the min_dy input needs to be the minimum amount of ETH to receive. However, minOut is denominated in stETH instead of ETH. If 1 stETH is worth more than 1 ETH at the moment, then minOut should be a larger number if it can be converted to ETH. Yet, since minOut, which is denominated in stETH instead of ETH, is used, stETH can be exchanged to ETH at a very suboptimal exchange rate when IStEthEthPool(LIDO_CRV_POOL).exchange(1, 0, stEthBal, minOut) is called. This would cause the slippage control to become incorrect and users to lose ETH when they should not.

https://etherscan.io/address/0xDC24316b9AE028F1497c275EB9192a3Ea0f67022#code#L438

@payable
@external
@nonreentrant('lock')
def exchange(i: int128, j: int128, dx: uint256, min_dy: uint256) -> uint256:
    ...
    @param j Index valie of the coin to recieve
    ...
    @param min_dy Minimum amount of `j` to receive
    ...
    assert dy >= min_dy, "Exchange resulted in fewer coins than expected"
    ...
    if i == 0:
        ...
    else:
        assert msg.value == 0
        assert ERC20(coin).transferFrom(msg.sender, self, dx)
        raw_call(msg.sender, b"", value=dy)
    ...

Proof of Concept

The following steps can occur for the described scenario.

1. The WstEth contract is the only derivative so far.
2. Alice calls the SafEth.unstake function, which further calls the WstEth.withdraw function.
3. When calling the WstEth.withdraw function, minOut is calculated to be 10e18 stETH. Calling IStEthEthPool(LIDO_CRV_POOL).exchange(1, 0, stEthBal, minOut) with minOut being 10e18 allows 10e18 ETH to be exchanged and eventually sent to Alice.
4. However, 1 stETH is worth 1.1 ETH at this moment so 10e18 stETH can be converted to 11e18 ETH. If IStEthEthPool(LIDO_CRV_POOL).exchange(1, 0, stEthBal, minOut) can be called with minOut being 11e18, such function call should revert if only 10e18 ETH can be exchanged.
5. Since receiving 10e18 ETH is very suboptimal and is a result of the incorrect slippage control, Alice loses ETH when she should not.

Tools Used

VSCode

Recommended Mitigation Steps

In the WstEth.withdraw function, minOut can be updated to be converted from stETH to ETH before it can be used for calling IStEthEthPool(LIDO_CRV_POOL).exchange(1, 0, stEthBal, minOut).

Lines of code

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

Vulnerability details

Impact

When calling the following Reth.deposit function, uint256 amountSwapped = swapExactInputSingleHop(W_ETH_ADDRESS, rethAddress(), 500, msg.value, minOut) can be executed. Calling the Reth.swapExactInputSingleHop function below then calls ISwapRouter(UNISWAP_ROUTER).exactInputSingle(params).

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

    function deposit() external payable onlyOwner returns (uint256) {
        ...
        if (!poolCanDeposit(msg.value)) {
            uint rethPerEth = (10 ** 36) / poolPrice();

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

            IWETH(W_ETH_ADDRESS).deposit{value: msg.value}();
            uint256 amountSwapped = swapExactInputSingleHop(
                W_ETH_ADDRESS,
                rethAddress(),
                500,
                msg.value,
                minOut
            );

            return amountSwapped;
        } else {
            ...
        }
    }

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

    function swapExactInputSingleHop(
        address _tokenIn,
        address _tokenOut,
        uint24 _poolFee,
        uint256 _amountIn,
        uint256 _minOut
    ) private returns (uint256 amountOut) {
        IERC20(_tokenIn).approve(UNISWAP_ROUTER, _amountIn);
        ISwapRouter.ExactInputSingleParams memory params = ISwapRouter
            .ExactInputSingleParams({
                tokenIn: _tokenIn,
                tokenOut: _tokenOut,
                fee: _poolFee,
                recipient: address(this),
                amountIn: _amountIn,
                amountOutMinimum: _minOut,
                sqrtPriceLimitX96: 0
            });
        amountOut = ISwapRouter(UNISWAP_ROUTER).exactInputSingle(params);
    }

When ISwapRouter(UNISWAP_ROUTER).exactInputSingle(params) is called, params does not include a deadline for the swap. This means that a staking transaction, which needs the Reth.deposit function to be called, can remain pending in the mempool for a long time, such as due to the accompanying transaction fee being too low. Later, when a miner finally includes such staking transaction, the price of ETH in terms of rETH can become much lower, which would also cause minOut in the Reth.deposit function to become much smaller, at that moment comparing to the time when such transaction was just sent. Because the user cannot directly control her or his own slippage, a much lower minOut means that it is more likely that such staking transaction can be sandwich attacked successfully, which can cause the user to lose some staking value.

Proof of Concept

The following steps can occur for the described scenario.

1. Given that the Reth contract is the only derivative so far, Alice calls the SafEth.stake function, which further calls the Reth.deposit function, to stake 10e18 ETH.
2. Since the transaction fee sent for her staking transaction is too low, Alice's staking transaction remains pending in the mempool for a long time.
3. Later, when a miner finally includes Alice's staking transaction, the price of ETH in terms of rETH is much lower than before due to the turbulent market condition. At this moment, 1 ETH can be worth 0.8 rETH while 1 ETH can be worth 1.2 rETH at the time when Alice just sent her staking transaction.
4. Given that maxSlippage stays at 1e16, minOut is evaluated to (((0.8e18 * 10e18) / 10 ** 18) * (10 ** 18 - 1e16)) / 10 ** 18 = 7.92e18 at this moment. Yet, minOut can be evaluated to (((1.2e18 * 10e18) / 10 ** 18) * (10 ** 18 - 1e16)) / 10 ** 18 = 11.88e18 at the time when Alice's staking transaction was just sent.
5. Because minOut is much lower at the time when the miner includes Alice's staking transaction, a malicious actor is more likely to successfully sandwich attack Alice's staking transaction. When such sandwich attack indeed happens and succeeds, Alice loses some staking value.

Tools Used

VSCode

Recommended Mitigation Steps

The SafEth.stake, Reth.deposit, and Reth.swapExactInputSingleHop functions can be updated to allow users to specify and use their own swap deadline for staking. Like Uniswap's SwapRouter contract's functions that check swap deadline, the Reth.deposit or Reth.swapExactInputSingleHop function can be further updated to revert if block.timestamp has passed the specified swap deadline.

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#L156-L204 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L120-L150 https://github.com/rocket-pool/rocketpool/blob/master/contracts/contract/deposit/RocketDepositPool.sol#L74-L91

Vulnerability details

Impact

Calling the following SafEth.stake function would call derivative.deposit{value: ethAmount}(). If the corresponding derivative is the Reth contract, the Reth.deposit function would be called. When poolCanDeposit(msg.value) is true, rocketDepositPool.deposit{value: msg.value}() would be called.

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

    function stake() external payable {
        ...
        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}();
            ...
        }
        ...
    }

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

    function deposit() external payable onlyOwner returns (uint256) {
        ...
        if (!poolCanDeposit(msg.value)) {
            uint rethPerEth = (10 ** 36) / poolPrice();

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

            IWETH(W_ETH_ADDRESS).deposit{value: msg.value}();
            uint256 amountSwapped = swapExactInputSingleHop(
                W_ETH_ADDRESS,
                rethAddress(),
                500,
                msg.value,
                minOut
            );

            return amountSwapped;
        } else {
            address rocketTokenRETHAddress = RocketStorageInterface(
                ROCKET_STORAGE_ADDRESS
            ).getAddress(
                    keccak256(
                        abi.encodePacked("contract.address", "rocketTokenRETH")
                    )
                );
            RocketTokenRETHInterface rocketTokenRETH = RocketTokenRETHInterface(
                rocketTokenRETHAddress
            );
            uint256 rethBalance1 = rocketTokenRETH.balanceOf(address(this));
            rocketDepositPool.deposit{value: msg.value}();
            ...
        }
    }

Comparing the Reth.poolCanDeposit and RocketDepositPool.deposit functions below, when the Reth.poolCanDeposit function returns true, calling the RocketDepositPool.deposit function can still revert if rocketDAOProtocolSettingsDeposit.getDepositEnabled() is false.

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) {
        ...
        return
            rocketDepositPool.getBalance() + _amount <=
            rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize() &&
            _amount >= rocketDAOProtocolSettingsDeposit.getMinimumDeposit();
    }

https://github.com/rocket-pool/rocketpool/blob/master/contracts/contract/deposit/RocketDepositPool.sol#L74-L91

    function deposit() override external payable onlyThisLatestContract {
        // Check deposit settings
        RocketDAOProtocolSettingsDepositInterface rocketDAOProtocolSettingsDeposit = RocketDAOProtocolSettingsDepositInterface(getContractAddress("rocketDAOProtocolSettingsDeposit"));
        require(rocketDAOProtocolSettingsDeposit.getDepositEnabled(), "Deposits into Rocket Pool are currently disabled");
        require(msg.value >= rocketDAOProtocolSettingsDeposit.getMinimumDeposit(), "The deposited amount is less than the minimum deposit size");
        RocketVaultInterface rocketVault = RocketVaultInterface(getContractAddress("rocketVault"));
        require(rocketVault.balanceOf("rocketDepositPool").add(msg.value) <= rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize(), "The deposit pool size after depositing exceeds the maximum size");
        ...
    }

Thus, when poolCanDeposit(msg.value) is true in the Reth.deposit function and rocketDAOProtocolSettingsDeposit.getDepositEnabled() is false in the RocketDepositPool.deposit function, calling rocketDepositPool.deposit{value: msg.value}() in the Reth.deposit function would revert. In this case, the corresponding rETH deposit pool does not allow more deposits, and the protocol should swap the staked ETH's WETH amount for rETH using Uniswap when staking; however, calling the SafEth.stake function, which further calls the Reth.deposit function, would always revert. As a result, the staking functionality is DOS'ed and users cannot stake anymore.

Proof of Concept

The following steps can occur for the described scenario.

1. The Reth contract is one of the derivatives.
2. rocketDAOProtocolSettingsDeposit.getDepositEnabled() is false in the RocketDepositPool.deposit function at this moment.
3. Alice calls the SafEth.stake function to stake an ETH amount that would make the Reth.poolCanDeposit function return true.
4. Calling the SafEth.stake function eventually calls rocketDepositPool.deposit{value: msg.value}() but such function call reverts.
5. Alice calls the SafEth.stake function again to stake a different ETH amount that would also make the Reth.poolCanDeposit function return true but such function call still reverts.
6. As long as rocketDAOProtocolSettingsDeposit.getDepositEnabled() remains false in the RocketDepositPool.deposit function, users, including Alice, cannot stake using this protocol.

Tools Used

VSCode

Recommended Mitigation Steps

The Reth.poolCanDeposit function can be updated to additionally check if rocketDAOProtocolSettingsDeposit.getDepositEnabled() is true. The Reth.poolCanDeposit function should only return true if rocketDepositPool.getBalance() + _amount <= rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize(), _amount >= rocketDAOProtocolSettingsDeposit.getMinimumDeposit(), and rocketDAOProtocolSettingsDeposit.getDepositEnabled() are all true.

Judge has assessed an item in Issue #706 as 2 risk. The relevant finding follows:

[02] USERS CANNOT SET OWN SLIPPAGE WHEN STAKING AND UNSTAKING Only the owner of the SafEth contract can call the following SafEth.setMaxSlippage function to set maxSlippage that is used in the corresponding derivative contract. This means that users cannot set their own slippage when staking and unstaking.

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

function setMaxSlippage(
    uint _derivativeIndex,
    uint _slippage
) external onlyOwner {
    derivatives[_derivativeIndex].setMaxSlippage(_slippage);
    emit SetMaxSlippage(_derivativeIndex, _slippage);
}

When calling the following SafEth.stake function, derivative.deposit{value: ethAmount}() is called. If the corresponding derivative contract's deposit function is like the Reth.deposit function that swaps the deposited ETH's corresponding WETH amount for the derivative tokens according to maxSlippage, the swapped derivative token amount can be less than expected to the user because she or he cannot specify her or his own maxSlippage against the potential price manipulation during the swap. This can then cause a less-than-expected SafEth token amount to be minted to the user.

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

function stake() external payable {
    ...
    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);
    ...
}

When calling the following SafEth.unstake function, derivatives[i].withdraw(derivativeAmount) is called. If the corresponding derivative contract's withdraw function is like the SfrxEth.withdraw or WstEth.withdraw function that exchanges the withdrawn derivative token amount for ETH according to maxSlippage, the exchanged ETH amount can be less than expected to the user because she or he cannot specify her or his own maxSlippage against the potential price manipulation during the exchange. This can then cause a less-than-expected ETH amount to be transferred to the user. Moreover, when the derivative token's market crashes, a user might want to unstake and withdraw ETH as fast as possible to avoid more losses. However, because she or he cannot specify her or his own slippage control, calling the SafEth.unstake function, which calls derivatives[i].withdraw(derivativeAmount), can revert due to that maxSlippage set by the owner of the SafEth contract is too low. In this case, such user cannot unstake and withdraw her or his ETH in a timely manner and loses more ETH as time moves on.

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

function unstake(uint256 _safEthAmount) external {
    ...
    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");
    ...
}

As a mitigation, the SafEth.stake and SafEth.unstake functions can be updated to include an input for specifying maxSlippage. The derivative contracts' deposit and withdraw functions can also be updated to include an input that corresponds to maxSlippage from the SafEth.stake and SafEth.unstake functions. These changes would allow users to set their own slippage when staking and unstaking.

QA REPORT

[01] LACK OF LIMITS FOR SETTING maxSlippage IN Reth, SfrxEth, AND WstEth CONTRACTS

When calling the following Reth.setMaxSlippage, SfrxEth.setMaxSlippage, and WstEth.setMaxSlippage functions, there are no limits for setting maxSlippage in the Reth, SfrxEth, and WstEth contracts.

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

    function setMaxSlippage(uint256 _slippage) external onlyOwner {
        maxSlippage = _slippage;
    }

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

    function setMaxSlippage(uint256 _slippage) external onlyOwner {
        maxSlippage = _slippage;
    }

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

    function setMaxSlippage(uint256 _slippage) external onlyOwner {
        maxSlippage = _slippage;
    }

If maxSlippage are set to 10 ** 18, calling the following Reth.deposit, SfrxEth.withdraw, and WstEth.withdraw functions will cause minOut to be 0 and result in no slippage control. If maxSlippage are set to be more than 10 ** 18, calling the Reth.deposit, SfrxEth.withdraw, and WstEth.withdraw functions can revert because 10 ** 18 - maxSlippage would underflow.

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

    function deposit() external payable onlyOwner returns (uint256) {
        ...
        if (!poolCanDeposit(msg.value)) {
            ...
            uint256 minOut = ((((rethPerEth * msg.value) / 10 ** 18) *
                ((10 ** 18 - maxSlippage))) / 10 ** 18);
            ...
            uint256 amountSwapped = swapExactInputSingleHop(
                W_ETH_ADDRESS,
                rethAddress(),
                500,
                msg.value,
                minOut
            );
            ...
        } else {
            ...
        }
    }

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

    function withdraw(uint256 _amount) external onlyOwner {
        ...
        uint256 minOut = (((ethPerDerivative(_amount) * _amount) / 10 ** 18) *
            (10 ** 18 - maxSlippage)) / 10 ** 18;

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

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

    function withdraw(uint256 _amount) external onlyOwner {
        ...
        uint256 minOut = (stEthBal * (10 ** 18 - maxSlippage)) / 10 ** 18;
        IStEthEthPool(LIDO_CRV_POOL).exchange(1, 0, stEthBal, minOut);
        ...
    }

As a mitigation, to prevent the Reth.deposit, SfrxEth.withdraw, and WstEth.withdraw functions from behaving unexpectedly, the Reth.setMaxSlippage, SfrxEth.setMaxSlippage, and WstEth.setMaxSlippage functions can be updated to only allow maxSlippage to be set to values that cannot exceed certain limits, which are reasonable values that are less than 10 ** 18.

[02] USERS CANNOT SET OWN SLIPPAGE WHEN STAKING AND UNSTAKING

Only the owner of the SafEth contract can call the following SafEth.setMaxSlippage function to set maxSlippage that is used in the corresponding derivative contract. This means that users cannot set their own slippage when staking and unstaking.

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

    function setMaxSlippage(
        uint _derivativeIndex,
        uint _slippage
    ) external onlyOwner {
        derivatives[_derivativeIndex].setMaxSlippage(_slippage);
        emit SetMaxSlippage(_derivativeIndex, _slippage);
    }

When calling the following SafEth.stake function, derivative.deposit{value: ethAmount}() is called. If the corresponding derivative contract's deposit function is like the Reth.deposit function that swaps the deposited ETH's corresponding WETH amount for the derivative tokens according to maxSlippage, the swapped derivative token amount can be less than expected to the user because she or he cannot specify her or his own maxSlippage against the potential price manipulation during the swap. This can then cause a less-than-expected SafEth token amount to be minted to the user.

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

    function stake() external payable {
        ...
        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);
        ...
    }

When calling the following SafEth.unstake function, derivatives[i].withdraw(derivativeAmount) is called. If the corresponding derivative contract's withdraw function is like the SfrxEth.withdraw or WstEth.withdraw function that exchanges the withdrawn derivative token amount for ETH according to maxSlippage, the exchanged ETH amount can be less than expected to the user because she or he cannot specify her or his own maxSlippage against the potential price manipulation during the exchange. This can then cause a less-than-expected ETH amount to be transferred to the user. Moreover, when the derivative token's market crashes, a user might want to unstake and withdraw ETH as fast as possible to avoid more losses. However, because she or he cannot specify her or his own slippage control, calling the SafEth.unstake function, which calls derivatives[i].withdraw(derivativeAmount), can revert due to that maxSlippage set by the owner of the SafEth contract is too low. In this case, such user cannot unstake and withdraw her or his ETH in a timely manner and loses more ETH as time moves on.

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

    function unstake(uint256 _safEthAmount) external {
        ...
        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");
        ...
    }

As a mitigation, the SafEth.stake and SafEth.unstake functions can be updated to include an input for specifying maxSlippage. The derivative contracts' deposit and withdraw functions can also be updated to include an input that corresponds to maxSlippage from the SafEth.stake and SafEth.unstake functions. These changes would allow users to set their own slippage when staking and unstaking.

[03] ETH CAN BE SENT TO SafEth, Reth, SfrxEth, AND WstEth CONTRACTS ACCIDENTALLY

The following receive functions do not prevent users from accidentally sending ETH to the SafEth, Reth, SfrxEth, and WstEth contracts. To prevent users from losing ETH in this way, each of the following receive functions can be updated to check if msg.sender is the sender contract where the SafEth, Reth, SfrxEth, or WstEth contract should receive ETH from; if msg.sender is not such sender contract, calling such receive function should revert. For example, the Reth.receive function can be updated to check if msg.sender is the corresponding rETH contract; if not, calling the Reth.receive function should revert.

contracts\SafEth\SafEth.sol
  246: receive() external payable {}

contracts\SafEth\derivatives\Reth.sol
  244: receive() external payable {}

contracts\SafEth\derivatives\SfrxEth.sol
  126: receive() external payable {}

contracts\SafEth\derivatives\WstEth.sol
  97: receive() external payable {}

[04] USERS' ACCIDENTALLY SENT frxETH AND stETH CAN BE LOST

When the following SfrxEth.withdraw function is called, the SfrxEth contract's balance of frxETH is exchanged for ETH, which is eventually withdrawn to the user. Yet, it is possible that other users accidentally send frxETH to the SfrxEth contract, and such frxETH amount becomes a part of the SfrxEth contract's balance of frxETH. Calling the SfrxEth.withdraw function will cause these other users to lose such accidentally sent frxETH.

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

    function withdraw(uint256 _amount) external onlyOwner {
        IsFrxEth(SFRX_ETH_ADDRESS).redeem(
            _amount,
            address(this),
            address(this)
        );
        uint256 frxEthBalance = IERC20(FRX_ETH_ADDRESS).balanceOf(
            address(this)
        );
        IsFrxEth(FRX_ETH_ADDRESS).approve(
            FRX_ETH_CRV_POOL_ADDRESS,
            frxEthBalance
        );

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

        IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).exchange(
            1,
            0,
            frxEthBalance,
            minOut
        );
        // solhint-disable-next-line
        (bool sent, ) = address(msg.sender).call{value: address(this).balance}(
            ""
        );
        require(sent, "Failed to send Ether");
    }

Similarly, when the following WstEth.withdraw function is called, the WstEth contract's balance of stETH is exchanged for ETH, which is eventually withdrawn to the user. However, it is possible that other users accidentally send stETH to the WstEth contract, and such stETH amount becomes a part of the WstEth contract's balance of stETH. Calling the WstEth.withdraw function will cause these other users to lose such accidentally sent stETH.

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

As a mitigation, to follow the similar pattern used in functions like WstEth.deposit below, the SfrxEth.withdraw function can be updated to keep track of the SfrxEth contract's balances of frxETH before and after IsFrxEth(SFRX_ETH_ADDRESS).redeem(_amount, address(this), address(this) is called and only exchange the difference between such frxETH balances for ETH; similarly, the WstEth.withdraw function can be updated to record the WstEth contract's balances of stETH before and after IWStETH(WST_ETH).unwrap(_amount) is called and only exchange the difference between such stETH balances for ETH. Moreover, functions, which are only callable by the trusted admin, can be added in the SfrxEth and WstEth contracts for transferring ERC20 tokens that are accidentally sent by users back to the corresponding users.

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

    function deposit() external payable onlyOwner returns (uint256) {
        uint256 wstEthBalancePre = IWStETH(WST_ETH).balanceOf(address(this));
        // solhint-disable-next-line
        (bool sent, ) = WST_ETH.call{value: msg.value}("");
        require(sent, "Failed to send Ether");
        uint256 wstEthBalancePost = IWStETH(WST_ETH).balanceOf(address(this));
        uint256 wstEthAmount = wstEthBalancePost - wstEthBalancePre;
        return (wstEthAmount);
    }

[05] 3RD-PARTY CONTRACT ADDRESSES ARE HARDCODED

The 3rd-party contract addresses are hardcoded in the Reth, SfrxEth, and WstEth contracts. If the 3rd-party protocols upgrade these contracts to new addresses or if this protocol wants to launch in a new chain where the hardcoded addresses do not correspond to the needed contracts (for instance, 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2 is not the WETH address on Optimism as shown by https://docs.uniswap.org/contracts/v3/reference/deployments), calling functions that rely on these hardcoded addresses can revert or return or use values that are no longer reliable.

As a mitigation, instead of hardcoding these 3rd-party contract addresses in the Reth, SfrxEth, and WstEth contracts, functions that are only callable by the trusted admin can be added for setting and updating these 3rd-party contract addresses.

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

    address public constant ROCKET_STORAGE_ADDRESS =
        0x1d8f8f00cfa6758d7bE78336684788Fb0ee0Fa46;
    address public constant W_ETH_ADDRESS =
        0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;
    address public constant UNISWAP_ROUTER =
        0x68b3465833fb72A70ecDF485E0e4C7bD8665Fc45;
    address public constant UNI_V3_FACTORY =
        0x1F98431c8aD98523631AE4a59f267346ea31F984;

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

    address public constant SFRX_ETH_ADDRESS =
        0xac3E018457B222d93114458476f3E3416Abbe38F;
    address public constant FRX_ETH_ADDRESS =
        0x5E8422345238F34275888049021821E8E08CAa1f;
    address public constant FRX_ETH_CRV_POOL_ADDRESS =
        0xa1F8A6807c402E4A15ef4EBa36528A3FED24E577;
    address public constant FRX_ETH_MINTER_ADDRESS =
        0xbAFA44EFE7901E04E39Dad13167D089C559c1138;

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

    address public constant WST_ETH =
        0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0;
    address public constant LIDO_CRV_POOL =
        0xDC24316b9AE028F1497c275EB9192a3Ea0f67022;
    address public constant STETH_TOKEN =
        0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84;

[06] MISSING address(0) CHECKS FOR CRITICAL ADDRESS INPUTS

To prevent unintended behaviors, critical address inputs should be checked against address(0). address(0) checks are missing for the _owner input variables in the following initialize functions. Please consider checking them.

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

    function initialize(address _owner) external initializer {
        _transferOwnership(_owner);
        ...
    }

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

    function initialize(address _owner) external initializer {
        _transferOwnership(_owner);
        ...
    }

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

    function initialize(address _owner) external initializer {
        _transferOwnership(_owner);
        ...
    }

[07] VULNERABILITIES IN VERSION 4.8.0 OF @openzeppelin/contracts AND VERSION 4.8.1 OF @openzeppelin/contracts-upgradeable

As shown in the following code in package.json, version 4.8.0 of @openzeppelin/contracts and version 4.8.1 of @openzeppelin/contracts-upgradeable can be used. As described in https://security.snyk.io/package/npm/@openzeppelin%2Fcontracts/4.8.0 and https://security.snyk.io/package/npm/@openzeppelin%2Fcontracts-upgradeable/4.8.1, these versions are vulnerable to incorrect calculation for minting NFTs in batches. To reduce the potential attack surface and be more future-proofed, please consider upgrading this package to at least version 4.8.2.

https://github.com/code-423n4/2023-03-asymmetry/blob/main/package.json#L82-L83

    "@openzeppelin/contracts": "^4.8.0",
    "@openzeppelin/contracts-upgradeable": "^4.8.1",

[08] SOLIDITY VERSION 0.8.19 CAN BE USED

Using the more updated version of Solidity can enhance security. As described in https://github.com/ethereum/solidity/releases, Version 0.8.19 is the latest version of Solidity, which "contains a fix for a long-standing bug that can result in code that is only used in creation code to also be included in runtime bytecode". To be more secured and more future-proofed, please consider using Version 0.8.19 for the following contracts.

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/VaultToken.sol#L2

contracts\SafEth\SafEth.sol
  2: pragma solidity ^0.8.13;

contracts\SafEth\derivatives\Reth.sol
  2: pragma solidity ^0.8.13;

contracts\SafEth\derivatives\SfrxEth.sol
  2: pragma solidity ^0.8.13;

contracts\SafEth\derivatives\WstEth.sol
  2: pragma solidity ^0.8.13;

[09] rebalanceToWeights FUNCTION DOES NOT NEED TO LOOP THROUGH DERIVATIVES WHEN ethAmountToRebalance == 0 IS TRUE

In the following rebalanceToWeights function, when ethAmountToRebalance == 0 is true, none of the derivatives need to be rebalanced. Thus, the following rebalanceToWeights function can be refactored to not loop through the derivatives when ethAmountToRebalance == 0 is true to make the code more efficient.

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

    function rebalanceToWeights() external onlyOwner {
        uint256 ethAmountBefore = address(this).balance;
        ...
        uint256 ethAmountAfter = address(this).balance;
        uint256 ethAmountToRebalance = ethAmountAfter - ethAmountBefore;

        for (uint i = 0; i < derivativeCount; i++) {
            if (weights[i] == 0 || ethAmountToRebalance == 0) continue;
            uint256 ethAmount = (ethAmountToRebalance * weights[i]) /
                totalWeight;
            // Price will change due to slippage
            derivatives[i].deposit{value: ethAmount}();
        }
        emit Rebalanced();
    }

[10] adjustWeight FUNCTION DOES NOT NEED TO LOOP THROUGH DERIVATIVES AND INCREASE localTotalWeight FOR UPDATING totalWeight

Calling the following adjustWeight function can update totalWeight. Yet, this function does not need to loop through the derivatives and increase localTotalWeight for updating totalWeight. To improve efficiency, this function can be updated to decrease totalWeight by the old weights[_derivativeIndex] and then increase totalWeight by the new _weight.

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

    function adjustWeight(
        uint256 _derivativeIndex,
        uint256 _weight
    ) external onlyOwner {
        weights[_derivativeIndex] = _weight;
        uint256 localTotalWeight = 0;
        for (uint256 i = 0; i < derivativeCount; i++)
            localTotalWeight += weights[i];
        totalWeight = localTotalWeight;
        emit WeightChange(_derivativeIndex, _weight);
    }

[11] addDerivative FUNCTION DOES NOT NEED TO LOOP THROUGH DERIVATIVES AND INCREASE localTotalWeight FOR UPDATING totalWeight

When the following addDerivative function is called, the derivatives are looped through and localTotalWeight is increased to update totalWeight. However, instead of looping through the derivatives and increasing localTotalWeight, this function can be updated to directly increase totalWeight by the new derivative's _weight for higher efficiency.

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

    function addDerivative(
        address _contractAddress,
        uint256 _weight
    ) external onlyOwner {
        derivatives[derivativeCount] = IDerivative(_contractAddress);
        weights[derivativeCount] = _weight;
        derivativeCount++;

        uint256 localTotalWeight = 0;
        for (uint256 i = 0; i < derivativeCount; i++)
            localTotalWeight += weights[i];
        totalWeight = localTotalWeight;
        emit DerivativeAdded(_contractAddress, _weight, derivativeCount);
    }

[12] CONSTANTS CAN BE USED INSTEAD OF MAGIC NUMBERS

( Please note that the following instances are not found in https://gist.github.com/muratkurtulus/c7a89b0ef411b5b96dd8af23ccd95dc4#nc-3-constants-should-be-defined-rather-than-using-magic-numbers. )

To improve readability and maintainability, a constant can be used instead of the magic number. Please consider replacing the magic numbers, such as 10 ** 18, used in the following code with constants.

contracts\SafEth\SafEth.sol
  54: minAmount = 5 * 10 ** 17; // initializing with .5 ETH as minimum    
  55: maxAmount = 200 * 10 ** 18; // initializing with 200 ETH as maximum    
  75: 10 ** 18;   
  80: preDepositPrice = 10 ** 18; // initializes with a price of 1   
  81: else preDepositPrice = (10 ** 18 * underlyingValue) / totalSupply;   
  94: ) * depositAmount) / 10 ** 18;   
  98: uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;   

contracts\SafEth\derivatives\Reth.sol
  44: maxSlippage = (1 * 10 ** 16); // 1% 
  171: uint rethPerEth = (10 ** 36) / poolPrice(); 
  173: uint256 minOut = ((((rethPerEth * msg.value) / 10 ** 18) *  
  174: ((10 ** 18 - maxSlippage))) / 10 ** 18);    
  180: 500,    
  214: RocketTokenRETHInterface(rethAddress()).getEthValue(10 ** 18);  
  215: else return (poolPrice() * 10 ** 18) / (10 ** 18);  
  238: factory.getPool(rocketTokenRETHAddress, W_ETH_ADDRESS, 500) 

contracts\SafEth\derivatives\SfrxEth.sol
  38: maxSlippage = (1 * 10 ** 16); // 1% 
  74: uint256 minOut = (((ethPerDerivative(_amount) * _amount) / 10 ** 18) *  
  75: (10 ** 18 - maxSlippage)) / 10 ** 18;   
  113: 10 ** 18    
  115: return ((10 ** 18 * frxAmount) /    

contracts\SafEth\derivatives\WstEth.sol
  35: maxSlippage = (1 * 10 ** 16); // 1% 
  87: return IWStETH(WST_ETH).getStETHByWstETH(10 ** 18); 

[13] UNNECESSARY OPERATION CAN BE REMOVED

Multiplying 10 ** 18 and then dividing by 10 ** 18 in (poolPrice() * 10 ** 18) / (10 ** 18) in the following ethPerDerivative function is unnecessary. To make the code more efficient, please consider updating (poolPrice() * 10 ** 18) / (10 ** 18) to poolPrice() in the following ethPerDerivative function.

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))
            ...
        else return (poolPrice() * 10 ** 18) / (10 ** 18);
    }

[14] uint256 CAN BE USED INSTEAD OF uint

Both uint and uint256 are used in the protocol's code. In favor of explicitness, please consider using uint256 instead of uint in the following code.

contracts\SafEth\SafEth.sol
  71: for (uint i = 0; i < derivativeCount; i++)    
  84: for (uint i = 0; i < derivativeCount; i++) {    
  140: for (uint i = 0; i < derivativeCount; i++) {    
  147: for (uint i = 0; i < derivativeCount; i++) {    
  203: uint _derivativeIndex,    
  204: uint _slippage

[15] SAME INTERFACE CAN BE USED FOR CALLING SAME FUNCTION

For better code consistency, the same interface can be used for calling the same function.

Both the RocketTokenRETHInterface and IERC20 interfaces are used for calling the balanceOf function in the following code. Please consider using only one of these interfaces.

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

            RocketTokenRETHInterface rocketTokenRETH = RocketTokenRETHInterface(
                rocketTokenRETHAddress
            );
            uint256 rethBalance1 = rocketTokenRETH.balanceOf(address(this));
            ...
            uint256 rethBalance2 = rocketTokenRETH.balanceOf(address(this));

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

    function balance() public view returns (uint256) {
        return IERC20(rethAddress()).balanceOf(address(this));
    }

Both the IWStETH and IERC20 interfaces are used for calling the balanceOf function in the following code. Please consider using only one of these interfaces.

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

        uint256 wstEthBalancePre = IWStETH(WST_ETH).balanceOf(address(this));
        ...
        uint256 wstEthBalancePost = IWStETH(WST_ETH).balanceOf(address(this));

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

    function balance() public view returns (uint256) {
        return IERC20(WST_ETH).balanceOf(address(this));
    }

[16] COMMENT CAN BE MORE DESCRIPTIVE

The following ethPerDerivative function returns how much ETH that 10**18 rETH are worth. For a higher clarity, this function's comment can be updated to Get price of derivative in terms of ETH for 10**18 rETH.

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

    /**
        @notice - Get price of derivative in terms of ETH
        ...
     */
    function ethPerDerivative(uint256 _amount) public view returns (uint256) {
        if (poolCanDeposit(_amount))
            return
                RocketTokenRETHInterface(rethAddress()).getEthValue(10 ** 18);
        else return (poolPrice() * 10 ** 18) / (10 ** 18);
    }

[17] TYPO IN COMMENT

For better code quality, room users amount can be changed to room for users amount in the following comment.

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

        @notice - Check whether or not rETH deposit pool has room users amount

[18] REDUNDANT () CAN BE REMOVED

To improve code readability and quality, the redundant () can be removed.

The redundant () around (10 ** 18 - maxSlippage) in the following code can be removed. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L173-L174

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

The redundant () around rethMinted in the following code can be removed. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L202

            return (rethMinted);

The redundant () around wstEthAmount in the following code can be removed. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L80

        return (wstEthAmount);

[19] UNUSED IMPORTS

The following interfaces are not used in the corresponding contracts. Please consider removing these import statements for better readability and maintainability.

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

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "../interfaces/IWETH.sol";
import "../interfaces/uniswap/ISwapRouter.sol";
import "../interfaces/lido/IWStETH.sol";
import "../interfaces/lido/IstETH.sol";

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

import "../../interfaces/frax/IsFrxEth.sol";

[20] FLOATING PRAGMAS

It is a best practice to lock pragmas instead of using floating pragmas to ensure that contracts are tested and deployed with the intended compiler version. Accidentally deploying contracts with different compiler versions can lead to unexpected risks and undiscovered bugs. Please consider locking pragmas for the following files.

contracts\SafEth\SafEth.sol
  2: pragma solidity ^0.8.13;

contracts\SafEth\derivatives\Reth.sol
  2: pragma solidity ^0.8.13;

contracts\SafEth\derivatives\SfrxEth.sol
  2: pragma solidity ^0.8.13;

contracts\SafEth\derivatives\WstEth.sol
  2: pragma solidity ^0.8.13;

[21] INCOMPLETE NATSPEC COMMENTS

NatSpec comments provide rich code documentation. The following functions miss the @param or @return comments. Please consider completing the NatSpec comments for these functions.

contracts\SafEth\derivatives\Reth.sol
  50: function name() public pure returns (string memory) {   
  66: function rethAddress() private view returns (address) { 
  107: function withdraw(uint256 amount) external onlyOwner {  
  120: function poolCanDeposit(uint256 _amount) private view returns (bool) {  
  156: function deposit() external payable onlyOwner returns (uint256) {   
  211: function ethPerDerivative(uint256 _amount) public view returns (uint256) {  
  221: function balance() public view returns (uint256) {  
  228: function poolPrice() private view returns (uint256) {   

contracts\SafEth\derivatives\SfrxEth.sol
  44: function name() public pure returns (string memory) {   
  51: function setMaxSlippage(uint256 _slippage) external onlyOwner { 
  94: function deposit() external payable onlyOwner returns (uint256) {   
  111: function ethPerDerivative(uint256 _amount) public view returns (uint256) {  
  122: function balance() public view returns (uint256) {  

contracts\SafEth\derivatives\WstEth.sol
  41: function name() public pure returns (string memory) {   
  48: function setMaxSlippage(uint256 _slippage) external onlyOwner { 
  56: function withdraw(uint256 _amount) external onlyOwner { 
  73: function deposit() external payable onlyOwner returns (uint256) { 
  93: function balance() public view returns (uint256) {