Asymmetry contest - ernestognw's results

Lines of code

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

Vulnerability details

Impact

Currently, all deposit operations split the received ETH between derivatives. One of them is rETH, trading on Pool 0xa4e0faA58465A2D369aa21B3e42d43374c6F9613 with around ~1500 ETH in liquidity for each side.

The price calculation for splitting deposits into derivatives is made using Uniswaps' V3 sqrtPriceX96 as expressed in the following function

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

However, these price can be manipulated, letting a user inflating the supply in the contract.

Proof of Concept

// SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.13;

import "./fixtures/SafEthSetup.sol";

contract SafEthTest is SafeEthSetup { address RETH_TOKEN_ADDRESS = 0xae78736Cd615f374D3085123A210448E74Fc6393; address W_ETH_ADDRESS = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2; address UNI_V3_FACTORY = 0x1F98431c8aD98523631AE4a59f267346ea31F984;

function test_alterBalance() public {
    (bool success, ) = address(safEth).call{value: 1 ether}("");
    require(success);
    assertEq(safEth.totalSupply(), 0);
    assertEq(address(safEth).balance, 1 ether);
}

function uniswapV3SwapCallback(
    int256 amount0Delta,
    int256 amount1Delta,
    bytes memory data
) external {
    IERC20(RETH_TOKEN_ADDRESS).transfer(
        address(msg.sender),
        uint256(amount0Delta)
    );
}

function test_manipulateReth() public {
    // Random address with balance
    vm.startPrank(0xBA12222222228d8Ba445958a75a0704d566BF2C8);
    vm.label(0xBA12222222228d8Ba445958a75a0704d566BF2C8, "Attacker");
    IERC20(RETH_TOKEN_ADDRESS).transfer(address(this), 10_000 ether);
    vm.stopPrank();

    IUniswapV3Factory factory = IUniswapV3Factory(UNI_V3_FACTORY);
    IUniswapV3Pool pool = IUniswapV3Pool(
        factory.getPool(RETH_TOKEN_ADDRESS, W_ETH_ADDRESS, 500)
    );

    (uint160 sqrtPriceX96Before, , , , , , ) = pool.slot0();
    console.log(sqrtPriceX96Before);

    pool.swap(
        0xBA12222222228d8Ba445958a75a0704d566BF2C8,
        true,
        10_000 ether,
        4295128739 + 1,
        ""
    );
    (uint160 sqrtPriceX96, , , , , , ) = pool.slot0();

    console.log(sqrtPriceX96);
    assertTrue(sqrtPriceX96 < sqrtPriceX96Before / 10); // 10%
}

}

Tools Used

• Foundry

Recommended Mitigation Steps

Use a TWAP instead https://blog.uniswap.org/uniswap-v3-oracles

ETH sent to the contract will be unlocked

The SafEth.sol contract contains a receive() external payable function that allows depositing into the contract directly. However, there's no sweeping mechanism for ETH in the contract and the only way to get withdraw ETH from the contract is by calling unstake() which only sends the difference in ETH after exchanging the derivatives.

With the current configuration, any excedent ETH on the contract will be locked.

PoC

Consider the following test using Foundry:

// @notice Test
function test_alterBalance() public {
    _deploy();
    (bool success, ) = address(safEth).call{value: 1 ether}("");
    require(success);
    assertEq(safEth.totalSupply(), 0);
    assertEq(address(safEth).balance, 1 ether);
}

function _deploy() internal {
   // Implementations of these omitted for clarity
    _deploySafETH();
    _deployDerivatives();
    _addDerivatives();
}

Recommendation

Whitelist the allowed callers of the receive() function, so compatibility with derivatives is not broken. Also, consider calling stake() on every receive() execution for every non-whitelisted caller.

OpenZeppelin's EnumerableSet is recommended for the whitelisting mechanism.

Calling stake() can lock ETH up to derivativeCount wei

While computing ethAmount inside the stake() function in line 87 of the SafEth.sol contract, there's a rounding error of 1 wei before depositing on each derivative, leaving small amounts of ETH in the contract.

uint256 ethAmount = (msg.value * weight) / totalWeight;

Although this can be considered negligible since the minimum deposit is 0.5 ether, it's recommended to have a mechanism of sweeping the loss funds if they become relevant enough. Otherwise, these can be carried in the same line by adding the current contract balance (although it may increase gas costs and be potentially dangerous if the receive() function issue is not resolved).

Derivatives sent to the contract will be unlocked

Since there's no mechanism for accepting ERC20 transfers, the SafEth contract is by default capable of receiving tokens, which means that if a user sends derivative tokens directly to the contract, they'll be stuck in a similar way to the ETH case for the receive() function.

It's recommended to add a function for sweeping the stuck tokens, a mechanism to withdraw them, or a way to account them into their respective IDerivative interface.

Calling unstake() can lock 1 unit of each derivative

Similar to calling stake(), the unstake() function contains a rounding error issue in line 116 for the derivativeAmount variable:

114: // withdraw a percentage of each asset based on the amount of safETH
115: uint256 derivativeAmount = (derivatives[i].balance() *
116:     _safEthAmount) / safEthTotalSupply;

Although the rounding issue is in favor of the protocol and already accounted into the balance, it makes the unstake() function break the invariant of totalSupply() === sum of derivatives[i].balance() assuming each derivative value is 1 ether

Reorder SafEthStorage variables for lesser storage accesses

Considering that stake() is reasonably expected to be the most-called function, it currently accesses 5 storage slots while reading the following variables:

• pauseStaking
• derivativeCount
• totalWeight
• minAmount
• maxAmount

However, both derivativeCount and totalWeight are not expected to grow much, so they can be packed along with pauseStaking, and for the case of minAmount and maxAmount both current values hardcoded in the contract can fit in uin128 to be packed together.

Concretely, the arrangement proposed is the following:

contract SafEthStorage {
    bool public pauseStaking; // true if staking is paused
    bool public pauseUnstaking; // true if unstaking is pause
    uint120 public derivativeCount; // amount of derivatives added to contract
    uint120 public totalWeight; // total weight of all derivatives (used to calculate percentage of derivative)
    uint128 public minAmount; // minimum amount to stake
    uint128 public maxAmount; // maximum amount to stake
    ...
}