Asymmetry contest - ck's results

Lines of code

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

Vulnerability details

Impact

Staking is vulnerable to the first depositor exploit where a malicious user can stake a very small amount such as 1 wei.

They could then send inflate the value of their share by send a large amount of underlying to the contract.

Now other stakers would unfairly receive very little shares when they stake. The attacker would also gain an inflated amount when they unstake because they already have inflated shares.

        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;

Proof of Concept

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

Tools Used

Recommended Mitigation Steps

Premint a certain number of shares before the first staker, alternatively require a minimum deposit for the first staker.

Lines of code

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

Vulnerability details

Impact

When withdrawal of a derivative fails, a revert happens. If the reverts are persisitent e.g due to an exploited derivative contract, this could prevent unstaking as the whole unstaking function would continue reverting.

Since the contract's (derivatives[i].balance() still returns some positive value, this issue would become persisitent.

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

An attempt at rebalancing would encounter the same issue as withdrawing is still done in a loop.

Proof of Concept

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

Tools Used

Recommended Mitigation Steps

Provide an alternative unstaking function that can be used in such cases where the derivative with an issue can be skipped or left out.

Pausing functionality can be done with just one function

There are currently separate functions for pausing and unpausing

   function setPauseStaking(bool _pause) external onlyOwner {
        pauseStaking = _pause;
        emit StakingPaused(pauseStaking);
    }

    /**
        @notice - Owner only function that enables/disables the unstake function
        @param _pause - true disables unstaking / false enables unstaking
    */
    function setPauseUnstaking(bool _pause) external onlyOwner {
        pauseUnstaking = _pause;
        emit UnstakingPaused(pauseUnstaking);
    }

This is unnecessary as one function is enough, the stakingPaused variable alone can be used to check were staking is paused or unpaused.

Validate minimum and maximum values when setting slippage

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

Currently there are no restrictions on the amount of slippage that can be set. To prevent unintended effects, the input should be validated for acceptable minimum and maximum slippage.

Validate that totalWeight should never be 0

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

When the weights are being set or adjusted, it would be good practice to ensure the totalWeight isn't 0 in a scenario where either by malice or mistake, all weights are set to 0.

Validate that the _contractAddress of a derivative cannot be set to 0

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

When adding a derivative, a check should be done to prevent _contractAddress being set to 0 by mistake.

Weight calculation can result in some derivatives failing to get a proportional stake

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

If msg.value is a low value and a derivative has low weight, there are instances where the ethAmount will be zero.

A suggestion would be to enforce a minimum weight to total weight ratio for derivatives.