Asymmetry contest - Sathish9098's results

A protocol to help diversify and decentralize liquid staking derivatives.

General Information

Platform: Code4rena

Start Date: 24/03/2023

Pot Size: $49,200 USDC

Total HM: 20

Participants: 246

Period: 6 days

Judge: Picodes

Total Solo HM: 1

Id: 226

League: ETH

Asymmetry Finance

Findings Distribution

Researcher Performance

Rank: 47/246

Findings: 2

Award: $132.80

QA:

grade-a

Gas:

grade-a

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Asymmetry Finance

Findings Information

🌟 Selected for report: brgltd

Also found by: 0x3b, 0xAgro, 0xGusMcCrae, 0xNorman, 0xRajkumar, 0xSmartContract, 0xTraub, 0xWagmi, 0xWaitress, 0xffchain, 0xhacksmithh, 0xkazim, 0xnev, 3dgeville, ArbitraryExecution, Aymen0909, BRONZEDISC, Bason, Bloqarl, BlueAlder, Brenzee, CodeFoxInc, CodingNameKiki, Cryptor, DadeKuma, DevABDee, Diana, Dug, Englave, Gde, Haipls, HollaDieWaldfee, Ignite, Infect3d, Jerry0x, Josiah, Kaysoft, Koko1912, KrisApostolov, Lavishq, LeoGold, Madalad, PNS, Rappie, RaymondFam, RedTiger, Rickard, Rolezn, Sathish9098, SunSec, T1MOH, UdarTeam, Udsen, Viktor_Cortess, Wander, adriro, ak1, alejandrocovrr, alexzoid, arialblack14, ayden, bin2chen, brevis, btk, c3phas, carlitox477, catellatech, ch0bu, chaduke, ck, climber2002, codeslide, descharre, dingo2077, ernestognw, fatherOfBlocks, favelanky, georgits, helios, hl_, inmarelibero, juancito, ks__xxxxx, lopotras, lukris02, m_Rassska, mahdirostami, maxper, nadin, navinavu, nemveer, p_crypt0, peanuts, pipoca, pixpi, qpzm, rbserver, reassor, roelio, rotcivegaf, scokaf, siddhpurakaran, slvDev, smaul, tnevler, tsvetanovv, turvy_fuzz, vagrant, wen, yac, zzzitron

Awards

42.0604 USDC - $42.06

Labels

bug

grade-a

QA (Quality Assurance)

sponsor acknowledged

edited-by-warden

Q-142

External Links

Link to GitHub issue

LOW FINDINGS

LOW

	Issues	Instances
[L-1]	UNUSED RECEIVE() functions	4
[L-2]	Imports can be grouped together	3
[L-3]	Sanity/Threshold/Limit Checks	6
[L-4]	LOSS OF PRECISION DUE TO ROUNDING	9
[L-5]	A single point of failure	17
[L-6]	Use safe variant of _mint() function	1
[L-7]	No Storage Gap for SafEth contract	7
[L-8]	Prevent division by 0	5
[L-9]	Consider using OpenZeppelin’s SafeCast library to prevent unexpected behavior when casting uint	1
[L-10]	abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()	6
[L-11]	Missing Event for critical parameters init and change	4
[L-12]	Gas griefing/theft is possible on unsafe external call	3
[L-13]	Front running attacks by the onlyOwner	2
[L-14]	Function Calls in Loop Could Lead to Denial of Service	4
[L-15]	Use safeTransferOwnership instead of transferOwnership function	4

NON CRITICAL

	Issues	Instances
[NC-1]	For modern and more readable code; update import usages	34
[NC-2]	Imports can be grouped together	-
[NC-3]	AVOID HARDCODED VALUES	11
[NC-4]	Shorter the inheritance	1
[NC-5]	EMPTY BLOCKS SHOULD BE REMOVED OR EMIT SOMETHING	5
[NC-6]	ADD PARAMETER TO EVENT-EMIT	1
[NC-7]	Pragma float	7
[NC-8]	Interchangeable usage of uint and uint256	4
[NC-9]	TYPOS	1
[NC-10]	Include (@param and @return) parameters in NatSpec comments	-
[NC-11]	NatSpec comments should be increased in contracts	-
[NC-12]	Function writing that does not comply with the Solidity Style Guide	-
[NC-13]	Add a timelock to critical functions	4
[NC-14]	For functions, follow Solidity standard naming conventions (private function style rule)	3
[NC-15]	Missing NatSpec	9
[NC-16]	Use scientific notation instead of exponential notation	12
[NC-17]	String constants used in multiple places should be defined as constants	6
[NC-18]	Events that mark critical parameter changes should contain both the old and the new value	3
[NC-19]	NO SAME VALUE INPUT CONTROL	4
[NC-20]	NOT USING THE NAMED RETURN VARIABLES ANYWHERE IN THE FUNCTION IS CONFUSING	1
[NC-21]	Use a more recent version of Solidity	4
[NC-22]	Use latest and same versions of openzeppelin contracts	22
[NC-23]	Implement some type of version counter that will be incremented automatically for contract upgrades	-
[NC-24]	Generate perfect code headers every time	-

[L-1] UNUSED RECEIVE() functions

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol

97:  receive() external payable {}

WstEth.sol#L97

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/SfrxEth.sol

126: receive() external payable {}

SfrxEth.sol#L126

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

246: receive() external payable {}

SafEth.sol#L246

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol

244: receive() external payable {}

[L-2] MISSING CHECKS FOR ADDRESS(0X0) WHEN TRANSFER OWNERSHIP

Owner address is not checked before calling _transferOwnership function

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol

function initialize(address _owner) external initializer {
        _transferOwnership(_owner);
        maxSlippage = (1 * 10 ** 16); // 1%
}

WstEth.sol#L33-L36

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/SfrxEth.sol

function initialize(address _owner) external initializer {
        _transferOwnership(_owner);
        maxSlippage = (1 * 10 ** 16); // 1%
}

SfrxEth.sol#L36-L39

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol

function initialize(address _owner) external initializer {
        _transferOwnership(_owner);
        maxSlippage = (1 * 10 ** 16); // 1%
}

Reth.sol#L42-L45

Recommendations

Add necessary address(0) check before tranferownership to new address

[L-3] Sanity/Threshold/Limit Checks

Devoid of sanity/threshold/limit checks, critical parameters can be configured to invalid values, causing a variety of issues and breaking expected interactions within/between contracts.

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol

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

}

WstEth.sol#L48-L50

WstEth.sol#L56-L67

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/SfrxEth.sol

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

SfrxEth.sol#L51-L53

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

function setMaxAmount(uint256 _maxAmount) external onlyOwner {
        maxAmount = _maxAmount;
        emit ChangeMaxAmount(maxAmount);
}

SafEth.sol#L223-L226

   function setMinAmount(uint256 _minAmount) external onlyOwner {
        minAmount = _minAmount;
        emit ChangeMinAmount(minAmount);
    }

SafEth.sol#L214-L217

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

SafEth.sol#L202-L208

Recommendations:

Add the sanity/threshold/limit checks before assigning critical variable values

[L-4] LOSS OF PRECISION DUE TO ROUNDING

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol

60: uint256 minOut = (stEthBal * (10 ** 18 - maxSlippage)) / 10 ** 18;

WstEth.sol#L60

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/SfrxEth.sol

uint256 minOut = (((ethPerDerivative(_amount) * _amount) / 10 ** 18) * (10 ** 18 - maxSlippage)) / 10 ** 18;

SfrxEth.sol#L74-L75

    return ((10 ** 18 * frxAmount) / IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle());

SfrxEth.sol#L115-L116

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

81:  else preDepositPrice = (10 ** 18 * underlyingValue) / totalSupply;

SafEth.sol#L81

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

SafEth.sol#L88

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

SafEth.sol#L92-L94

98: uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;

SafEth.sol#L98


uint256 derivativeAmount = (derivatives[i].balance() * _safEthAmount) / safEthTotalSupply;

SafEth.sol#L115-L116

uint256 ethAmount = (ethAmountToRebalance * weights[i]) / totalWeight;

SafEth.sol#L149-L150

[L-5] A single point of failure

The onlyOwner role has a single point of failure and onlyOwner can use critical a few functions.

Even if protocol admins/developers are not malicious there is still a chance for Owner keys to be stolen. In such a case, the attacker can cause serious damage to the project due to important functions. In such a case, users who have invested in project will suffer high financial losses

File: contracts/SafEth/SafEth.sol


138:     function rebalanceToWeights() external onlyOwner {

168:     ) external onlyOwner {

185:     ) external onlyOwner {

205:     ) external onlyOwner {

214:     function setMinAmount(uint256 _minAmount) external onlyOwner {

223:     function setMaxAmount(uint256 _maxAmount) external onlyOwner {

232:     function setPauseStaking(bool _pause) external onlyOwner {

241:     function setPauseUnstaking(bool _pause) external onlyOwner {

File: contracts/SafEth/derivatives/Reth.sol


58:     function setMaxSlippage(uint256 _slippage) external onlyOwner {

107:     function withdraw(uint256 amount) external onlyOwner {

156:     function deposit() external payable onlyOwner returns (uint256) {

File: contracts/SafEth/derivatives/SfrxEth.sol


51:     function setMaxSlippage(uint256 _slippage) external onlyOwner {

60:     function withdraw(uint256 _amount) external onlyOwner {

94:     function deposit() external payable onlyOwner returns (uint256) {

File: contracts/SafEth/derivatives/WstEth.sol


48:     function setMaxSlippage(uint256 _slippage) external onlyOwner {

56:     function withdraw(uint256 _amount) external onlyOwner {

73:     function deposit() external payable onlyOwner returns (uint256) {

Recommended Mitigation Steps

Add a time lock to critical functions. Admin-only functions that change critical parameters should emit events and have timelocks.

Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services.

Also detail them in documentation and NatSpec comments

[L-6] Use safe variant of _mint() function

.mint won’t check if the recipient is able to receive the tokens. If an incorrect address is passed, it will result in a silent failure and loss of asset.

OpenZeppelin recommendation is to use the safe variant of _mint

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

99:  _mint(msg.sender, mintAmount);

SafEth.sol#L99

Recommendation

Replace _mint() with _safeMint()

[L-7] No Storage Gap for SafEth contract

Impact For upgradeable contracts, inheriting contracts may introduce new variables. In order to be able to add new variables to the upgradeable contract without causing storage collisions, a storage gap should be added to the upgradeable contract.

If no storage gap is added, when the upgradable contract introduces new variables, it may override the variables in the inheriting contract.

Storage gaps are a convention for reserving storage slots in a base contract, allowing future versions of that contract to use up those slots without affecting the storage layout of child contracts. To create a storage gap, declare a fixed-size array in the base contract with an initial number of slots. This can be an array of uint256 so that each element reserves a 32 byte slot. Use the naming convention __gap so that OpenZeppelin Upgrades will recognize the gap:

Storage Gaps This makes the storage layouts incompatible, as explained in Writing Upgradeable Contracts. The size of the __gap array is calculated so that the amount of storage used by a contract always adds up to the same number (in this case 50 storage slots).

SafEth.sol#L15-L20

Recommended Mitigation Steps

Consider adding a storage gap at the end of the upgradeable abstract contract

uint256[50] private __gap;

[L-8] Prevent division by 0

On several locations in the code precautions are not being taken for not dividing by 0, this will revert the code. These functions can be called with 0 value in the input, this value is not checked for being bigger than 0, that means in some scenarios this can potentially trigger a division by zero

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

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

SafEth.sol#L88

98: uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;

SafEth.sol#L98

115: uint256 derivativeAmount = (derivatives[i].balance() * _safEthAmount) / safEthTotalSupply;

SafEth.sol#L115-L116

149: uint256 ethAmount = (ethAmountToRebalance * weights[i]) /totalWeight;

SafEth.sol#L149-L150)

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol

171: uint rethPerEth = (10 ** 36) / poolPrice();

Reth.sol#L171

Recommendations:

Make sure the denominator value is not 0

[L-9] Consider using OpenZeppelin’s SafeCast library to prevent unexpected behavior when casting uint

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol

241: return (sqrtPriceX96 * (uint(sqrtPriceX96)) * (1e18)) >> (96 * 2);

derivatives/Reth.sol#L241

Recommendations:

Use OpenZeppelin’s SafeCast library

[L-10] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead. If all arguments are strings and or bytes, bytes.concat() should be used instead

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol

  RocketStorageInterface(ROCKET_STORAGE_ADDRESS).getAddress(
                keccak256(
                    abi.encodePacked("contract.address", "rocketTokenRETH")
                )
            );

Reth.sol#L68-L72

     address rocketDepositPoolAddress = RocketStorageInterface(
            ROCKET_STORAGE_ADDRESS
        ).getAddress(
                keccak256(
                    abi.encodePacked("contract.address", "rocketDepositPool")
                )
            );

Reth.sol#L121-L127

    address rocketProtocolSettingsAddress = RocketStorageInterface(
            ROCKET_STORAGE_ADDRESS
        ).getAddress(
                keccak256(
                    abi.encodePacked(
                        "contract.address",
                        "rocketDAOProtocolSettingsDeposit"
                    )
                )
            );

Reth.sol#L132-L141

     address rocketDepositPoolAddress = RocketStorageInterface(
            ROCKET_STORAGE_ADDRESS
        ).getAddress(
                keccak256(
                    abi.encodePacked("contract.address", "rocketDepositPool")
                )
            );

Reth.sol#L158-L164

    address rocketTokenRETHAddress = RocketStorageInterface(
                ROCKET_STORAGE_ADDRESS
            ).getAddress(
                    keccak256(
                        abi.encodePacked("contract.address", "rocketTokenRETH")
                    )
                );

Reth.sol#L187-L193

    address rocketTokenRETHAddress = RocketStorageInterface(
            ROCKET_STORAGE_ADDRESS
        ).getAddress(
                keccak256(
                    abi.encodePacked("contract.address", "rocketTokenRETH")
                )
            );

Reth.sol#L229-L235

Recommendations:

abi.encode should be preferred

[L-11] Missing Event for critical parameters init and change

Description

Events help non-contract tools to track changes, and events prevent users from being surprised by changes.

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol

    function initialize(address _owner) external initializer {
        _transferOwnership(_owner);
        maxSlippage = (1 * 10 ** 16); // 1%
    }

WstEth.sol#L33-L36

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/SfrxEth.sol

    function initialize(address _owner) external initializer {
        _transferOwnership(_owner);
        maxSlippage = (1 * 10 ** 16); // 1%
    }

SfrxEth.sol#L36-L39

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

    function initialize(
        string memory _tokenName,
        string memory _tokenSymbol
    ) external initializer {
        ERC20Upgradeable.__ERC20_init(_tokenName, _tokenSymbol);
        _transferOwnership(msg.sender);
        minAmount = 5 * 10 ** 17; // initializing with .5 ETH as minimum
        maxAmount = 200 * 10 ** 18; // initializing with 200 ETH as maximum
    }

SafEth.sol#L48-L56

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol

   function initialize(address _owner) external initializer {
        _transferOwnership(_owner);
        maxSlippage = (1 * 10 ** 16); // 1%
    }

Reth.sol#L42-L45

Recommendation

Add Event-Emit

[L-12] Gas griefing/theft is possible on unsafe external call

return data (bool success,) has to be stored due to EVM architecture, if in a usage like below, ‘out’ and ‘outsize’ values are given (0,0) . Thus, this storage disappears and may come from external contracts a possible Gas griefing/theft problem is avoided

https://twitter.com/pashovkrum/status/1607024043718316032?t=xs30iD6ORWtE2bTTYsCFIQ&s=19

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol


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

SafEth.sol#L124-L126

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol

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

WstEth.sol#L76,WstEth.sol#L63-L65

recommanded Mitigations:

assembly {                                    
                success := call(gas(), dest, amount, 0, 0)
         }

[L-13] Front running attacks by the onlyOwner

Attacker can front run the minAmount, maxAmount limitations. If minAmount increased still attacker can frontrun and stake their ETH with old values. Same way for maxAmount values

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

function setMinAmount(uint256 _minAmount) external onlyOwner {
        minAmount = _minAmount;
        emit ChangeMinAmount(minAmount);
    }

function setMaxAmount(uint256 _maxAmount) external onlyOwner {
        maxAmount = _maxAmount;
        emit ChangeMaxAmount(maxAmount);
    }

SafEth.sol#L214-L226

Recommended Mitigation:

Use a timelock to avoid instant changes of the parameters

[L-14] Function Calls in Loop Could Lead to Denial of Service

Function calls made in unbounded loop are error-prone with potential resource exhaustion as it can trap the contract due to the gas limitations or failed transactions. Here are some of the instances entailed.

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

[L-15] Use safeTransferOwnership instead of transferOwnership function

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol

function initialize(address _owner) external initializer {
        _transferOwnership(_owner);
        maxSlippage = (1 * 10 ** 16); // 1%
}

WstEth.sol#L33-L36

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/SfrxEth.sol

function initialize(address _owner) external initializer {
        _transferOwnership(_owner);
        maxSlippage = (1 * 10 ** 16); // 1%
}

SfrxEth.sol#L36-L39

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol

function initialize(address _owner) external initializer {
        _transferOwnership(_owner);
        maxSlippage = (1 * 10 ** 16); // 1%
}

Reth.sol#L42-L45

Description:

transferOwnership function is used to change Ownership

Use a 2 structure transferOwnership which is safer. safeTransferOwnership, use it is more secure due to 2-stage ownership transfer

Recommendation:

Use Ownable2Step.sol

NON CRITICAL FINDINGS

[NC-1] For modern and more readable code; update import usages

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol

import "../../interfaces/IDerivative.sol";
import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "../../interfaces/curve/IStEthEthPool.sol";
import "../../interfaces/lido/IWStETH.sol";

WstEth.sol#L4-L8

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/SfrxEth.sol

import "../../interfaces/IDerivative.sol";
import "../../interfaces/frax/IsFrxEth.sol";
import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "../../interfaces/curve/IFrxEthEthPool.sol";
import "../../interfaces/frax/IFrxETHMinter.sol";

SfrxEth.sol#L4-L9

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "../interfaces/IWETH.sol";
import "../interfaces/uniswap/ISwapRouter.sol";
import "../interfaces/lido/IWStETH.sol";
import "../interfaces/lido/IstETH.sol";
import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import "./SafEthStorage.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol";

SafEth.sol#L4-L11

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol

import "../../interfaces/IDerivative.sol";
import "../../interfaces/frax/IsFrxEth.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "../../interfaces/rocketpool/RocketStorageInterface.sol";
import "../../interfaces/rocketpool/RocketTokenRETHInterface.sol";
import "../../interfaces/rocketpool/RocketDepositPoolInterface.sol";
import "../../interfaces/rocketpool/RocketDAOProtocolSettingsDepositInterface.sol";
import "../../interfaces/IWETH.sol";
import "../../interfaces/uniswap/ISwapRouter.sol";
import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import "../../interfaces/uniswap/IUniswapV3Factory.sol";
import "../../interfaces/uniswap/IUniswapV3Pool.sol";

Reth.sol#L4-L15

Recommendation

import {contract1 , contract2} from "filename.sol";

[NC-2] Imports can be grouped together

Consider importing interfaces first, then OpenZeppelin imports

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol

import "../../interfaces/IDerivative.sol";
import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "../../interfaces/curve/IStEthEthPool.sol";
import "../../interfaces/lido/IWStETH.sol";

WstEth.sol#L4-L8

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/SfrxEth.sol

import "../../interfaces/IDerivative.sol";
import "../../interfaces/frax/IsFrxEth.sol";
import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "../../interfaces/curve/IFrxEthEthPool.sol";
import "../../interfaces/frax/IFrxETHMinter.sol";

SfrxEth.sol#L4-L9

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "../interfaces/IWETH.sol";
import "../interfaces/uniswap/ISwapRouter.sol";
import "../interfaces/lido/IWStETH.sol";
import "../interfaces/lido/IstETH.sol";
import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import "./SafEthStorage.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol";

SafEth.sol#L4-L11

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol

import "../../interfaces/IDerivative.sol";
import "../../interfaces/frax/IsFrxEth.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "../../interfaces/rocketpool/RocketStorageInterface.sol";
import "../../interfaces/rocketpool/RocketTokenRETHInterface.sol";
import "../../interfaces/rocketpool/RocketDepositPoolInterface.sol";
import "../../interfaces/rocketpool/RocketDAOProtocolSettingsDepositInterface.sol";
import "../../interfaces/IWETH.sol";
import "../../interfaces/uniswap/ISwapRouter.sol";
import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import "../../interfaces/uniswap/IUniswapV3Factory.sol";
import "../../interfaces/uniswap/IUniswapV3Pool.sol";

Reth.sol#L4-L15

[NC-3] Hardcode the address causes no future updates

In case the addresses change due to reasons such as updating their versions in the future, addresses coded as constants cannot be updated

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol

address public constant WST_ETH = 0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0;
address public constant LIDO_CRV_POOL = 0xDC24316b9AE028F1497c275EB9192a3Ea0f67022;
address public constant STETH_TOKEN = 0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84

WstEth.sol#L13-L18

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/SfrxEth.sol

address public constant SFRX_ETH_ADDRESS = 0xac3E018457B222d93114458476f3E3416Abbe38F;
address public constant FRX_ETH_ADDRESS = 0x5E8422345238F34275888049021821E8E08CAa1f;
address public constant FRX_ETH_CRV_POOL_ADDRESS = 0xa1F8A6807c402E4A15ef4EBa36528A3FED24E577;
address public constant FRX_ETH_MINTER_ADDRESS = 0xbAFA44EFE7901E04E39Dad13167D089C559c1138;

SfrxEth.sol#L14-L21

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol

address public constant ROCKET_STORAGE_ADDRESS = 0x1d8f8f00cfa6758d7bE78336684788Fb0ee0Fa46;
address public constant W_ETH_ADDRESS = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;
address public constant UNISWAP_ROUTER = 0x68b3465833fb72A70ecDF485E0e4C7bD8665Fc45;
address public constant UNI_V3_FACTORY = 0x1F98431c8aD98523631AE4a59f267346ea31F984;

Reth.sol#L20-L27

Recommendations:

It is recommended to add the update option with the onlyOwner modifier

[NC-4] Shorter the inheritance

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

   contract SafEth is
    Initializable,
    ERC20Upgradeable,
    OwnableUpgradeable,
    SafEthStorage

SafEth/SafEth.sol#L15-L19

[NC-5] EMPTY BLOCKS SHOULD BE REMOVED OR EMIT SOMETHING

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol

97:  receive() external payable {}

WstEth.sol#L97

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/SfrxEth.sol

126: receive() external payable {}

SfrxEth.sol#L126

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

246: receive() external payable {}

SafEth.sol#L246

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol

244: receive() external payable {}

[NC-6] ADD PARAMETER TO EVENT-EMIT

Some event-emit description hasn’t parameter. Add to parameter for front-end website or client app , they can has that something has happened on the blockchain

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

34: event Rebalanced();

SafEth.sol#L34

[NC-7] Pragma float

All the contracts in scope are floating the pragma version.

Recommendation

Locking the pragma helps to ensure that contracts do not accidentally get deployed using an outdated compiler version.

Note that pragma statements can be allowed to float when a contract is intended for consumption by other developers, as in the case with contracts in a library or a package.

[NC-8] Interchangeable usage of uint and uint256

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L91-L92 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L171-L173

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

SafEth.sol#L202-L208

[NC-9] TYPOS

FILE: 2023-03-asymmetry/contracts/SafEth/SafEth.sol

/// @audit derivates => derivatives 
- 160:    @dev - If you want exact weights either do the math off chain or reset all existing derivates to the weights you want
+ 160:    @dev - If you want exact weights either do the math off chain or reset all existing derivatives to the weights you want

[NC-10] Include (@param and @return) parameters in NatSpec comments

Context All Contracts

Description It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.

natspec-format.html

Recommendation

Include return and function arguments(@param,@return) parameters in NatSpec comments

Recommendation Code Style: (from Uniswap3)

    /// @notice Adds liquidity for the given recipient/tickLower/tickUpper position
    /// @dev The caller of this method receives a callback in the form of IUniswapV3MintCallback#uniswapV3MintCallback
    /// in which they must pay any token0 or token1 owed for the liquidity. The amount of token0/token1 due depends
    /// on tickLower, tickUpper, the amount of liquidity, and the current price.
    /// @param recipient The address for which the liquidity will be created
    /// @param tickLower The lower tick of the position in which to add liquidity
    /// @param tickUpper The upper tick of the position in which to add liquidity
    /// @param amount The amount of liquidity to mint
    /// @param data Any data that should be passed through to the callback
    /// @return amount0 The amount of token0 that was paid to mint the given amount of liquidity. Matches the value in the callback
    /// @return amount1 The amount of token1 that was paid to mint the given amount of liquidity. Matches the value in the callback
    function mint(
        address recipient,
        int24 tickLower,
        int24 tickUpper,
        uint128 amount,
        bytes calldata data
    ) external returns (uint256 amount0, uint256 amount1);

[NC-11] NatSpec comments should be increased in contracts

Context

All Contracts

Description:

It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation.

In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.

https://docs.soliditylang.org/en/v0.8.15/natspec-format.html

Recommendation

NatSpec comments should be increased in contracts

[NC-12] Function writing that does not comply with the Solidity Style Guide

Context

All Contracts

Description Order of Functions; ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier. But there are contracts in the project that do not comply with this.

https://docs.soliditylang.org/en/v0.8.17/style-guide.html

Functions should be grouped according to their visibility and ordered:

constructor
receive function (if exists)
fallback function (if exists)
external
public
internal
private
within a grouping, place the view and pure functions last

[N-13] Add a timelock to critical functions

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user). Consider adding a timelock to

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol

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

WstEth.sol#L48-L50

WstEth.sol#L56-L67

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/SfrxEth.sol

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

SfrxEth.sol#L51-L53

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

function setMaxAmount(uint256 _maxAmount) external onlyOwner {
        maxAmount = _maxAmount;
        emit ChangeMaxAmount(maxAmount);
}

SafEth.sol#L223-L226

   function setMinAmount(uint256 _minAmount) external onlyOwner {
        minAmount = _minAmount;
        emit ChangeMinAmount(minAmount);
    }

SafEth.sol#L214-L217

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

SafEth.sol#L202-L208

[NC-14] For functions, follow Solidity standard naming conventions (private function style rule)

Description

The above codes don’t follow Solidity’s standard naming convention,

For private functions : the mixedCase format starting with an underscore (_mixedCase starting with an underscore)

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol


66: function rethAddress() private view returns (address) {

function swapExactInputSingleHop(
        address _tokenIn,
        address _tokenOut,
        uint24 _poolFee,
        uint256 _amountIn,
        uint256 _minOut
) private returns (uint256 amountOut) {

120:  function poolCanDeposit(uint256 _amount) private view returns (bool) {

228:  function poolPrice() private view returns (uint256) {

[NC-15] Missing NatSpec

Consider adding specification to the following code blocks

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

event ChangeMinAmount(uint256 indexed minAmount);
event ChangeMaxAmount(uint256 indexed maxAmount);
event StakingPaused(bool indexed paused);
event UnstakingPaused(bool indexed paused);
event SetMaxSlippage(uint256 indexed index, uint256 slippage);
event Staked(address indexed recipient, uint ethIn, uint safEthOut);
event Unstaked(address indexed recipient, uint ethOut, uint safEthIn);
event WeightChange(uint indexed index, uint weight);
event DerivativeAdded(
        address indexed contractAddress,
        uint weight,
        uint index
    );
event Rebalanced();

SafEth.sol#L21-L34

[NC-16] Use scientific notation instead of exponential notation

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol


35: maxSlippage = (1 * 10 ** 16); // 1%
60: uint256 minOut = (stEthBal * (10 ** 18 - maxSlippage)) / 10 ** 18;
87: return IWStETH(WST_ETH).getStETHByWstETH(10 ** 18);

WstEth.sol#L35,WstEth.sol#L60,WstEth.sol#L87

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/SfrxEth.sol


38: maxSlippage = (1 * 10 ** 16); // 1%
74: uint256 minOut = (((ethPerDerivative(_amount) * _amount) / 10 ** 18) *
            (10 ** 18 - maxSlippage)) / 10 ** 18;
112: uint256 frxAmount = IsFrxEth(SFRX_ETH_ADDRESS).convertToAssets(
            10 ** 18
        );
116: return ((10 ** 18 * frxAmount) /
            IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle());

SfrxEth.sol#L38,SfrxEth.sol#L74-L75,SfrxEth.sol#L112-L116

FILE: 2023-03-asymmetry/contracts/SafEth/SafEth.sol

75:  10 ** 18;
80:  preDepositPrice = 10 ** 18; // initializes with a price of 1
81:  else preDepositPrice = (10 ** 18 * underlyingValue) / totalSupply;
94:  ) * depositAmount) / 10 ** 18;
98:  uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;

SafEth.sol#L75,SafEth.sol#L80-L81,SafEth.sol#L94,SafEth.sol#L98

[NC-17] String constants used in multiple places should be defined as constants

contract.address and rocketTokenRETH should be defined rathar than using multiple times

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L136-L139 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L125 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L70 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L162 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L191 https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L233

[NC-18] Events that mark critical parameter changes should contain both the old and the new value

This should especially be done if the new value is not required to be different from the old value

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

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

function setMaxAmount(uint256 _maxAmount) external onlyOwner {
        maxAmount = _maxAmount;
        emit ChangeMaxAmount(maxAmount);
}

SafEth.sol#L223-L226

   function setMinAmount(uint256 _minAmount) external onlyOwner {
        minAmount = _minAmount;
        emit ChangeMinAmount(minAmount);
    }

SafEth.sol#L214-L217

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

SafEth.sol#L202-L208

[N-19] NO SAME VALUE INPUT CONTROL

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

function setMaxAmount(uint256 _maxAmount) external onlyOwner {
        maxAmount = _maxAmount;
        emit ChangeMaxAmount(maxAmount);
}

SafEth.sol#L223-L226

   function setMinAmount(uint256 _minAmount) external onlyOwner {
        minAmount = _minAmount;
        emit ChangeMinAmount(minAmount);
    }

SafEth.sol#L214-L217

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

SafEth.sol#L202-L208

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

Recommended Mitigation Steps:

Add code like this

if (oracle == _oracle revert SAME VALUE);

[NC-20] NOT USING THE NAMED RETURN VARIABLES ANYWHERE IN THE FUNCTION IS CONFUSING

Consider changing the variable to be an unnamed one

Reth.sol#L83-L89

[NC-21] Use a more recent version of Solidity

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol

2: pragma solidity ^0.8.13;

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/SfrxEth.sol

2: pragma solidity ^0.8.13;

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

2: pragma solidity ^0.8.13;

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol

2: pragma solidity ^0.8.13;

[NC-22] Use latest and same versions of openzeppelin contracts

The latest openzeppelin version is 4.8.2

FILE : Package.json

82: "@openzeppelin/contracts": "^4.8.0", 83: "@openzeppelin/contracts-upgradeable": "^4.8.1",

[NC-23] Implement some type of version counter that will be incremented automatically for contract upgrades

I suggest implementing some kind of version counter that will be incremented automatically when you upgrade the contract

Recommended Mitigation Steps

uint256 public authorizeUpgradeCounter;
 function upgradeTo(address _newImplementation) external onlyOwner {
        _setPendingImplementation(_newImplementation);
       authorizeUpgradeCounter+=1;
    }

[NC-24] Generate perfect code headers every time

Description: I recommend using header for Solidity code layout and readability

https://github.com/transmissions11/headers

/////////////////////////////////////////////////////////////// TESTING 123 ///////////////////////////////////////////////////////////////

#0 - c4-sponsor
2023-04-07T22:18:36Z

elmutt marked the issue as sponsor acknowledged

#1 - c4-judge
2023-04-24T19:06:58Z

Picodes marked the issue as grade-a

Findings Information

🌟 Selected for report: Rolezn

Also found by: 0x3b, 0xGordita, 0xSmartContract, 0xhacksmithh, 0xnev, 0xpanicError, 4lulz, Angry_Mustache_Man, ArbitraryExecution, Aymen0909, Bason, BlueAlder, EvanW, Franfran, HHK, Haipls, IgorZuk, JCN, KrisApostolov, Madalad, MiksuJak, MiniGlome, RaymondFam, ReyAdmirado, Rickard, Sathish9098, Udsen, adriro, alexzoid, anodaram, arialblack14, c3phas, carlitox477, ch0bu, chaduke, codeslide, d3e4, dicethedev, ernestognw, fatherOfBlocks, georgits, hunter_w3b, inmarelibero, lukris02, mahdirostami, maxper, pavankv, pixpi, rotcivegaf, smaul, tank, tnevler, wen, yac

Awards

90.7432 USDC - $90.74

Labels

bug

G (Gas Optimization)

grade-a

sponsor confirmed

edited-by-warden

G-54

External Links

Link to GitHub issue

GAS OPTIMIZATIONS

	Issues	Instances
[G-1]	Use a more recent version of solidity	4
[G-2]	Setting the constructor to payable	4
[G-3]	OPTIMIZE NAMES TO SAVE GAS	37
[G-4]	Don't declare the variables inside the loops costs more gas	11
[G-5]	Avoid contract existence checks by using low level calls	19
[G-6]	ADD UNCHECKED {} FOR SUBTRACTIONS WHERE THE OPERANDS CANNOT UNDERFLOW BECAUSE OF A PREVIOUS REQUIRE() OR IF-STATEMENT	1
[G-7]	Public function not called by contract declare external to save gas	2
[G-8]	With assembly, .call (bool success) transfer can be done gas-optimized	3
[G-9]	Empty blocks should be removed or emit something	4
[G-10]	Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead	2
[G-11]	Private functions only called once can be inlined to save gas	1
[G-12]	Make 3 event parameters indexed when possible	5
[G-13]	Save gas with address(0) checks before transfer ownership	4
[G-14]	Gas overflow during iteration (DoS)	4
[G-15]	Sort Solidity operations using short-circuit mode	1
[G-16]	Save gas with the use of the specific import statement	4
[G-17]	Instead of calculating the value with keccak256() every time the contract is made pre calculate them before and only give the result to a constant	3

[G-1] Use a more recent version of solidity

INSTANCES (4):

In 0.8.15 the conditions necessary for inlining are relaxed. Benchmarks show that the change significantly decreases the bytecode size (which impacts the deployment cost) while the effect on the runtime gas usage is smaller.

In 0.8.17 prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call; Simplify the starting offset of zero-length operations to zero. More efficient overflow checks for multiplication.

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol

2: pragma solidity ^0.8.13;

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/SfrxEth.sol

2: pragma solidity ^0.8.13;

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

2: pragma solidity ^0.8.13;

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol

2: pragma solidity ^0.8.13;

[G-2] Setting the constructor to payable

INSTANCES (4):

APPRIMATE GAS SAVED : 52 GAS

You can cut out 10 opcodes in the creation-time EVM bytecode if you declare a constructor payable. Making the constructor payable eliminates the need for an initial check of msg.value == 0 and saves 13 gas on deployment with no security risks

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol

24:    constructor() {

WstEth.sol#L24

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/SfrxEth.sol

27:   constructor() {

SfrxEth.sol#L27

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

38: constructor() {

SafEth.sol#L38

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol

33:    constructor() {

Reth.sol#L33

[G-3] OPTIMIZE NAMES TO SAVE GAS

INSTANCES (37):

public/external function names and public member variable names can be optimized to save gas. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol

/// @audit 
initialize(),name(),setMaxSlippage(),withdraw(),deposit(),ethPerDerivative(),balance()
12: contract WstEth is IDerivative, Initializable, OwnableUpgradeable {

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/SfrxEth.sol

/// @audit 
initialize(),name(),setMaxSlippage(),withdraw(),deposit(),ethPerDerivative(),balance()
13: contract SfrxEth is IDerivative, Initializable, OwnableUpgradeable {

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

/// @audit 
initialize(),stake(),unstake(),rebalanceToWeights(),adjustWeight(),addDerivative(),setMaxSlippage(),setMinAmount(),setMaxAmount(),setPauseStaking(),setPauseUnstaking()
15: contract SafEth is

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol

/// @audit 
initialize(),name(),setMaxSlippage(),rethAddress(),swapExactInputSingleHop(),withdraw(),poolCanDeposit(),deposit(),ethPerDerivative(),balance(),poolPrice()
19: contract Reth is IDerivative, Initializable, OwnableUpgradeable {

[G-4] Don't declare the variables inside the loops costs more gas

INSTANCES (11):

APPRIMATE GAS SAVED : 99 GAS

GAS SAMPLE TEST IN remix ide(Without gas optimizations) :

Before Mitigation :

     function testGas() public view {

        for(uint256 i = 0; i < 10; i++) {
          address collateral = msg.sender;
          address  collateral1 = msg.sender;
            
        }

The execution Cost : 2176

After Mitigation :

     function testGas() public view {
    address collateral; address  collateral1;
        for(uint256 i = 0; i < 10; i++) {
         collateral = msg.sender;
         collateral1 = msg.sender;
            
        }

The execution Cost : 2086 .

Hence clearly after mitigation the gas cost is reduced. Approximately its possible to save 9 gas for every iterations

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

SafEth.sol#L71-L96 SafEth.sol#L113-L119 SafEth.sol#L140-L153

[G-5] Avoid contract existence checks by using low level calls

INSTANCES (19):

APPRIMATE GAS SAVED : 1900 GAS

Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (100 gas), to check for contract existence for external function calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value. Similar behavior can be achieved in earlier versions by using low-level calls, since low level calls never check for contract existence

FILE: 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol

90:    IERC20(_tokenIn).approve(UNISWAP_ROUTER, _amountIn);
101:   amountOut = ISwapRouter(UNISWAP_ROUTER).exactInputSingle(params);
108:   RocketTokenRETHInterface(rethAddress()).burn(amount);
197:   uint256 rethBalance1 = rocketTokenRETH.balanceOf(address(this));
199:   uint256 rethBalance2 = rocketTokenRETH.balanceOf(address(this));
222:   return IERC20(rethAddress()).balanceOf(address(this));

Reth.sol#L90,Reth.sol#L101,Reth.sol#L108,Reth.sol#L197,Reth.sol#L199,Reth.sol#L222

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/SfrxEth.sol

IsFrxEth(SFRX_ETH_ADDRESS).redeem(
           _amount,
            address(this),
            address(this)
        );
        uint256 frxEthBalance = IERC20(FRX_ETH_ADDRESS).balanceOf(
            address(this)
        );
        IsFrxEth(FRX_ETH_ADDRESS).approve(
            FRX_ETH_CRV_POOL_ADDRESS,
            frxEthBalance
        );

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

   uint256 sfrxBalancePre = IERC20(SFRX_ETH_ADDRESS).balanceOf(
            address(this)
        );

   uint256 sfrxBalancePost = IERC20(SFRX_ETH_ADDRESS).balanceOf(
            address(this)
        );

112: uint256 frxAmount = IsFrxEth(SFRX_ETH_ADDRESS).convertToAssets(
            10 ** 18
        );
115: return ((10 ** 18 * frxAmount) /
            IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle());

123: return IERC20(SFRX_ETH_ADDRESS).balanceOf(address(this));

SfrxEth.sol#L61-L84,SfrxEth.sol#L95-L104,SfrxEth.sol#L112-L116,SfrxEth.sol#L123

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol

101:  amountOut = ISwapRouter(UNISWAP_ROUTER).exactInputSingle(params);
197:  uint256 rethBalance1 = rocketTokenRETH.balanceOf(address(this));
199:  uint256 rethBalance2 = rocketTokenRETH.balanceOf(address(this));
222:  return IERC20(rethAddress()).balanceOf(address(this));

Reth.sol#L101,Reth.sol#L197,Reth.sol#L199,Reth.sol#L222

[G-6] ADD UNCHECKED {} FOR SUBTRACTIONS WHERE THE OPERANDS CANNOT UNDERFLOW BECAUSE OF A PREVIOUS REQUIRE() OR IF-STATEMENT . This saves 30-40 gas

INSTANCES (1):

APPRIMATE GAS SAVED : 40 GAS

SOLUTION:

 require(a <= b); x = b - a => require(a <= b); unchecked { x = b - a } if(a <= b); x = b - a => if(a <= b); unchecked { x = b - a }

This will stop the check for overflow and underflow so it will save gas.

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol

200: require(rethBalance2 > rethBalance1, "No rETH was minted");
201: uint256 rethMinted = rethBalance2 - rethBalance1;

Reth.sol#L200-L201

[G-7] Public function not called by contract declare external to save gas

INSTANCES (2):

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol

211: function ethPerDerivative(uint256 _amount) public view returns (uint256) {
221: function balance() public view returns (uint256) {

Reth.sol#L211,Reth.sol#L221

[G-8] With assembly, .call (bool success) transfer can be done gas-optimized

INSTANCES (3):

return data (bool success,) has to be stored due to EVM architecture, but in a usage like below, ‘out’ and ‘outsize’ values are given (0,0), this storage disappears and gas optimization is provided

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol


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

SafEth.sol#L124-L126

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol

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

WstEth.sol#L76,WstEth.sol#L63-L65

Recommended Mitigation:

bool success;                                 
+ assembly {                                    
+        success := call(gas(), dest, amount, 0, 0)
+          }

[G-9] Empty blocks should be removed or emit something

INSTANCES (4):

Empty receive()/fallback() payable functions that are not used, can be removed to save deployment gas

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol

97:  receive() external payable {}

WstEth.sol#L97

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/SfrxEth.sol

126: receive() external payable {}

SfrxEth.sol#L126

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

246: receive() external payable {}

SafEth.sol#L246

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol

244: receive() external payable {}

[G-10] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

INSTANCES (2):

When using elements that are smaller than 32 bytes, your contracts gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol

86:  uint24 _poolFee,
240:  (uint160 sqrtPriceX96, , , , , , ) = pool.slot0();

Reth.sol#L86.Reth.sol#L240

[G-11] Private functions only called once can be inlined to save gas

INSTANCES (1):

APPRIMATE GAS SAVED : 40 GAS

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol


function swapExactInputSingleHop(
        address _tokenIn,
        address _tokenOut,
        uint24 _poolFee,
        uint256 _amountIn,
        uint256 _minOut
    ) private returns (uint256 amountOut) {

Reth.sol#L83-L89

[G-12] Make 3 event parameters indexed when possible

INSTANCES (5):

It’s the most gas efficient to make up to 3 event parameters indexed. If there are less than 3 parameters, you need to make all parameters indexed

use 3 event parameters indexed
If less than 3 all event parameters should be indexed

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

    event SetMaxSlippage(uint256 indexed index, uint256 slippage);
    event Staked(address indexed recipient, uint ethIn, uint safEthOut);
    event Unstaked(address indexed recipient, uint ethOut, uint safEthIn);
    event WeightChange(uint indexed index, uint weight);
    event DerivativeAdded(
        address indexed contractAddress,
        uint weight,
        uint index
    );

[G-13] Save gas with address(0) checks before transfer ownership

INSTANCES (4):

The owner address value is assigned in the constructor, there is no address(0) value control. This means that transfer ownership to ADDRESS(0), The contract must be deployed again. This possibility mean gas consumption.

ADDRESS(0) control is the most error-prone value control . There is the possibility that ownership set to address(0). In this case contract must be redeployed again. This costs the large volume of the gas

Adding a address(0) control does not cause high gas consumption.

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol

function initialize(address _owner) external initializer {
        _transferOwnership(_owner);
        maxSlippage = (1 * 10 ** 16); // 1%
}

WstEth.sol#L33-L36

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/SfrxEth.sol

function initialize(address _owner) external initializer {
        _transferOwnership(_owner);
        maxSlippage = (1 * 10 ** 16); // 1%
}

SfrxEth.sol#L36-L39

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol

function initialize(address _owner) external initializer {
        _transferOwnership(_owner);
        maxSlippage = (1 * 10 ** 16); // 1%
}

Reth.sol#L42-L45

Recommendation:

It is recommended to perform a ADDRESS(0) check for critical OWNER assignments

[G-14] Gas overflow during iteration (DoS)

INSTANCES (4):

Each iteration of the cycle requires a gas flow. A moment may come when more gas is required than it is allocated to record one block. In this case, all iterations of the loop will fail

[G-15] Sort Solidity operations using short-circuit mode

INSTANCES (1):

Short-circuiting is a solidity contract development model that uses OR/AND logic to sequence different cost operations. It puts low gas cost operations in the front and high gas cost operations in the back, so that if the front is low, if the cost operation is feasible, you can skip (short-circuit) the subsequent high-cost Ethereum virtual machine operation

//f(x) is a low gas cost operation 
//g(y) is a high gas cost operation 
//Sort operations with different gas costs as follows 
f(x) || g(y) 
f(x) && g(y)

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

Use ethAmountToRebalance == 0 first then weights[i] == 0 . Because weights[i] is a state variable this consume more gas than ethAmountToRebalance == 0.


148:  if (weights[i] == 0 || ethAmountToRebalance == 0) continue;

[G-16] Save gas with the use of the specific import statements

INSTANCES (34):

With the import statement, it saves gas to specifically import only the parts of the contracts, not the complete ones.

Description:

Solidity code is also cleaner in another way that might not be noticeable: the struct Point. We were importing it previously with global import but not using it. The Point struct polluted the source code with an unnecessary object we were not using because we did not need it.

This was breaking the rule of modularity and modular programming: only import what you need Specific imports with curly braces allow us to apply this rule better.

Recommendation:

import {contract1 , contract2} from "filename.sol";

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/WstEth.sol

import "../../interfaces/IDerivative.sol";
import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "../../interfaces/curve/IStEthEthPool.sol";
import "../../interfaces/lido/IWStETH.sol";

WstEth.sol#L4-L8

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/SfrxEth.sol

import "../../interfaces/IDerivative.sol";
import "../../interfaces/frax/IsFrxEth.sol";
import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "../../interfaces/curve/IFrxEthEthPool.sol";
import "../../interfaces/frax/IFrxETHMinter.sol";

SfrxEth.sol#L4-L9

FILE : 2023-03-asymmetry/contracts/SafEth/SafEth.sol

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "../interfaces/IWETH.sol";
import "../interfaces/uniswap/ISwapRouter.sol";
import "../interfaces/lido/IWStETH.sol";
import "../interfaces/lido/IstETH.sol";
import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import "./SafEthStorage.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol";

SafEth.sol#L4-L11

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol

import "../../interfaces/IDerivative.sol";
import "../../interfaces/frax/IsFrxEth.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "../../interfaces/rocketpool/RocketStorageInterface.sol";
import "../../interfaces/rocketpool/RocketTokenRETHInterface.sol";
import "../../interfaces/rocketpool/RocketDepositPoolInterface.sol";
import "../../interfaces/rocketpool/RocketDAOProtocolSettingsDepositInterface.sol";
import "../../interfaces/IWETH.sol";
import "../../interfaces/uniswap/ISwapRouter.sol";
import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import "../../interfaces/uniswap/IUniswapV3Factory.sol";
import "../../interfaces/uniswap/IUniswapV3Pool.sol";

Reth.sol#L4-L15

[G-17] Instead of calculating the value with keccak256() every time the contract is made pre calculate them before and only give the result to a constant

INSTANCES (3):

FILE : 2023-03-asymmetry/contracts/SafEth/derivatives/Reth.sol

               keccak256(
                    abi.encodePacked("contract.address", "rocketTokenRETH")
                )

Reth.sol#L69-L71

                   keccak256(
                        abi.encodePacked("contract.address", "rocketTokenRETH")
                    )

Reth.sol#L190-L192

                keccak256(
                    abi.encodePacked("contract.address", "rocketTokenRETH")
                )

Reth.sol#L232-L234

#0 - c4-sponsor
2023-04-07T22:27:56Z

elmutt marked the issue as sponsor confirmed

#1 - c4-judge
2023-04-23T19:30:35Z

Picodes marked the issue as grade-a

Asymmetry contest - Sathish9098's results

A protocol to help diversify and decentralize liquid staking derivatives.

General Information

Findings Distribution

Researcher Performance

External Links

[Q-142] QA Report - $42.06

Findings Information

Awards

Labels

External Links

LOW FINDINGS

LOW

NON CRITICAL

[L-1] UNUSED RECEIVE() functions

[L-2] MISSING CHECKS FOR ADDRESS(0X0) WHEN TRANSFER OWNERSHIP

Recommendations

[L-3] Sanity/Threshold/Limit Checks

Recommendations:

[L-4] LOSS OF PRECISION DUE TO ROUNDING

[L-5] A single point of failure

Recommended Mitigation Steps

[L-6] Use safe variant of _mint() function

Recommendation

[L-7] No Storage Gap for SafEth contract

Recommended Mitigation Steps

[L-8] Prevent division by 0

Recommendations:

[L-9] Consider using OpenZeppelin’s SafeCast library to prevent unexpected behavior when casting uint

Recommendations:

[L-10] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Recommendations:

[L-11] Missing Event for critical parameters init and change

Description

Recommendation

[L-12] Gas griefing/theft is possible on unsafe external call

recommanded Mitigations:

[L-13] Front running attacks by the onlyOwner

Recommended Mitigation:

[L-14] Function Calls in Loop Could Lead to Denial of Service

[L-15] Use safeTransferOwnership instead of transferOwnership function

Description:

Recommendation:

NON CRITICAL FINDINGS

[NC-1] For modern and more readable code; update import usages

Recommendation

[NC-2] Imports can be grouped together

[NC-3] Hardcode the address causes no future updates

Recommendations:

[NC-4] Shorter the inheritance

[NC-5] EMPTY BLOCKS SHOULD BE REMOVED OR EMIT SOMETHING

[NC-6] ADD PARAMETER TO EVENT-EMIT

[NC-7] Pragma float

Recommendation

[NC-8] Interchangeable usage of uint and uint256

[NC-9] TYPOS

[NC-10] Include (@param and @return) parameters in NatSpec comments

Recommendation

[NC-11] NatSpec comments should be increased in contracts

Context

Description:

Recommendation

[NC-12] Function writing that does not comply with the Solidity Style Guide

Context

[N-13] Add a timelock to critical functions

[NC-14] For functions, follow Solidity standard naming conventions (private function style rule)

Description

[NC-15] Missing NatSpec

[NC-16] Use scientific notation instead of exponential notation

[NC-17] String constants used in multiple places should be defined as constants

[NC-18] Events that mark critical parameter changes should contain both the old and the new value

[N-19] NO SAME VALUE INPUT CONTROL

Recommended Mitigation Steps:

[NC-20] NOT USING THE NAMED RETURN VARIABLES ANYWHERE IN THE FUNCTION IS CONFUSING

[NC-21] Use a more recent version of Solidity

[NC-22] Use latest and same versions of openzeppelin contracts

[NC-23] Implement some type of version counter that will be incremented automatically for contract upgrades

Recommended Mitigation Steps

[NC-24] Generate perfect code headers every time

#0 - c4-sponsor2023-04-07T22:18:36Z

#0 - c4-sponsor
2023-04-07T22:18:36Z

#1 - c4-judge
2023-04-24T19:06:58Z

#0 - c4-sponsor
2023-04-07T22:27:56Z

#1 - c4-judge
2023-04-23T19:30:35Z