Asymmetry contest - hihen's results

Lines of code

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

Vulnerability details

Impact

If the withdraw function of any derivative fails, the unstake() and rebalanceToWeights() functions will always revert, resulting in all staked assets being frozen.

Proof of Concept

In the unstake() and rebalanceToWeights() functions, all derivatives' withdraw() functions are called (as long as the amount involved is not 0). Therefore, if the withdraw() function of any derivative reverts, the entire transaction will revert.

function unstake(uint256 _safEthAmount) external {
    ...
    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);
    }
    ...
}

function rebalanceToWeights() external onlyOwner {
    ...
    for (uint i = 0; i < derivativeCount; i++) {
        if (derivatives[i].balance() > 0)
            derivatives[i].withdraw(derivatives[i].balance());
    }
    ...
}

Although theoretically the likelihood of a derivative.withdraw() failure is not common, this possibility exists, and there are quite a few risk points:

• Any underlying derivative contract errors: such as vulnerabilities, upgrades, or hacker attacks, etc.
• Withdrawal-related DEX (Uniswap, Curve) errors: such as vulnerabilities, upgrades, or hacker attacks, or liquidity pool liquidity anomalies, etc.

In other words, any of these external factors can cause all staked ETH in SafEth to be frozen, which is too risky and the consequences are too severe.

Moreover, the problem cannot be solved by setting the weight of the faulty derivatives to zero

Tools Used

VS Code

Recommended Mitigation Steps

We can add a parameter to the unstake() and rebalanceToWeights(): address[] skipDerivatives, allowing users to provide some derivatives to be ignored in those special cases, so that assets that are not affected can be safely unstaked/withdrawn.

Lines of code

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

Vulnerability details

Impact

If no derivative contracts have been added to SafEth yet and a user calls the SafEth.stake(), all ETH will be locked in the contract.

Proof of Concept

After SafEth is deployed, pauseStaking is set to false by default, and users can call the SafEth.stake() successfully at this time.

However, since the derivatives need to be added manually by the owner one by one, derivatives are empty at this point (i.e. derivativeCount = 0), it will cause the following issues in stake():

• The ETH transferred (msg.value) with the tx is not deposited into any derivative contract.
• totalStakeValueEth and mintAmount are always 0, so 0 SafEth tokens will be minted to the msg.sender.

In other words, all the ETH staked by the user is locked in the SafEth contract, and the user receives 0 SafEth tokens.

Tools Used

VS Code

Recommended Mitigation Steps

We should add require(derivativeCount > 0, "derivatives is empty"); in stake() to prevent this issue.

Additionally, it may be considered to set pauseStaking = true in SafEth.initialize(), and call SafEth.setPauseStaking() after the configuration is complete(such as adding derivatives).