Asymmetry contest - 0xhacksmithh's results

[Low-01] User may lost his ETH due to precision loss

Inside stake() User deposited ETH sent to different derivative contracts according to their weights. Now the amount of eth will sent to those each derivative contract is decided by following

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; 

            ......
            ......

Here calculated ethAmount may suffers from precision loss and user will get less safETH than he actually deserve.

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

[Low-02] Old Solidity Version Used

Instances(4)

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

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

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

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

[Low-03] Single point of failure

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

owner role in the project:

Owner is not behind a multisig and changes are not behind a timelock.

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
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol

[NC-01] Use Ownable2StepUpgradeable instead of OwnableUpgradeable contract

Instances(1)

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

[NC-02] Owner can Renounce Ownership.

Typically, the contract’s owner is the account that deploys the contract. As a result, the owner is able to perform certain privileged activities.

The OpenZeppelin’s Ownable used in this project contract implements renounceOwnership. This can represent a certain risk if the ownership is renounced for any other reason than by design.

Renouncing ownership will leave the contract without an owner, thereby removing any functionality that is only available to the owner.

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

[NC-03] Floating Pragma Used

Instances(4)

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

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

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

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

[NC-04] Absence of both Old and New parameter in event

event only emited with newly set value, not with old value. There should be both present, newly set one and previous old value as well. Instances(5)

File: contracts/SafEth/SafEth.sol
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L21
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L22
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L25
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L28
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L174

[NC-05] Use specific notation (e.g 1e18) rather than exponentiation (e.g 10 ** 18)

Instances(3)

File: contracts/SafEth/SafEth.sol
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L54
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L55
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L98

[NC-06] Absence of sanity checks

Like max and low uint limit while setting uint256 like slipage minAmount maxAmount

Is it previously true or false or current value is equal with previous one for bools pauseStaking pauseUnstaking

Instances(5)

File: 
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L206
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L215
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L224
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L233
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L242

[NC-07] For modern and more readable code, update import usages.

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.

[NC-08] Upgradeable contract is missing a __gap[50] storage variable

__gap, is empty reserved space in storage that is put in place in Upgradeable contracts. It allows us to freely add new state variables in the future without compromising the storage compatibility with existing deployments.

[NC-09] Insufficient tests coverage

The test coverage rate of the project is 98%. Testing all functions is best practice in terms of security criteria.

[Gas-01] Function result should be cached in memory instead of calling it again and again.

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

Here function result of derivatives[i].balance() should be cached in memory instead of calling it again. Instances(2)

File: contracts/SafEth/SafEth.sol
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L72-L75
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L141-L142

[Gas-02] Mathematic logics for adjustWeight() could be improved to save gas.

 function adjustWeight(
        uint256 _derivativeIndex, 
        uint256 _weight
    ) external onlyOwner {  
        weights[_derivativeIndex] = _weight;
        uint256 localTotalWeight = 0;
        for (uint256 i = 0; i < derivativeCount; i++) 
            localTotalWeight += weights[i]; 
        totalWeight = localTotalWeight;
        emit WeightChange(_derivativeIndex, _weight); 
    }

Here first weight set in mapping, then pointer(program pointer) itterate over whole mapping and add their corresponding weights and finally set result(localTotalWeight) to totalWeight state variable.

Here multiple stateVariable read and addition occure to set just one derivative's weight

Which can preform with less cost i.e

1. Whenever we change a weight a. first delete that much weight from totalWeight b. Set corresponding weight with its index in weights mapping. b. then add new weight to totalWeight

 function adjustWeight(
        uint256 _derivativeIndex, 
        uint256 _weight
    ) external onlyOwner {  
+        totalWeight = totalWeight - weights[_derivativeIndex];
         weights[_derivativeIndex] = _weight;
+        totalWeight = totalWeight + _weight;

-        uint256 localTotalWeight = 0;
-       for (uint256 i = 0; i < derivativeCount; i++) 
-            localTotalWeight += weights[i]; 
-       totalWeight = localTotalWeight;
-       emit WeightChange(_derivativeIndex, _weight); 
    }

Instances(1)

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

[Gas-03] > 0 could replace with != 0, which is more gas efficient.

Instances(1)

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

[Gas-04] Absence of sanity checks for _derivativeIndex in adjustWeight()

function adjustWeight(
        uint256 _derivativeIndex, 
        uint256 _weight
    ) external onlyOwner {  
        weights[_derivativeIndex] = _weight;
        uint256 localTotalWeight = 0;
        for (uint256 i = 0; i < derivativeCount; i++)

        ......
        ......

There should be a check that ensure value pair forweights[_derivativeIndex] mapping exists, before making further calls.

Instances(1)

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

[Gas-05] Functions Guaranteed To Revert When Called By Nomal Users Can Be Marked As payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost. Instances(8)

File: contracts/SafEth/SafEth.sol
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L138
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L168
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L185
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L205
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L214
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L223
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L232
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L241

[Gas-06] Arithmatic operations should uncheck

Instances(4)

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

[Gas-07] Setting constructor to payable will save gas

Saves ~13 gas per interaction Instances(4)

[Gas-08] Save gas with use of the import statement

Instances(Multiple Instances)