Asymmetry contest - dec3ntraliz3d's results

Lines of code

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

Vulnerability details

Impact

A wrong derivative address can brick the staking and unstaking functionality in the contract, preventing further user interaction. Users cannot stake or unstake their tokens, causing a loss of functionality and potential financial impact.

The below function doesn't check for zero address or a contract address that conforms to the IDerivative interface.

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

Proof of Concept

The issue occurs when a wrong derivative address is added by owner. In the provided test cases, two scenarios were tested:

1. Using the zero address (address(0)) as the derivative address.

it("Should brick contract if address(0) is set as  derivative contract address ", async function () {
      const depositAmount = ethers.utils.parseEther("10");
      await safEthProxy.stake({ value: depositAmount });

      // set zero address for derivative contract address
      await safEthProxy.addDerivative(
        ethers.constants.AddressZero,
        ethers.utils.parseEther("0")
      );
      await expect(safEthProxy.unstake(ethers.utils.parseEther("1"))).to.be
        .reverted;

      // no more staking is possible.
      await expect(safEthProxy.stake({ value: 1 })).to.be.reverted;
    });

2. Using a random, non-conforming address as the derivative address.

    it("Should brick contract if a non conforming IDerivative address  is set as  derivative contract address ", async function () {
      const depositAmount = ethers.utils.parseEther("10");
      await safEthProxy.stake({ value: depositAmount });

      // set zero address for derivative contract address
      await safEthProxy.addDerivative(
        ethers.Wallet.createRandom().address,
        ethers.utils.parseEther("1")
      );
      await expect(safEthProxy.unstake(ethers.utils.parseEther("1"))).to.be
        .reverted;

      // no more staking is possible.
      await expect(safEthProxy.stake({ value: 1 })).to.be.reverted;
    });

In both scenarios, the contract will lose its main functionality, staking and unstaking.

Tools Used

Manual

Recommended Mitigation Steps

1. Implement an address(0) check .
2. Add a validation function that verifies if an address implements the IDerivative interface.
3. Implement a mechanism for the contract owner to update and/or remove Derivative address.

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L111-L117 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L74-L75

Vulnerability details

Impact

The ethPerDerivative() function uses Curve ETH-frxETH pool's price oracle. However, the calculation is incorrect. The value returned by the price oracle is the value of 1 frxETH in ETH, not the other way around.

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

Proof of Concept

This issue will have side effects in other parts of the contract. For example, in the withdraw function, it uses ethPerDerivative() function to calculate maximum slippage when swapping from frxETH to ETH.

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

Let's say frxETH is trading at a 1% discount. Also, assume that 1 stakedFrxETH is 1.03 frxETH. As per current implementation, 1 stakedFrxETH will be:

(1.03 / 0.99) or ~1.04 ETH

Let's say we want to withdraw 1 stakedfrxETH, so:

minOut = 1 * 1.04 * 99% = ~1.03 ETH // 1% slippage

However, in reality, the maximum ETH that you can get for 1 stakedfrxETH is 1.03 * 0.99 or ~1.02. So the swap will fail even with just 1% discount.

Tools Used

Manual review.

Recommended Mitigation Steps

To resolve this issue, update the ethPerDerivative() function to properly calculate the ETH value of a derivative. The corrected function should look like this:

     function ethPerDerivative(uint256) 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());
+
+        uint256 frxPriceInETH = IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS)
+            .price_oracle();
+        return (frxAmount * frxPriceInETH) / 10 ** 18;

}