Asymmetry contest - Dug's results

Lines of code

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

Vulnerability details

Impact

An initial depositor can attack the protocol in a way that inflates the perDepositPrice for future stakers.

This is a variation of the classic first depositor attack. In this case, the attacker can manipulate the underlyingValue to inflate the perDepositPrice. This allows the attacker to steal funds from future stakers.

Proof of Concept

First, the attacker makes a minAmount deposit into the protocol, receiving safEth in return. Then, the attacker immediately withdraws all but 1 wei of safEth, this leaves the totalSupply of safEth as 1 wei.

A non-zero totalSupply will cause future calls to stake() to be calculated using the underlyingValue instead of having an initial price of 1 ether.

if (totalSupply == 0)
    preDepositPrice = 10 ** 18; // initializes with a price of 1
else preDepositPrice = (10 ** 18 * underlyingValue) / totalSupply;

The attacker then sends a tiny amount of Reth, SfrxEth, and WstEth to the address of the corresponding derivatives. Directly altering the balance of the derivatives will cause the underlyingValue to increase, but the totalSupply of safEth will remain at 1 wei. This will cause the perDepositPrice to be greatly inflated.

For example, by making the derivatives balance for Reth, SfrxEth, and WstEth each equal to 2 wei, the underlyingValue will be ~6 wei.

uint256 underlyingValue = 0;

// Getting underlying value in terms of ETH for each derivative
for (uint i = 0; i < derivativeCount; i++)
    // @audit - Underlying value can be me manipulated by sending tokens directly to the derivate contracts
    underlyingValue +=
        (derivatives[i].ethPerDerivative(derivatives[i].balance()) *
            derivatives[i].balance()) /
        10 ** 18;

So, we now have ~6 wei of underlyingValue, and 1 wei totalSupply of safEth. This causes the next call to stake() to set the preDepositPrice as 6 ether.

preDepositPrice = (10 ** 18 * underlyingValue) / totalSupply; 

This results in safEth being priced 6x higher than it should be for the next staker. Their transaction can then be backrun by the attacker, staking and withdrwaing, to effectively steal funds from the victim.

Tools Used

Manual review

Recommended Mitigation Steps

During contract creation, make an initial deposit that is large enough to prevent large swings in the perDepositPrice. This initial deposit should remain locked in the vault.

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L170-L185 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L228-L242

Vulnerability details

Impact

When a deposit is made the Reth derivative contract, if the amount is larger than the pool can hold, a swap is made using a Uniswap pool. minOut is determined using the spot price for the pool.

AMM spot prices can be manipulated by attackers and should not be used directly for price discovery. This results in a frontrunning vulnerability where higher slippage can occur and fewer Reth tokens are held in the safEth contract than expected.

Proof of Concept

When a deposit is made to the the Reth derivative contract, if there is not room in the pool for the amount deposited, a Uniswap swap is made to acquire Reth.

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

There are safeguards in place that calculate a minOut value that is used to prevent slippage from exceeding maxSlippage, however, This value is determined using poolPrice() which is calculated using the spot price of the pool.

  (uint160 sqrtPriceX96, , , , , , ) = pool.slot0();
  return (sqrtPriceX96 * (uint(sqrtPriceX96)) * (1e18)) >> (96 * 2);

An attacker can frontrun the deposit, taking out a large flash loan for ETH and swapping it for Reth via the Uniswap pool. This trade will increase the price of Reth because of the increased demand.

An increased price will result in a lower minOut value, which will result in more slippage than expected when the swap is made and fewer tokens being held in the safEth contract than expected.

Tools Used

Manual review

Recommended Mitigation Steps

Use a more robust method for determining the poolPrice. A secure price calculation can be performed by using time-weighted average prices (TWAPs) across longer time intervals.

uint32 _twapDuration = twapDuration; // Duration configured and stored in contract
uint32[] memory secondsAgo = new uint32[](2);
secondsAgo[0] = _twapDuration;
secondsAgo[1] = 0;

(int56[] memory tickCumulatives, ) = pool.observe(secondsAgo);
return uint256((tickCumulatives[1] - tickCumulatives[0]) / _twapDuration);

Unstaked event data can be manipulated

When the unstake() function is called, withdraw() is called on each derivative. At the end of each, the derivative sends its full ether balance to the SafEth contract.

(bool sent, ) = address(msg.sender).call{value: address(this).balance}(
    ""
);

After receiving ether from each derivative, unstake() sends the total received ether to the user and then emits the Unstaked event.

emit Unstaked(msg.sender, ethAmountToWithdraw, _safEthAmount);

Because of how this works, a user could send ether to a derivative contract ahead of calling unstake(). This would cause the derivative to send more ether to the SafEth contract than it normally would as it would include the amount just sent to the derivative.

The funds would ultimately be returned to the user and the Unstaked event would store this manipulated ethAmountToWithdraw amount instead of the amount that was actually related to the unstaking of safETH.

As a result, event data and any dependent reporting/analytics would be compromised.

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

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

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#L56-L67

Funds are lost if stake() is called before the first derivative added

The stake() function will successfully execute even if no derivatives are added. This means that a user could stake safETH and lose all of their funds sent with the call.

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

A check should be added that causes the call to revert if mintAmount is 0.

Checks for minAmount and maxAmount are inconsequential

In code documentation describes the minAmount and maxAmount parameters as the minimum and maximum amount a user is allowed to stake. However, a user can easily bypass these checks.

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

Th max amount check is bypassed by simply calling stake() more than once.

The min amount check is bypassed by staking the minAmount and then calling unstake() to withdraw a partial balance.

Use Ownable2StepUpgradeable instead of OwnableUpgradable to prevent loss of admin functionality

The SafeEth contract currently uses OwnableUpgradeable to manage admin functionality. This contract is vulnerable to a loss of admin functionality if transferOwnership() is called with the incorrect address.

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

It is recommended to use the Ownable2StepUpgradeable contract instead. This contract requires the new owner to accept the ownership transfer before it can be completed. This prevents the loss of functionality if the incorrect address is used.

Derivative contracts do not emit events for state changes

Generally, it is a good practice to emit events for all state changes. This allows interested parties to easily track changes to the state of the contract.

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol

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

IDerivative should include events that are emitted by all derivatives.