Asymmetry contest - silviaxyz's results

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L63 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L73-L81 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L156-L204 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L170 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L120-L150

Vulnerability details

Impact

The users will not be able to stake their funds and there will be loss of reputation

Proof of Concept

The SafEth contract's stake() function is the main entry point to add liquid Eth to the derivatives. Accordingly the stake() function takes the users' ETH and convert it into various derivatives based on their weights and mint an amount of safETH that represents a percentage of the total assets in the system.

The execution to deposit to the available derivative is done through iterating the derivatives mapping in SafEthStorage contract.

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

And for all the derivatives the stake function calls the derivative contract's deposit() function. Below is for WstEth contract's deposit() function to adapt to Lido staking;

function deposit() external payable onlyOwner returns (uint256) {
    uint256 wstEthBalancePre = IWStETH(WST_ETH).balanceOf(address(this));
    // solhint-disable-next-line
    (bool sent, ) = WST_ETH.call{value: msg.value}("");
    require(sent, "Failed to send Ether");
    uint256 wstEthBalancePost = IWStETH(WST_ETH).balanceOf(address(this));
    uint256 wstEthAmount = wstEthBalancePost - wstEthBalancePre;
    return (wstEthAmount);
}

The Lido protocol implements a daily staking limit both for stETH and WstETH as per their docs Accordingly the daily rate is 150000 ETH and the deposit() function will revert if the limit is hit. From the docs:

Staking rate limits In order to handle the staking surge in case of some unforeseen market conditions, the Lido protocol implemented staking rate limits aimed at reducing the surge's impact on the staking queue & Lido’s socialized rewards distribution model. There is a sliding window limit that is parametrized with _maxStakingLimit and _stakeLimitIncreasePerBlock. This means it is only possible to submit this much ether to the Lido staking contracts within a 24 hours timeframe. Currently, the daily staking limit is set at 150,000 ether. You can picture this as a health globe from Diablo 2 with a maximum of _maxStakingLimit and regenerating with a constant speed per block. When you deposit ether to the protocol, the level of health is reduced by its amount and the current limit becomes smaller and smaller. When it hits the ground, transaction gets reverted. To avoid that, you should check if getCurrentStakeLimit() >= amountToStake, and if it's not you can go with an alternative route. The staking rate limits are denominated in ether, thus, it makes no difference if the stake is being deposited for stETH or using the wstETH shortcut, the limits apply in both cases.

However this check was not done either in SafEth::stake() or WstEth::deposit() functions. So if the function reverts, the stake function will all revert and it will not be possible to deposit to the other derivatives as well.

Another issue lies in the Reth contract having the same root cause below - Missing Validation & external tx dependency.

For all the derivatives the stake function calls the derivative contract's deposit() function. Below is rETH contract's deposit() function;

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

At Line#170 it checks the pools availability to deposit with poolCanDeposit(msg.value);

PoolCanDeposit function below;

function poolCanDeposit(uint256 _amount) private view returns (bool) {
    address rocketDepositPoolAddress = RocketStorageInterface(
        ROCKET_STORAGE_ADDRESS
    ).getAddress(
            keccak256(
                abi.encodePacked("contract.address", "rocketDepositPool")
            )
        );
    RocketDepositPoolInterface rocketDepositPool = RocketDepositPoolInterface(
            rocketDepositPoolAddress
        );
    address rocketProtocolSettingsAddress = RocketStorageInterface(
        ROCKET_STORAGE_ADDRESS
    ).getAddress(
            keccak256(
                abi.encodePacked(
                    "contract.address",
                    "rocketDAOProtocolSettingsDeposit"
                )
            )
        );
    RocketDAOProtocolSettingsDepositInterface rocketDAOProtocolSettingsDeposit = RocketDAOProtocolSettingsDepositInterface(
            rocketProtocolSettingsAddress
        );
    return
        rocketDepositPool.getBalance() + _amount <=
        rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize() &&
        _amount >= rocketDAOProtocolSettingsDeposit.getMinimumDeposit();
}

However, as per Rocket Pool's RocketDepositPool contract, there is an other check to confirm the availability of the intended deposit;

require(rocketDAOProtocolSettingsDeposit.getDepositEnabled(), "Deposits into Rocket Pool are currently disabled");

The Reth::deposit() function doesn't check this requirement whether the deposits are disabled. As a result, the SafEth::stake() function will all revert and it will not be possible to deposit to the other derivatives as well.

Tools Used

Manual Review

Recommended Mitigation Steps

1. For WstETH contract; checking the daily limit via getCurrentStakeLimit() >= amountToStake
2. For Reth contract; Checking the Rocket Pool's deposit availability
3. Wrap the stake() function's iteration inside try/catch block to make the transaction success until it reverts.

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L60-L88 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L60 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L174

Vulnerability details

Impact

The users will not be able to unstake their funds and suffer financial loss, instead the SafEth price will fall due to panic

Proof of Concept

The derivative contracts implement a limit value called: maxSlippage. It's sen during initializing the contracts as;

maxSlippage = (1 * 10 ** 16); // 1%

The main usage of the value is to implement a ration to control the receiving ETH during withdrawal state from the derivative contracts and in deposit() function of Reth contract. Accordingly, the withdraw() functions in WstETH and SfrxEth contracts and deposit() function in Reth contract check whether the min received amount is in the percentage limits. So if the msg.value is 1 ETH and the underlying price of derivative token is 1:1 for that moment, the minimum received underlying token should be 0,99 once 1% maxSlippage applied and should not fall below this, else the transaction reverts. An example code snippet from SfrxEth contract is below;

function withdraw(uint256 _amount) external onlyOwner {
    IsFrxEth(SFRX_ETH_ADDRESS).redeem(
        _amount,
        address(this),
        address(this)
    );
    uint256 frxEthBalance = IERC20(FRX_ETH_ADDRESS).balanceOf(
        address(this)
    );
    IsFrxEth(FRX_ETH_ADDRESS).approve(
        FRX_ETH_CRV_POOL_ADDRESS,
        frxEthBalance
    );
    uint256 minOut = (((ethPerDerivative(_amount) * _amount) / 10 ** 18) *
        (10 ** 18 - maxSlippage)) / 10 ** 18;//<----------------------------------- minOut is calculated by applying 1% @audit-info
    IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).exchange(
        1,
        0,
        frxEthBalance,
        minOut
    );
    // solhint-disable-next-line
    (bool sent, ) = address(msg.sender).call{value: address(this).balance}(
        ""
    );
    require(sent, "Failed to send Ether");
}

Same can be seen in WstEth and Reth contracts.

However, 1% of buffer protection will not work as intended to protect the users during turbulent market conditions. Especially the withdraw functions will revert and the users will not be able to unstake their assets. On the contrary, they will sell their SafEth tokens in possession to keep their funds' financial value. This will cause a deep price dump of SafEth tokens and loss of reputation which the protocol will not benefit at all.

Tools Used

Manual Review

Recommended Mitigation Steps

Instead a protocol maxSlippage implementation, allow users to chose their slippage at unstake or stake functions and the withdraw function will be as below;

function withdraw(uint256 _amount, uint256 _slippage) external onlyOwner {
    IsFrxEth(SFRX_ETH_ADDRESS).redeem(
        _amount,
        address(this),
        address(this)
    );
    uint256 frxEthBalance = IERC20(FRX_ETH_ADDRESS).balanceOf(
        address(this)
    );
    IsFrxEth(FRX_ETH_ADDRESS).approve(
        FRX_ETH_CRV_POOL_ADDRESS,
        frxEthBalance
    );
    uint256 minOut = (((ethPerDerivative(_amount) * _amount) / 10 ** 18) *
        (10 ** 18 - _slippage)) / 10 ** 18;
    IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).exchange(
        1,
        0,
        frxEthBalance,
        minOut
    );
    // solhint-disable-next-line
    (bool sent, ) = address(msg.sender).call{value: address(this).balance}(
        ""
    );
    require(sent, "Failed to send Ether");
}