Asymmetry contest - CodingNameKiki'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#L156-L204 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L211-L216

Vulnerability details

Derivative Reth prices

A quick explanation of the issue causing it, the problem is based on the function "ethPerDerivative" in the Reth derivative. As you can see two statements can be triggered here, the first one "if (poolCanDeposit(_amount))" checks if the given amount + the pool balance isn't greater than the maximumDepositPoolSize and that the amount is greater than the minimum deposit in the pool. Second statement is meant to return a poolPrice which is slightly more than the regular one, because it's used in order to swap tokens in Uniswap and therefore the price per token is overpriced.

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

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

Below you can see the regular price returned in the first statement - 1063960369075232250:

<img width="417" alt="Screenshot 2023-03-27 at 8 53 34" src="https://user-images.githubusercontent.com/112419701/227852484-9bf0144d-820d-414c-8cb9-e1fb44f5c806.png">

Below you can see the pool price from the second statement, supposed to be used only when a swap is made.

else return (poolPrice() * 10 ** 18) / (10 ** 18);

// poolPrice calculates and returns
uint160 sqrtPriceX96, , , , , , ) = pool.slot0();
        return (sqrtPriceX96 * (uint(sqrtPriceX96)) * (1e18)) >> (96 * 2);

// uint160 sqrtPriceX96 = 81935751724326368909606241317
// return (sqrtPriceX96 * (uint(sqrtPriceX96)) * (1e18)) >> (96 * 2);
// return 1069517062752670179 (pool price)

// The function "ethPerDerivative" for the else statement return (poolPrice() * 10 ** 18) / (10 ** 18);
// Which will be - 1069517062752670179

Difference between the regular price and the pool price:

regular price - 1063960369075232250
pool price -    1069517062752670179

Quick Overview

What can result to users receiving less minted tokens?

The first thing the staking function does is calculating the derivative underlyingValue. This issue occurs on the Reth derivative, as we can see the staking function calls "ethPerDerivative" to get the price, but takes as account the whole Reth balance of the derivative contract.

For example let's say the derivative Reth holds 200e18. The pool has free space for 100e18 more till it reaches its maximum pool size. As the function calls ethPerDerivative with the Reth balance of 200e18 instead of the amount being staked. The contract will think there is no more space in the pool (even tho there is 100e18 more) and will return the pool price which is overpriced and meant for the swap in Uniswap.

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

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

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

Lets follow what actually happens, for now we have wrong overpriced underlying value of the derivative Reth.

Next the function calculates the preDepositPrice. l will do the real calculations in the POC, but its easy to assume that if the underlyingValue is overpriced the preDepositPrice will be too based on the calculation below.

else preDepositPrice = (10 ** 18 * underlyingValue) / totalSupply;

// Lets say the user deposits 5e18

Here comes the real problem, so far the function calculates the local variables as there will be swap to Uniswap. As mentioned in the beginning the pool has 100e18 free space, so in the deposit function in Reth, the swap to Uniswap will be ignored as "poolCanDeposit(msg.value) == true" and the msg.value will be deposited in the rocket pool.

uint256 depositAmount = derivative.deposit{value: ethAmount}();

function deposit() external payable onlyOwner returns (uint256) {
        // Per RocketPool Docs query addresses each time it is used
        address rocketDepositPoolAddress = RocketStorageInterface(
            ROCKET_STORAGE_ADDRESS
        ).getAddress(
                keccak256(
                    abi.encodePacked("contract.address", "rocketDepositPool")
                )
            );

        RocketDepositPoolInterface rocketDepositPool = RocketDepositPoolInterface(
                rocketDepositPoolAddress
            );

        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}();
            uint256 rethBalance2 = rocketTokenRETH.balanceOf(address(this));
            require(rethBalance2 > rethBalance1, "No rETH was minted");
            uint256 rethMinted = rethBalance2 - rethBalance1;
            return (rethMinted);
        }
    }

Next the function calculates the "derivativeReceivedEthValue", this time the function ethPerDerivative(depositAmount) will return the normal price as there is space in the pool. Both "derivativeReceivedEthValue" and "totalStakeValueEth" will be calculated based on the normal price.

uint derivativeReceivedEthValue = (derivative.ethPerDerivative(
                depositAmount
            ) * depositAmount) / 10 ** 18;

totalStakeValueEth += derivativeReceivedEthValue;

If we take the info so far and apply it on the mintAmount calculation below, we know that "totalStakeValueEth" is calculated on the normal price and "preDepositPrice" is calculated on the overpriced pool price. So the user will actually receive less minted shares than he is supposed to get.

uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;

Proof of Concept - Part 1

Will start from the start in order to get the right amounts of "totalSupply" and the Reth balance of derivative. So l can show the issue result in POC Part 2.

// The values below are only made for the example.

Let's say we have two stakers - Bob and Kiki each depositing 100e18. We have only one derivative which is Reth, so it will have 100% weight.

Bob deposits 100e18 as the first depositer and receives (99999999999999999932) minted tokens of safETH.

So far after Bob deposit:

totalSupply = 99999999999999999932

Reth derivative balance = 93988463204618701706

uint256 underlyingValue = 0;
uint256 totalSupply = 0; 
uint256 preDepositPrice = 1e18

// As we have only derivative Reth in the example, it owns all of the weight.
uint256 ethAmount = (msg.value * weight) / totalWeight;
uint256 ethAmount = (100e18 * 1000) / 1000;

// not applying the deposit fee in rocketPool

uint256 depositAmount = derivative.deposit{value: ethAmount}();
uint256 depositAmount = 93988463204618701706

uint derivativeReceivedEthValue = (derivative.ethPerDerivative(depositAmount) * depositAmount) / 10 ** 18;
uint derivativeReceivedEthValue = (1063960369075232250 * 93988463204618701706) / 10 ** 18;
uint derivativeReceivedEthValue = 99999999999999999932

totalStakeValueEth = 99999999999999999932;

 
uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;
uint256 mintAmount = (99999999999999999932 * 10 ** 18) / 1e18;
uint256 mintAmount = 99999999999999999932

Kiki deposits 100e18 as well and receives (99999999999999999932) minted tokens of safEth.

So far after Kiki's deposit:

totalSupply = 199999999999999999864;

Reth derivative balance = 187976926409237403412;

// take the info after bob's deposit and the normal price
underlyingValue  = (derivatives[i].ethPerDerivative(derivatives[i].balance()) * derivatives[i].balance()) / 10 ** 18;
uint256 underlyingValue = (1063960369075232250 * 93988463204618701706) / 10 ** 18;
uint256 underlyingValue = 99999999999999999932;

uint256 totalSupply = 99999999999999999932; 

uint256 preDepositPrice = (10 ** 18 * underlyingValue) / totalSupply;
uint256 preDepositPrice = (10 ** 18 * 99999999999999999932) / 99999999999999999932;
uint256 preDepositPrice = 1e18;


// As we have only derivative Reth in the example, it owns all of the weight.
uint256 ethAmount = (msg.value * weight) / totalWeight;
uint256 ethAmount = (100e18 * 1000) / 1000;

// not applying the deposit fee in rocketPool
uint256 depositAmount = 93988463204618701706

uint derivativeReceivedEthValue = (derivative.ethPerDerivative(depositAmount) * depositAmount) / 10 ** 18;
uint derivativeReceivedEthValue = (1063960369075232250 * 93988463204618701706) / 10 ** 18;
uint derivativeReceivedEthValue = 99999999999999999932

totalStakeValueEth = 99999999999999999932;
 
uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;
uint256 mintAmount = (99999999999999999932 * 10 ** 18) / 1e18;
uint256 mintAmount = 99999999999999999932

Proof of Concept - Part 2

From the first POC, we calculated the outcome of 200e18 staked into the Reth derivative. We got the totalSupply and the Reth balance the derivative holds. So we can move onto the main POC, where l can show the difference and how much less minted tokens the user gets.

totalSupply = 199999999999999999864;
Reth derivative balance = 187976926409237403412;

First l am going to show how much minted tokens the user is supposed to get without applying the issue occurring. And after that l will do the second one and apply the issue. So we can compare the outcomes and see how much less minted tokens the user gets.

Without the issue occurring, a user deposits 5e18 by calling the staking function. The user received (4999549277935239332) minted tokens of safEth.

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


uint256 totalSupply = 199999999999999999864; 

uint256 preDepositPrice = (10 ** 18 * underlyingValue) / totalSupply;
uint256 preDepositPrice = (10 ** 18 * 199999999999999999864) / 199999999999999999864;
uint256 preDepositPrice = 1e18;

// As we have only derivative Reth in the example, it owns all of the weight.
uint256 ethAmount = (msg.value * weight) / totalWeight;
uint256 ethAmount = (5e18 * 1000) / 1000;

// not applying the deposit fee in rocketPool
uint256 depositAmount = 4698999533488942411

uint derivativeReceivedEthValue = (derivative.ethPerDerivative(depositAmount) * depositAmount) / 10 ** 18;
uint derivativeReceivedEthValue = (1063960369075232250 * 4698999533488942411) / 10 ** 18;
uint derivativeReceivedEthValue = 4999549277935239332

totalStakeValueEth = 4999549277935239332;
 
uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;
uint256 mintAmount = (4999549277935239332 * 10 ** 18) / 1e18;
uint256 mintAmount = 4999549277935239332

Stats after the deposit without the issue:

totalSupply = 204999549277935239196
Reth derivative balance = 192675925942726345823;

This time we apply the issue occurring and as the first one a user deposits 5e18 by calling the staking function. The user receives (4973574036557377784) minted tokens of saEth

uint256 underlyingValue  = (derivatives[i].ethPerDerivative(derivatives[i].balance()) * derivatives[i].balance()) / 10 ** 18;
// the function takes as account the pool price here which is overpriced.
uint256 underlyingValue  = (1069517062752670179 * 187976926409237403412) / 10 ** 18;
uint256 underlyingValue  = 201044530198482424206

uint256 totalSupply = 199999999999999999864;

uint256 preDepositPrice = (10 ** 18 * underlyingValue) / totalSupply;
uint256 preDepositPrice = (10 ** 18 * 201044530198482424206) / 199999999999999999864;
uint256 preDepositPrice = 1005222650992412121;

// As we have only derivative Reth in the example, it owns all of the weight.
uint256 ethAmount = (msg.value * weight) / totalWeight;
uint256 ethAmount = (5e18 * 1000) / 1000;

// not applying the deposit fee in rocketPool
uint256 depositAmount = 4698999533488942411

// Here the function calculates based on the normal price, as the pool has free space and the user deposits only 5e18.
uint derivativeReceivedEthValue = (derivative.ethPerDerivative(depositAmount) * depositAmount) / 10 ** 18;
uint derivativeReceivedEthValue = (1063960369075232250 * 4698999533488942411) / 10 ** 18;
uint derivativeReceivedEthValue = 4999549277935239332

totalStakeValueEth = 4999549277935239332;
 
uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;
uint256 mintAmount = (4999549277935239332 * 10 ** 18) / 1005222650992412121;
uint256 mintAmount = 4973574036557377784

Stats after the deposit with the issue:

totalSupply = 204973574036557377648;
Reth derivative balance = 192675925942726345823;

Difference between outcomes:

Without the issue based on 5e18 deposit, the user receives -        4999549277935239332 minted tokens
With the issue occurring based on 5e18 deposit, the user receives - 4973574036557377784 minted tokens

Proof of Concept - Plus

So far we found that this issue leads to users receiving less minted shares, but let's go even further and see how much the user losses in terms of ETH. By unstaking the minted amount.

First we apply the stats without the issue occurring.

totalSupply = 204999549277935239196
Reth derivative balance = 192675925942726345823;

uint256 derivativeAmount = (derivatives[i].balance() * _safEthAmount) / safEthTotalSupply;
uint256 derivativeAmount = (192675925942726345823 * 4999549277935239332) / 204999549277935239196;
uint256 derivativeAmount = 4698999533488942410;

// Eth value based on the current eth price
// Reth to Eth value - 4698999533488942410 => 4.999999999999999998 - 8766.85 usd

Second we apply the stats with the issue occurring.

totalSupply = 204973574036557377648;
Reth derivative balance = 192675925942726345823;

uint256 derivativeAmount = (derivatives[i].balance() * _safEthAmount) / safEthTotalSupply;
uint256 derivativeAmount = (192675925942726345823 * 4973574036557377784) / 204973574036557377648;
uint256 derivativeAmount = 4675178189396666336;

// Eth value based on the current eth price
// Reth to Eth value - 4675178189396666336 => 4.974637740558436705 - 8722.41 usd

Tools Used

Manual review

Recommended Mitigation Steps

The problem occurs with calculating the underlyingValue in the staking function. The function "ethPerDerivative" is called with all of the Reth balance, which should not be the case here. Therefore the function calls "poolCanDeposit" in order to check if the pool has space for the Reth derivative balance (Basically the contract thinks that the Reth balance in the derivative will be deposited in the pool, which is not the case here). So even if the pool has space for the depositing amount by the user, the poolCanDeposit(_amount) will return false and the contract will get the poolPrice of the reth which is supposed to be used only for the swap in Uniswap. The contract process executing the staking function with the overpriced pool price and doesn't perform any swap, but deposits the user funds to the pool.

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

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

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

l'd recommend creating a new function in the reth derivative contract. Which converts the msg.value to reth tokens and using it instead of the whole Reth balance the derivative holds.

function rethValue(uint256 _amount) public view returns (uint256) {
      RocketTokenRETHInterface(rethAddress()).getRethValue(amount);
    }

Like this we check if the msg.value converted into reth tokens is below the maximumPoolDepositSize and greater than the minimum deposit.

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

Lines of code

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

Vulnerability details

Impact

When depositing into the rocket pool isn't possible, the Reth derivative contract is designed to get an amount of Reth tokens by swapping through Uniswap. However the contract doesn't include the scenario where depositing into the rocket pool is paused, so instead of working normally and getting the Reth tokens through Uniswap, the staking function will revert and won't be able to perform depositing for the Reth derivative.

Proof of Concept

If we look over the deposit function in the Reth derivative, we can see that even if the msg.value can't be deposited in the rocket pool duo to the pool hitting maximum deposits or the msg.value is below the minimum deposit amount. The function still gets the msg.value converts it to WETH and calls Uniswap in order to exchange the WETH for an amount of Reth tokens. So the contract is designed to swap tokens through Uniswap in order to get the amount of tokens, even if depositing into the rocket pool doesn't work.

function deposit() external payable onlyOwner returns (uint256) {
        // Per RocketPool Docs query addresses each time it is used
        address rocketDepositPoolAddress = RocketStorageInterface(
            ROCKET_STORAGE_ADDRESS
        ).getAddress(
                keccak256(
                    abi.encodePacked("contract.address", "rocketDepositPool")
                )
            );

        RocketDepositPoolInterface rocketDepositPool = RocketDepositPoolInterface(
                rocketDepositPoolAddress
            );

        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}();
            uint256 rethBalance2 = rocketTokenRETH.balanceOf(address(this));
            require(rethBalance2 > rethBalance1, "No rETH was minted");
            uint256 rethMinted = rethBalance2 - rethBalance1;
            return (rethMinted);
        }
    }

The issue occurring here is that the function "poolCanDeposit" only checks if the amount isn't below the minimum deposit and if the pool balance + amount is less than the maximum deposit pool size. But doesn't check if depositing was paused in the rocket pool, so therefore if depositing is paused in the rocket pool - the staking function in safEth will revert and won't account deposits to the Reth derivative. Even tho there is a way to receive Reth tokens by swapping through Uniswap, the contract doesn't include this kind of scenario and instead of working normally it revert.

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

<img width="1153" alt="Screenshot 2023-03-27 at 12 35 11" src="https://user-images.githubusercontent.com/112419701/227903177-e922dd26-8b8c-48a3-9c07-3c12e59d3c82.png">

Tools Used

Manual review

Recommended Mitigation Steps

Consider allowing swaps in Uniswap to be made, if depositing into the pool is paused. So that the function "stake" can account depositing to Reth derivative instead of reverting.

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

Issues Template

Low Risk Template

Non-Critical Template

Refactor Issues Template

[L-01] minAmount to stake shouldn't be set below the rocket pool minimum deposit, as the function stake will only swap tokens through Uniswap which are slightly overpriced

Basically minAmount is the minimum amount of msg.value that users can provide in order to stake. Currently there are 3 derivatives controlled by the system - Reth, SfrxEth and WstEth. The problem occurs only on the Reth derivative, as rocket pool enforce a minimum deposit, so if the minAmount is below the rocket pool minimum deposit. The function will only try to convert the msg.value to WETH and perform a swap on Uniswap in order to get Reth tokens. Considering the Uniswap pool price is slightly overpriced than the normal price, it will lead to slippage of the msg.value and less minted tokens to the user.

<img width="1222" alt="Screenshot 2023-03-28 at 13 57 56" src="https://user-images.githubusercontent.com/112419701/228215104-957b20bb-c363-4e56-8609-485e5a02f923.png">

Consider applying a minimum limit so the new set minAmount isn't below it:

function setMinAmount(uint256 _minAmount) external onlyOwner {
        require(_minAmount >= rocketDAOProtocolSettingsDeposit.getMinimumDeposit(),"");
        minAmount = _minAmount;
        emit ChangeMinAmount(minAmount);
    }

[L-02] The function "adjustWeight" shouldn't be possible to set an amount of weight on a non-existing derivative, e.g should only issue indexes till the current count of derivatives

As how it is right now the function "adjustWeight" allows adjusting weights for non-existing derivatives. In my opinion the function should check the derivative count and revert if the inputted _derivativeIndex is over the general count of derivatives. Even tho this problem doesn't lead to anything bad, that l am aware of, it should be fixed to prevent some unwanted issues from occurring.

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

Consider applying a require statement which checks the number of derivatives and revert incase the _derivativeIndex is over them.

function adjustWeight(
        uint256 _derivativeIndex,
        uint256 _weight
    ) external onlyOwner {
        require(derivativeCount >= _derivativeIndex, "Non-existing index");
        weights[_derivativeIndex] = _weight;
        uint256 localTotalWeight = 0;
        for (uint256 i = 0; i < derivativeCount; i++)
            localTotalWeight += weights[i];
        totalWeight = localTotalWeight;
        emit WeightChange(_derivativeIndex, _weight);
    }

[L-03] An already existing address of derivative can be added with the function "addDerivative", which might lead to wrong accounting on staking

The function "addDerivative" is used by the owner in order to add new derivatives to the system. The problem is the function doesn't check if an already existing address of derivative is added. Therefore two identical derivatives will exist and this issue might lead to the wrong accounting of the balances prior to staking and unstaking. Consider performing some sort of check, which ensures that the new derivative added to the system isn't already an existing one.

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

[L-04] The setter function for changing the current maxSlippage for a derivative should have a upper limit, so stakers won't experience a significant loss based on a big amount of slippage

The setter function for changing the current maxSlippage for a derivative should have an upper limit, so stakers won't experience a significant loss based on a big amount of slippage. Currently as higher this maxSlippage is set by the owner, the higher chance the staker will experience a significant lost of funds. It would be good to have an upper limit like 5%-10%, so the maxSlippage can't be set over that.

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

[L-05] There should be a way to remove derivatives and not just setting their weight as 0, as too many derivatives may lead to a greater gas consumption in the loops prior to staking and unstaking

Currently there is a function for adding new derivatives to the system, but an opposite function for removing unneeded derivatives anymore is not applied. Therefore in order to remove unwanted derivative, the function "adjustWeight" is called which basically adjust its weight to 0 so no more funds will be staked there, but it doesn't remove the derivative fully. So therefore both staking and unstaking function will loop over this unneeded derivatives and will raise the gas consumption even more. In my opinion there should some sort of way to full remove an unwanted derivative instead of just setting its weight to zero.

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

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

[L-06] The function "rebalanceToWeights" should not be used often or else the contracts will lose a significant amount of funds duo to slippage, by withdrawing and depositing again the funds to the derivatives.

The function "rebalanceToWeights" is used by the owner in order to rebalance each derivative to resemble the weight set for it. Basically what the function does is withdrawing the amount of funds that the derivatives hold and re-deposit it back to them based on the new set weight. The problem with this function is that calling it results to slippage and shorten the overall funds in the contracts. Shorten derrative's funds means that the staker's minted tokens will get less valuable. Therefore calling this function often, will result to the stakers taking a significant lose.

function rebalanceToWeights() external onlyOwner {
        uint256 ethAmountBefore = address(this).balance;
        for (uint i = 0; i < derivativeCount; i++) {
            if (derivatives[i].balance() > 0)
                derivatives[i].withdraw(derivatives[i].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();
    }

[L-07] It should not be possible to set all of the derivatives weights as zero, as this issue leads to users losing their ether prior to calling the function stake

The function "adjustWeight" is used by the owner in order to apply new weight to a certain _derivativeIndex. The main problem here is that, if all of the derivatives are successfully set as zero weight. The function "stake" won't deposit the users ether to the derivatives and therefore won't mint any tokens to them. But will consume the staker's msg.value and therefore this funds will be stuck in the contract.

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

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

Consider adding a check in the adjusting weight function, that totalWeights needs to be over zero otherwise the function should revert.

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;
        require(totalWeight > 0,"");
        emit WeightChange(_derivativeIndex, _weight);
    }

[N-01] The initialize function should emit the already existing events for changing the minAmount and maxAmount

The function initialize is used in order to initialize values for the contract. Like seen below, the function successfully sets the minAmount and maxAmount to certain values. But forgets to emit the already existing events allocated for this changes e.g "event ChangeMinAmount(uint256 indexed minAmount);" and "event ChangeMaxAmount(uint256 indexed maxAmount);".

function initialize(
        string memory _tokenName,
        string memory _tokenSymbol
    ) external initializer {
        ERC20Upgradeable.__ERC20_init(_tokenName, _tokenSymbol);
        _transferOwnership(msg.sender);
        minAmount = 5 * 10 ** 17; // initializing with .5 ETH as minimum
        maxAmount = 200 * 10 ** 18; // initializing with 200 ETH as maximum
    }

event ChangeMinAmount(uint256 indexed minAmount);
event ChangeMaxAmount(uint256 indexed maxAmount);

[R-01] Unstaking function could check in the beginning if the amount of "_safEthAmount" inputted is greater than zero, otherwise it should revert

The unstaking function can perform a require statement in the beginning of the function to ensure the inputted amount of safEth is greater than zero, so the function won't loop over the derivatives just to revert at the end.

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

Consider applying a require statement to ensure the safEth amount inputted by the user is greater than zero.

function unstake(uint256 _safEthAmount) external {
        require(pauseUnstaking == false, "unstaking is paused");
        require(_safEthAmount > 0, "Zero unstable not allowed");
        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);
    }