Asymmetry contest - Co0nan's results

Lines of code

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

Vulnerability details

Impact

The WstETH.sol incorrectly calculates the price of ETH in terms of WstETH.

The function "getStETHByWstETH" returns the price of WstETH in terms of stETH. according to the docs. However, the underlying token which we desire is ETH.

ETH and stETH do not always have the same value, if the price of ETH changes, the value of stETH will also change to reflect this. But, there may be small discrepancies between the two due to transaction fees and other factors.

The incorrect price may over or understate the harvestable amount which is a core calculation in the protocol.

Proof of Concept

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);
        //Returns amount of stETH for a given amount of wstETH
    }

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

Tools Used

Manual

Recommended Mitigation Steps

Add extra steps to getStETHByWstETH() to approximate the rate for converting stETH to ETH. This can be done using the same curve pool that is used to convert stETH to ETH in unwrap().

Lines of code

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

Vulnerability details

Impact

There will be little ETH loss if the rebalanceToWeights function called before user withdraw his funds, there are some slippage when the derivatives are being rebalanced. This is because the function withdraws all the ETH from the derivative contracts and then deposits them back based on the target weightings.

/WstETH.sol

L60: uint256 minOut = (stEthBal * (10 ** 18 - maxSlippage)) / 10 ** 18;

Owner can cause loss of funds either intentionally or unintentionally by calling this function multiply times.

Setting this as Medium since the impact is High but the likelihood is Low.

Proof of Concept

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

1 - The following test case will show you the excepted ETH users will gain in the normal scenario. Add below code on SafETH.test.ts

  describe("Lose of fund test", function () {
    it.only("Testing ", async function () {
      const accounts = await ethers.getSigners();
      const admin = safEthProxy.connect(accounts[0]);
      const alice = safEthProxy.connect(accounts[1]);
      const bob = safEthProxy.connect(accounts[2]);
      const mark = safEthProxy.connect(accounts[3]);


      const depositAmount = ethers.utils.parseEther("10");
      const manipulateWstETHPrice = ethers.utils.parseEther("1");


      await alice.stake({ value: depositAmount });
      await bob.stake({value : depositAmount});
      await mark.stake({value : depositAmount});
      
      await alice.unstake(await safEthProxy.balanceOf(accounts[1].address));
      await bob.unstake(await safEthProxy.balanceOf(accounts[2].address));
      await mark.unstake(await safEthProxy.balanceOf(accounts[3].address));
      console.log("Alice FB : " + await accounts[1].getBalance())
      console.log("Bob FB : " + await accounts[2].getBalance())
      console.log("Mark FB : " + await accounts[3].getBalance())
      
    });
  });

2 - Output:

 Alice FB : 9999964342323255477277
 Bob FB : 9999965317569651706561
 Mark FB : 9999965791016809200480

3 - Change the test code to simulate owner called rebalanceToWeights for example 3 times before users call unstake

describe("Lose of fund test", function () {
    it.only("Testing ", async function () {
      const accounts = await ethers.getSigners();
      const admin = safEthProxy.connect(accounts[0]);
      const alice = safEthProxy.connect(accounts[1]);
      const bob = safEthProxy.connect(accounts[2]);
      const mark = safEthProxy.connect(accounts[3]);


      const depositAmount = ethers.utils.parseEther("10");
      const manipulateWstETHPrice = ethers.utils.parseEther("1");


      await alice.stake({ value: depositAmount });
      await bob.stake({value : depositAmount});
      await mark.stake({value : depositAmount});

      await admin.rebalanceToWeights();
      await admin.rebalanceToWeights();
      await admin.rebalanceToWeights();
      
      await alice.unstake(await safEthProxy.balanceOf(accounts[1].address));
      await bob.unstake(await safEthProxy.balanceOf(accounts[2].address));
      await mark.unstake(await safEthProxy.balanceOf(accounts[3].address));
      console.log(" Alice FB : " + await accounts[1].getBalance())
      console.log(" Bob FB : " + await accounts[2].getBalance())
      console.log(" Mark FB : " + await accounts[3].getBalance())
      
    });
  });

4 - Output :

9999874690441675992902
9999875523035510546687
9999875927489681717072

5 - As shown, users ETH balance decreased. ex: Alice lost 0.089651881579484375 ETH == 160.08 USD.

Tools Used

Manual

Recommended Mitigation Steps

Maybe you can delete this function as it's not making anything useful with the current implementation.

Lines of code

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

Vulnerability details

Impact

The current implementation of SafETH.sol is that owner must update/add derivatives manually, with that case, when the contract get deployed. The default value for weights mapping array and totalWeights will be Zero.

Given that, if user call stack function to deposit ETH before owner call addDerivatives(), or when the the totalWeights is zero, the if condition here will always skip for each derivatives as this mapping didn't updated yet so each weight will be 0. Hence, the deposit function for each derivatives will not be called and the function will continue executing on line 98-100.

While user excepting to receive token in exchange for his ETH, he will gain 0 token and his ETH will be already sent to the contract.

Proof of Concept

On line 87, if (weight == 0) continue; will pass to the next loop as uint256 weight = weights[i]; will be 0 since it didn't get updated yet.

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; // 1000000000000000000 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]; //3
            IDerivative derivative = derivatives[i];
            if (weight == 0) continue;
            uint256 ethAmount = (msg.value * weight) / totalWeight; //  2

            // 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; // 0 always
        _mint(msg.sender, mintAmount);
        emit Staked(msg.sender, msg.value, mintAmount);
    }

Below function is the only way to update this mapping and the totalWeight

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

    /**
        @notice - Adds new derivative to the index fund
        @param _contractAddress - Address of the derivative contract launched by AF
        @param _weight - new weight for this derivative. 
    */
    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);
    }

Tools Used

Manual

Recommended Mitigation Steps

Either update the weights and add derivatives on the initialize function, or add a require check:

require(totalWeights != 0, "Weights is Zero");