A problem occurs in the staking function, which does calculations based on two different eth per derivative values in the beginning and on the end of the function. The problem occurs for the derivatives SfrxEth and WstEth.

[code423n4]

Lines of code

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

Vulnerability details

Impact

Based on the description in the poc section, we can see that the staking function calculates the derivatives "underlyingValue" and "derivativeReceivedEthValue" based on two different values of "ethPerDerivative". This both local amounts are later used for the calculation of the amount of tokens minted to the user.

Proof of Concept

The problem occurs mainly for the derivatives SfrxEth and WstEth, if we look on the staking function below, we can see that the underlying value is calculated based on the function "ethPerDerivative", which returns the eth per derivative value.

The preDepositPrice is calculated based on this underlying value, which is later used for the mintAmount calculation. The problem which occurs here is that prior to calculating the "derivativeReceivedEthValue", a different "ethPerDerivative" will be used, as the function first deposits the msg.value to the derivatives, which changes the balances in the pool and therefore a different value might be returned.

Let's look at the SfrxEth derivative first, we can see that the function converts shares of 1e18 to assets. This function may return a different outcome at the end when calculating "derivativeReceivedEthValue" , as depositing funds happens first which will change the balances in the Sfrx contract, therefore the amount returned from the function convertToAssets might also be different.

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

Same issue occurs for the WstEth derivative as well.

function ethPerDerivative(uint256 _amount) public view returns (uint256) {
        return IWStETH(WST_ETH).getStETHByWstETH(10 ** 18);
    }

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

Tools Used

Manual review.

Recommended Mitigation Steps

To simple fix this issue and prevent the use of two different eth per derivative values. We can cache the return value of the function "ethPerDerivative" before the deposit was made. So both "underlyingValue" and "derivativeReceivedEthValue" can be calculated based on the same eth per derivative.

uint256 cache = derivative.ethPerDerivative(depositAmount);
uint256 depositAmount = derivative.deposit{value: ethAmount}();
uint derivativeReceivedEthValue = (cache * depositAmount) / 10 ** 18;

[c4-pre-sort]

0xSorryNotSorry marked the issue as high quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #698

[c4-sponsor]

toshiSat marked the issue as sponsor disputed

[toshiSat]

This is as intented

[c4-judge]

Picodes marked the issue as not a duplicate

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[Picodes]

Briefly checking the code of sfrxETH and wstETH I don't see how the price could change between the 2 calls, and this isn't demonstrated in this report.

This function may return a different outcome

This would need to be demonstrated or at least we should have some hypothetical scenario for the severity to be Med