Asymmetry contest - KrisApostolov's results

QA report

Disclaimer

Some of the examples may contain truncated versions of the original code. There may also be @audit tags, which denote the actual place affected

Table of contents

Low severity findings

[L-01] - Lack of balance check allows the unstake function to be called without a corresponding balance or amount

The unstake function in the SafEth contract does not implement any balance checks whatsoever, which enables anyone to call the function with a _safEthAmount = 0 without any reverts. The function will execute everything like normal thus wasting gas and opening the contract to other potential vulnerabilities.

Instances

File ./contracts/SafEth/SafEth.sol:

[1] - unstake()

Recommendations

Consider adding 2 types of checks in the unstake function:

1. A require(_safEthAmount ≠ 0, “error message”); statement or similar.
2. A require(balanceOf(msg.sender) ≥ _safEthAmount, “error message”) statement or similar.

[L-02] - Not following the CEI pattern can lead to a reentrancy

The unstake function in the SafEth contract does not follow the checks-effects-interactions pattern and goes straight to calling an external contract (a derivative) to withdraw amount the user wants before burning the user’s receipt tokens. This creates an opportunity in which someone can find a way to re-enter into the function before their tokens get burned, which could lead to fund theft.

Instances

File **./contracts/SafEth/SafEth.sol:**

[1] - unstake()

Recommendations

Consider burning the receipt tokens of the user before actually withdrawing any eth from the derivatives, thus minimizing the risk of reentrancy.

[L-03] - Slight precision loss occurring in the calculations

Instances

File ./contracts/SafEth/derivatives/Reth.sol 1 instance:

**[1] -** ((((rethPerEth * msg.value) / 10 ** 18) * ((10 ** 18 - maxSlippage))) / 10 ** 18)

The Uniswap minimum amount calculation in the deposit() of the Rocket Eth derivative produces a 1-2 WEI precision loss in half of the times. It happens because division is done before multiplication, which causes loss in precision. This effectively locks that small amount of eth each time a deposit in the derivative has been made.

Recommendations

Consider re-factoring the equation to the following: (((rethPerEth * msg.value * (1e18 - maxSlippage)) / (1e36))).

File ./contracts/SafEth/derivatives/SfrxEth.sol 1 instance:

[1] -

The Curve pool minimum amount calculation in the withdraw() of the Sfrx Eth derivative produces a 1-2 WEI precision loss in half of the times. It happens because division is done before multiplication, which causes loss in precision. This effectively locks that small amount of eth each time a deposit in the derivative has been made.

Recommendations

Consider re-factoring the equation to the following: **(_amount * ethPerDerivative(_amount) * (1e18 - maxSlippage)) / (1e36)**

Non-Critical findings

[N-01] - Insufficient coverage

The derivative contracts that have been implemented do not have 100% code coverage.

Having 100% code coverage is a best practice in terms of security.

Recommendations

Consider testing all of the functions of the derivative contracts until 100%.

[N-02] - Use scientific notation instead of x * 10 ** y

Instances

Add all of the expressions where ** is used.

Recommendations

Instead of using the x * **10 ** y** syntax use the scientific notation syntax: **xey**

Example:

**x = 1**, y = 18.

Before: 1 * 10 ** 18 After: 1e18.

[N-03] - Use specific imports instead of whole file imports for cleanliness

In order to keep the codebase cleaner and not know where each thing comes from all imports should be explicit.

Instances

The whole protocol.

Recommendations

Consider using the following syntax for imports:

import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

[N-04] - Omit the parameter name of an unused param in the specific scenario

The ethPerDerivative() functions in the WstEth and SfrxEth contracts contain an unused parameter that is there because of the way the function of each derivative gets called in the stake function of the SafEth contract.

Instances

File ./contracts/SafEth/derivatives/SfrxEth.sol 1 instance:

[1]

File ./contracts/SafEth/derivatives/WstEth.sol 1 instance:

[2]

Recommendations

Consider removing the _amount name of the parameter and leaving out only the uint256 type so the compiler does not revert when the function gets called with a parameter.

[N-05] - Floating pragma

When intended for deployment a contract should always use a locked solidity version in order to make sure that the code is optimized and tested for its solidity version. Check this for more details.

Instances

File ./contracts/SafEth/SafEth.sol

File ./contracts/SafEth/derivatives/Reth.sol

File ./contracts/SafEth/derivatives/SfrxEth.sol

File ./contracts/SafEth/derivatives/WstEth.sol

Recommendations

Consider changing the compiler version of all of the files from pragma solidity ^0.8.13 to pragma solidity 0.8.13.

[N-06] - Create getter functions for rocketDepositPool and rocketDAOProtocolSettingsDeposit

The following blocks just pollute the business logic of the Rocket Eth derivative and make it hard to understand what is happening in the poolCanDeposit() function.

Instances

File ./contracts/SafEth/derivatives/Reth.sol:

[1] - rocketDepositPoolAddress

**[2] -** rocketProtocolSettingsAddress

Recommendations

Consider isolating them in the same manner as the rethAddress() function.

[N-07] - Use the rethAddress() function instead of re-implementing the same code in another function

The code block referenced below contains exactly the same code as the rethAddress() function. There is no point it doing so instead of calling the function directly below.

Instance

File ./contracts/SafEth/derivatives/Reth.sol:

[1]

Recommended

Consider replacing the block with the invocation of the rethAddress() function.

[N-08] - Unnecessary else block

Unnecessary else block that does no function

Instance

File ./contracts/SafEth/derivatives/Reth.sol:

[1]

Recommendations

Consider removing the else block and unnesting the code

[N-09] - Remove unnecessary calculation

The following calculation in the Rocket Eth derivative contract is excessive: **(poolPrice() * 10 ** 18) / (10 ** 18)**

Instance

File ./contracts/SafEth/derivatives/Reth.sol:

[1] - (poolPrice() * 10 ** 18) / (10 ** 18)

Recommendation

Simplify the equation to only poolPrice()

Gas Report

Disclaimer

Some of the examples may contain truncated versions of the original code. There may also be @audit tags, which denote the actual place affected

Table of contents

Estimated savings: ^14500 gas

[G-01] - Functions guaranteed to revert, when called by normal users should be marked private to save on gas

In the event that a function modifier or a "require" statement like onlyOwner is utilized, an attempt by a regular user to make a call to the function will result in the function being reverted. If the function is marked as "payable," the cost of gas for legitimate callers will decrease, as the compiler will no longer need to include checks to determine if payment was provided. This means that the additional opcodes that are typically avoided, including CALLVALUE(2), DUP1(3), ISZERO(3), PUSH2(3), JUMPI(10), PUSH1(3), DUP1(3), REVERT(0), JUMPDEST(1), and POP(2), which typically cost an average of around 21 gas per call to the function, will not be included. The following instances even include the proxy initializers as they are not an exception to the gas saving.

Instances:

File ./contracts/SafEth/SafEth.sol - 9 instances:

[1] - initialize()

[2] - rebalanceToWeights()

**[3] - adjustWeight()**

[4] - addDerivative()

[5] - setMaxSlippage()

[6] - setMinAmount()

[7] - setMaxAmount()

[8] - setPauseStaking()

[9] - setPauseUnstaking()

File ./contracts/SafEth/derivatives/Reth.sol - 4 instances:

[1] - initialize()

[2] - setMaxSlippage()

[3] - withdraw()

[4] - deposit()

File ./contracts/SafEth/derivatives/SfrxEth.sol - 4 instances:

[1] - initialize()

[2] - setMaxSlippage()

[3] - withdraw()

[4] - deposit()

File ./contracts/SafEth/derivatives/WstEth.sol - 4 instances:

[1] - initialize()

[2] - setMaxSlippage()

[3] - withdraw()

[4] - deposit()

Recommendations

Consider adding the payable modifier to the mentioned functions.

This will decrease the gas usage of each function by 21.

[G-02] - Use unchecked blocks on variables that are guaranteed to not overflow or underflow

In the current situation, the projected amount of derivatives that the protocol will support is around 5 derivatives as per Toshi. This means that it is highly unlikely for the derivativeCount variable to overflow.

Instances:

File ./contracts/SafEth/SafEth.sol - 1 instance:

[1] - derivativeCount++;

Recommendations

Consider doing the following:

derivativeCount++; to unchecked { ++derivativeCount; }

Decreases gas usage by ~126.

[G-03] - Unnecessary external call(s)

Certain functions call external contracts without it being needed.

Instances:

File ./contracts/SafEth/derivatives/SfrxEth.sol - 2 instances:

[1] -

SfrxEth.sol
function withdraw(uint256 _amount) external onlyOwner {
// The redeem function returns the withdrawn amount, which is equal to the frxEthBalance variable
        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
        );

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

        IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).exchange(
            1,
            0,
            frxEthBalance,
            minOut
        );
        // solhint-disable-next-line
        (bool sent, ) = address(msg.sender).call{value: address(this).balance}(
            ""
        );
        require(sent, "Failed to send Ether");
    }

The redeem function returns the withdrawn amount.

Recommendations

Consider removing lines 66-68 altogether and assigning the uint256 frxEthBalance variable to the redeem function.

This can save upwards of 1000 gas.

[2] -

Sfrx.sol
function deposit() external payable onlyOwner returns (uint256) {
        IFrxETHMinter frxETHMinterContract = IFrxETHMinter(
            FRX_ETH_MINTER_ADDRESS
        );
        uint256 sfrxBalancePre = IERC20(SFRX_ETH_ADDRESS).balanceOf(
            address(this)
        );
        frxETHMinterContract.submitAndDeposit{value: msg.value}(address(this));
        uint256 sfrxBalancePost = IERC20(SFRX_ETH_ADDRESS).balanceOf(
            address(this)
        );
        return sfrxBalancePost - sfrxBalancePre;
    }

Line 101 returns sfrxBalancePost - sfrxBalancePre.

Recommendations

Consider removing the lines 98-100 and 102-104 entirely and directly return the result of line 101.

This can save upwards of 2000 gas and can mitigate the risks that external calls bring.

[G-04] - Unnecessary variable allocation

Instances:

File ./contracts/SafEth/derivatives/Reth.sol - 2 instances:

[1] - deposit()

Recommendations

****Instead of assigning the uint256 amountSwapped variable return the swapExactInputSingleHop(W_ETH_ADDRESS, rethAddress(), 500, msg.value, minOut); expression directly.

[2] - deposit()

Recommendations

Instead of assigning rethMinted variable simply return rethBalance2 - rethBalance1.

File ./contracts/SafEth/derivatives/WstEth.sol - 1 instance:

[1] - deposit()

Recommendations

Instead of declaring both the wstEthAmount and stEthBalancePost variables ****directly return the following return IWStETH(WST_ETH).balanceOf(address(this)) - wstEthBalancePre; .

All occurrences save 13 gas per variable.

[G-05] - Hashes can be cached into into immutable variables

Instances:

File ./contracts/SafEth/derivatives/Reth.sol - 6 instances:

**[1] [2] [3] -** keccak256(abi.encodePacked("contract.address", "rocketTokenRETH")) hash.

**[4] [5] -** abi.encodePacked("contract.address", "rocketDepositPool") hash.

[6] - keccak256(abi.encodePacked("contract.address", "rocketDAOProtocolSettingsDeposit")) hash.

Recommendations

Isolate the following instances into immutable storage variables. Assign the hashes’ values to the variables in the initialize function. This way it caches the output of the operation and only gas for calling a storage variable is added.

Saves 462 gas per instance.

[G-06] - Refactor the weight calculation

The following expression:

for (uint256 i = 0; i < derivativeCount; i++) localTotalWeight += weights[i];

is reading i times from storage. Which can get pretty expensive over time if multiple derivatives get added and removed.

Instances:

File ./contracts/SafEth/SafEth.sol - 1 instance:

[1] - adjustWeight()

Recommendations

Consider re-writing the function in the following way:

SafEth.sol
function adjustWeight(uint256 _derivativeIndex, uint256 _weight) external onlyOwner {
    uint256 oldWeight = weights[_derivativeIndex]
		weights[_derivativeIndex] = _weight;
    totalWeight = totalWeight + _weight - oldWeight;
    emit WeightChange(_derivativeIndex, _weight);
  }

This way the storage reads get minimized to 2 and the gas cost stays constant.

This saves i * (100 + 3) - 2 * 100 gas per function call.

In a case where there are 5 derivatives this can save up to 327 gas

[G-07] - Saving storage variable into memory saves gas when variable is called more than once

When referencing storage variables more than once it is best to cache them in a stack variable simply because an **sload** operation costs 100 gas, when a mload costs 3.

Instances

File ./contracts/SafEth/SafEth.sol - 11 instances:

[1] [2] - stake()

**[3] - unstake()**

[4] [5] - rebalanceToWeights()

**[6] - adjustWeight()**

**[7] [8] [9] [10] [11] - addDerivative()**

Recommendations

Cache the storage variables that are being referenced more than once to save an additional 97 gas per additional call.

[G-08] - Saving the output of a function when possible to save on gas

This minimizes the number of external calls the functions make.

Instances

File ./contracts/SafEth/SafEth.sol - 2 instances:

[1] [2]

Recommendations

Cache the derivatives[i].balance()'s value in a stack variable so it does not get called twice.

This can save upwards of 1000 gas or even more per iteration depending on the derivative’s mechanisms. Typically it will save around 4-5k gas on function call depending on the amount of derivatives.

[G-09] - Optimize names to save gas

It is possible to optimize the public/external function names and public member variable names to reduce gas consumption. To see an example of how this optimization works, please see the example. The interfaces/abstract contracts listed below can be optimized so that the functions that are called frequently consume the least amount of gas during method lookup. By prefixing the method IDs with two zero bytes, up to 128 gas can be saved during deployment, and renaming functions to have lower method IDs can save up to 22 gas per call.

To learn more check the following article.

[G-10] Emitting an event with a storage variable wastes gas

Emitting an event with a storage variable wastes 130 gas.

Instances

File ./contracts/SafEth/SafEth.sol - 5 instances:

[1] - emit DerivativeAdded(_contractAddress, _weight, derivativeCount);

[2] - emit ChangeMinAmount(minAmount);

[3] - emit ChangeMaxAmount(maxAmount);

[4] - emit StakingPaused(pauseStaking);

[5] - **emit UnstakingPaused(pauseUnstaking);**

Recommendations

Consider swapping the storage variables out for the stack variables already present in the functions, as most of them already set the storage variable to the stack variable.

[G-11] address.call can be optimized with assembly

The standard (bool success, bytes memory data) = address.call(””) syntax copies the return data even if we omit the data.

Instances

File ./contracts/SafEth/SafEth.sol - 1 instance:

**[1]**

File ./contracts/SafEth/derivatives/Reth.sol - 1 instance:

[2]

File ./contracts/SafEth/derivatives/WstEth.sol - 2 instances:

[3]

[4]

File [./contracts/SafEth/derivatives/SfrxEth.sol](https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol) - 1 instance:

[5]

Recommendations

Using the following instead of the normal call will save on gas:

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