Asymmetry contest - SunSec's results

Lines of code

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

Vulnerability details

Impact

The price of derivative tokens is obtained from the current pool price, which makes it susceptible to price manipulation through flash loan. which may pose a risk. Price can be easily manipulated by attackers using flash loans to control the pool's price. So attacker can mint more than expected SafEth token. Then attacker can use SafEth to unsatke to make profit. Therefore, it is advisable to use like Uniswap's TWAP to obtain an average price.

The stake() & unstake() functions in SafEth.sol relies: 1.In Reth.sol ethPerDerivative() fetch the current price of Reth from uniswap. Use poolPrice() to calculate Reth's underlying value, derivativeReceivedEthValue, and the derivative.deposit{} of Reth will be affected as well.

2.In SfrxEth.sol ethPerDerivative() fetch the current price of frxETH from curve pool over price_oracle.

3.In WstEth.sol ethPerDerivative() fetch the current price of WstEth from stETH.getPooledEthByShares(_wstETHAmount) @return the amount of Ether that corresponds to _sharesAmount token shares.

These three prices are the current prices and are susceptible to price manipulation.

Proof of Concept

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

        @notice - Price of derivative in liquidity pool
     */
    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);
    }

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

    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()) *    //@audit reth price can be manipulated over flash loan
                    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}();   //@audit reth price can be manipulated over flash loan
            uint derivativeReceivedEthValue = (derivative.ethPerDerivative(   //@audit reth price can be manipulated over flash loan
                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);
    }

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

            IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle());

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

    function ethPerDerivative(uint256 _amount) public view returns (uint256) {
        return IWStETH(WST_ETH).getStETHByWstETH(10 ** 18);
    }

Tools Used

Manual

Recommended Mitigation Steps

Use TWAP to obtain an average price.

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L88-L91

Vulnerability details

Impact

From the stake function, we can see that if a user stakes 1 ETH, it will be divided by the number of derivatives and transferred to different derivative accounts based on the weights. However, the remaining balance of the ETH will not be returned to the user, but will be kept in the contract.

The unused balance of the ETH depends on the number of derivatives and their respective weights.

Proof of Concept

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L88-L91

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}(); //@audit no return rest of funds to user.
            uint derivativeReceivedEthValue = (derivative.ethPerDerivative(
                depositAmount
            ) * depositAmount) / 10 ** 18;
            totalStakeValueEth += derivativeReceivedEthValue;
        }

Test

let depositAmount = ethers.utils.parseEther("100");
safEthProxy.stake({ value: depositAmount });
output:
ethAmount 33333333333333333333
ethAmount 33333333333333333333
ethAmount 33333333333333333333
ethAmountAfter balance left on safETH contract:1 

Tools Used

Manual

Recommended Mitigation Steps

Refund any excess back to the user.

asymmetry

[L1] one step owner transferRole transfer actions done in a single-step manner are dangerous

Impact

Inheriting from OpenZeppelin's Ownable contract means you are using a single-step ownership transfer pattern. If an admin provides an incorrect address for the new owner this will result in none of the onlyOwner marked methods being callable again. The better way to do this is to use a two-step ownership transfer approach, where the new owner should first claim its new rights before they are transferred.

Proof of Concept

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

import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L6 import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";

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

import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";

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

import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";

Tools Used

Manual Review

Recommended Mitigation Steps

Use OpenZeppelin's Ownable2Step instead of Ownable

[L2] Missing admin input sanitization admin

Impact

Missing maximum value check that is set by the contract owner. This issue can allow the contract owner to set big sllippage or big weight even over 100. It will cause unexpected operactions in protocol.

Proof of Concept

1.https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L165-L174

For example, total weight should be 100 = 50/25/25 (3 derivatives). But there is no maximum threshold validation.

    function adjustWeight(
        uint256 _derivativeIndex,
        uint256 _weight
    ) external onlyOwner {
        weights[_derivativeIndex] = _weight;
        uint256 localTotalWeight = 0;
        for (uint256 i = 0; i < derivativeCount; i++)
            localTotalWeight += weights[i];  //@audit no maximum threshold validation.
        totalWeight = localTotalWeight;
        emit WeightChange(_derivativeIndex, _weight);
    }

2.https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L192

For example, total weight should be 100 = 50/25/25 (3 derivatives). But there is no maximum threshold validation.

    function addDerivative(
        address _contractAddress,
        uint256 _weight
    ) external onlyOwner {
        derivatives[derivativeCount] = IDerivative(_contractAddress);
        weights[derivativeCount] = _weight;
        derivativeCount++;

        uint256 localTotalWeight = 0;
        for (uint256 i = 0; i < derivativeCount; i++)
            localTotalWeight += weights[i];   //@audit no maximum threshold validation.
        totalWeight = localTotalWeight;
        emit DerivativeAdded(_contractAddress, _weight, derivativeCount);
    }
 

3. function setMaxSlippage() - No maximum threshold validation. https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L202

    function setMaxSlippage(
        uint _derivativeIndex,
        uint _slippage
    ) external onlyOwner {
        derivatives[_derivativeIndex].setMaxSlippage(_slippage);
        emit SetMaxSlippage(_derivativeIndex, _slippage);
    }

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

    function setMaxSlippage(uint256 _slippage) external onlyOwner {
        maxSlippage = _slippage;
    }

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

    function setMaxSlippage(uint256 _slippage) external onlyOwner {
        maxSlippage = _slippage;
    }

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

    function setMaxSlippage(uint256 _slippage) external onlyOwner {
        maxSlippage = _slippage;
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Add maximum threshold validation.

[L3] Missing events for critical parameter changing operations by onlyOwner

Impact

onlyOwner only functions that change critical parameters should emit events. There are also no any events log deposit, withdraw, etc. in SfrxEth.sol, Reth.sol and WstEth.sol.

Proof of Concept

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

    function setMaxSlippage(uint256 _slippage) external onlyOwner {
        maxSlippage = _slippage;
    }

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

    function setMaxSlippage(uint256 _slippage) external onlyOwner {
        maxSlippage = _slippage;
    }

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

    function setMaxSlippage(uint256 _slippage) external onlyOwner {
        maxSlippage = _slippage;
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Consider emitting events when these addresses/values are updated by onlyOwner.

[L4] sqrtPriceLimitX96 is set to 0 to disable slippage protection

Impact

sqrtPriceLimitX96 is set to 0 to disable slippage protection in the Reth.sol contract.

Proof of Concept

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

    function swapExactInputSingleHop(
        address _tokenIn,
        address _tokenOut,
        uint24 _poolFee,
        uint256 _amountIn,
        uint256 _minOut
    ) private returns (uint256 amountOut) {
        IERC20(_tokenIn).approve(UNISWAP_ROUTER, _amountIn);
        ISwapRouter.ExactInputSingleParams memory params = ISwapRouter
            .ExactInputSingleParams({
                tokenIn: _tokenIn,
                tokenOut: _tokenOut,
                fee: _poolFee,
                recipient: address(this),
                amountIn: _amountIn,
                amountOutMinimum: _minOut,
                sqrtPriceLimitX96: 0    //@audit disable slippage protection
            });
        amountOut = ISwapRouter(UNISWAP_ROUTER).exactInputSingle(params);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Consider to set sqrtPriceLimitX96.

[L5] Without contract integrity check

Impact

It is recommended to add a "checkContract" function to verify the integrity of the contract code.

Proof of Concept

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

    function addDerivative(
        address _contractAddress,
        uint256 _weight
    ) external onlyOwner {
        derivatives[derivativeCount] = IDerivative(_contractAddress //@audit no contract check
        weights[derivativeCount] = _weight;

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to add a "checkContract" function. Example:

    function checkContract(address _account) internal view {
        require(_account != address(0), "Account cannot be zero address");

        uint256 size;
        // solhint-disable-next-line no-inline-assembly
        assembly { size := extcodesize(_account) }
        require(size > 0, "Account code size cannot be zero");
    }