Asymmetry contest - Aymen0909's results

QA Report

Summary

Findings

1- Same derivative can be added multiple times :

Risk : Low

The addDerivative function does not check if the new derivative exists or not so the same derivative can be added multiple times (by accident), the impact of this error is that this derivative will have a bigger weight than it was intended to because the derivativeCount is incremented when adding a duplicate derivative thus when looping over all the derivatives for calculating the total weight the weight of this derivative is counted multiple times.

It's important to note that there is no way to remove duplicate derivative because if its weight is set to 0 this will also remove the original derivative (as his weight will also be set to 0).

As this issue can potential cause problems for the protocol working it should be addressed by adding a way to chack for the existance of derivative contracts.

Mitigation

Add a checks in the addDerivative function to verify the existance of the derivative.

2- maxSlippage should have a maximum upper bound :

Risk : Low

The value of maxSlippage doesn't have a known constant maximum value so the owner can set it to any value between 0 and 1e18 (can't be higher due underflow errors), so when a user stakes his funds he doesn't really know what would be the max slippage when he intend to withdraw for example when a user stakes the max slippage was set to 1% which the user accept but at the moment he wants to unstake the max slippage value is now 8% which is too high for him.

To avoid any confusion the maxSlippage should have a constant upper bound value set in each derivative contract, so the users always know in advance the maximum slippage value they are risking.

Mitigation

You should considere adding a constant upper bound value for maxSlippage in each derivative contract.

3- addDerivative should contain non zero address check :

Risk : Low

The addDerivative function is used to add new derivative contract to the staking process, this function does not contain a check on the input derivative contract address _contractAddress which means it can be set to address(0) by accident and this will still sets the derivative weight and increment the derivativeCount and totalWeight, this can have a big negative impact on the protocol as when users will stakes their funds part of them will go to address(0) (get burned) and can't be withdrawn afterwards leading to a loss of funds.

The risk of this issue happening is very low but due to its bad impact it should be addressed.

Mitigation

Add non-zero address checks in the addDerivative function for the value of _contractAddress.

4- Avoid floating pragma where possible :

Risk : Non critical

All the contracts of the protocol use a floating solidity version pragma solidity ^0.8.13 which should be fixed to agiven version (preferably to a recent one) to avoid any issues in the future, as 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.

5- Use scientific notation :

When using multiples of 10 you shouldn't use decimal literals or exponentiation (e.g. 1000000, 10**18) but use scientific notation for better readability.

Risk : NON CRITICAL

Proof of Concept

Instances include:

10 ** 16 :

Reth.sol : Line 44

SfrxEth.sol : Line 38

WstEth.sol : Line 35

10 ** 17 :

SafEth.sol : Line 54

10 ** 18 :

SafEth.sol : Line 55, Line 75, Line 80, Line 81, Line 94, Line 98

Reth.sol : Line 173-174, Line 214-215

SfrxEth.sol : Line 74-75, Line 113-115

WstEth.sol : Line 60, Line 87

10 ** 36 :

Reth.sol : Line 171

Mitigation

Use scientific notation for the instances aforementioned.

Gas Optimizations

Summary

Findings

1- Multiple address/IDs mappings can be combined into a single mapping of an address/id to a struct :

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.

There are 2 instances of this issue :

File: SafEth.sol Line 22-23

mapping(uint256 => IDerivative) public derivatives; // derivatives in the system
mapping(uint256 => uint256) public weights; // weights for each derivative

These mappings could be refactored into the following struct and mapping for example :

struct Derivative {
    uint256 weight;
    IDerivative derivative;
}
    
mapping(uint256 => Derivative) public derivatives;

2- storage variable should be cached into memory variables instead of re-reading them :

The instances below point to the second+ access of a state variable within a function, the caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read, thus saves 100gas for each instance.

There are 5 instances of this issue :

File: SafEth.sol Line 71-84

In the code linked above the value of derivativeCount is read multiple times (2) from storage and its value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

File: SafEth.sol Line 140-147

In the code linked above the value of derivativeCount is read multiple times (2) from storage and its value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

File: SafEth.sol Line 148-149

In the code linked above the value of weights[i] is read multiple times (2) from storage and its value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

File: SafEth.sol Line 186-187

In the code linked above the value of derivativeCount is read multiple times (2) from storage and its value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

File: SafEth.sol Line 191-194

In the code linked above the value of derivativeCount is read multiple times (2) from storage and its value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

3- Use unchecked blocks to save gas :

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block.

There is 1 instance of this issue:

File: SafEth.sol Line 188

derivativeCount++;

The increment of derivativeCount cannot realisticaly overflow as they represent the numbers of derivative contracts, so the operation should be marked as unchecked to save gas.

4- derivatives[i].balance() should be cached into memory :

derivatives[i].balance() is a call to an external contract which cost a lot of gas, so when the actual derivative contract balance is not changing derivatives[i].balance() should only be called once and its value should be cached into a memory variable to save gas instead of making this call multiple times.

There are 2 instances of this issue :

File: SafEth.sol Line 73-74

underlyingValue +=
    (derivatives[i].ethPerDerivative(derivatives[i].balance()) *
        derivatives[i].balance()) /
    10 ** 18;

File: SafEth.sol Line 141-142

if (derivatives[i].balance() > 0)
    derivatives[i].withdraw(derivatives[i].balance());

5- derivativeCount should not be read at each loop iteration :

The value of derivativeCount is read directly from storage at each iteration of the for loops which will cost a lot of gas, its value should be cached into memory variable instead.

There are 7 instances of this issue :

File: SafEth.sol

71      for (uint i = 0; i < derivativeCount; i++)
84      for (uint i = 0; i < derivativeCount; i++)
113     for (uint256 i = 0; i < derivativeCount; i++)
140     for (uint i = 0; i < derivativeCount; i++)
147     for (uint i = 0; i < derivativeCount; i++)
171     for (uint256 i = 0; i < derivativeCount; i++)
191     for (uint256 i = 0; i < derivativeCount; i++)

6- memory values should be emitted in events instead of storage ones :

The values emitted in events shouldn’t be read from storage but the existing memory values should be used instead, this will save ~100 GAS.

There are 4 instances of this issue :

File: SafEth.sol Line 216

emit ChangeMinAmount(minAmount);

File: SafEth.sol Line 225

emit ChangeMaxAmount(maxAmount);

File: SafEth.sol Line 234

emit StakingPaused(pauseStaking);

File: SafEth.sol Line 243

emit UnstakingPaused(pauseUnstaking);

7- public functions not called by the contract should be declared external instead :

The public functions that are not called inside the contract should be declared external instead to save gas.

There are 8 instances of this issue:

File: Reth.sol

50     function name() public pure returns (string memory)
211    function ethPerDerivative(uint256 _amount) public view returns (uint256)
221    function balance() public view returns (uint256)

File: SfrxEth.sol

44     function name() public pure returns (string memory)
122    function balance() public view returns (uint256)

File: WstEth.sol

41     function name() public pure returns (string memory)
86     function ethPerDerivative(uint256 _amount) public view returns (uint256)
93     function balance() public view returns (uint256)