Asymmetry contest - MadWookie's results

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L71-L75 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L211-L215 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L120-L150

Vulnerability details

When a user initiates a stake the function ethPerDerivative is first used to calculate the underlying value of the protocol and then it is used again to calculate the value of the users deposit.

First call

 for (uint i = 0; i < derivativeCount; i++)
            underlyingValue +=
                (derivatives[i].ethPerDerivative(derivatives[i].balance()) *
                    derivatives[i].balance()) /
                10 ** 18;

Second call

uint derivativeReceivedEthValue = (derivative.ethPerDerivative(
                depositAmount
            ) * depositAmount) / 10 ** 18;
            totalStakeValueEth += derivativeReceivedEthValue;

In the case of Reth, the _amount variable is used as the function poolCanDeposit determines if _amount can be deposited directly to the Reth pool or rETH tokens must be bought on UniswapV3.

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

The issue is that on the first call to ethPerDerivative when initiating a stake, the contract balance of Reth is passed into _amount and not the amount the user is staking. Thus a deposit of the whole contract balance can fail while the deposit of the user's amount can return true in poolCanDeposit. This leads to additional unnecessary slippage for the user as the quoted uniswap price trends at a premium and is even stated in the Rocketpool docs

Proof of Concept

Further explained in the example below.

1. Reth.sol has a balance of 100 rETH

2. RocketDepositPool has 1 ETH open for deposit(meaning rocketDepositPool.getBalance() + 1e18 == rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize())

3. User Alice deposits 3 ETH into SafEth.sol with even split between all derivatives.

4. poolCanDeposit() will fail on first call as rocketDepositPool.getBalance() + _amount <= rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize() evaluates false thus returning Uniswapv3 price.

5. Second call to poolCanDeposit() now passes in Alice's 1 Eth and returns true allowing Alice to directly deposit into the pool.

6. Alice then loses out of the difference between the Uniswapv3 pool price even though she was able to directly deposit into the pool.

Note that this also can also occur when the the Reth.sol rETH balance is less than rocketDAOProtocolSettingsDeposit.getMinimumDeposit() and a user makes a deposit greater than rocketDAOProtocolSettingsDeposit.getMinimumDeposit()

Recommend mitigation

Pass in the user deposit amount to ethPerDerivative and not the total balance of the Reth.sol contract. With the current implementation it makes no sense to use this protocol when depositing directly to the rocket pool is available and the protocol has sufficient balance.

Lines of code

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

Vulnerability details

The function poolPrice()returns the current Uniswapv3 price of rETH and is used to calculate the value of current staked rETH and newly deposited ETH in terms of rETH.

function poolPrice() private view returns (uint256) {
        address rocketTokenRETHAddress = RocketStorageInterface(
            ROCKET_STORAGE_ADDRESS
        ).getAddress(
                keccak256(
                    abi.encodePacked("contract.address", "rocketTokenRETH")
                )
            );
        IUniswapV3Factory factory = IUniswapV3Factory(UNI_V3_FACTORY);
        IUniswapV3Pool pool = IUniswapV3Pool(
            factory.getPool(rocketTokenRETHAddress, W_ETH_ADDRESS, 500)
        );
        (uint160 sqrtPriceX96, , , , , , ) = pool.slot0();
        return (sqrtPriceX96 * (uint(sqrtPriceX96)) * (1e18)) >> (96 * 2);
    }

The problem is that this function returns the current price and not the TWAP price leaving it susceptible to temporary manipulations in the Uniswapv3 pool via flash loans. Thus a malicious user could potentially undervalue the current staked rETH and overvalue their own deposit.

Recommended Mitigation Steps

A time-weighted-average price(TWAP) function can be implemented to make the protocol more resilient to flash loan style attacks.