Golom contest - 0xSmartContract's results

1-Maximum vote number problem

Context:

ERC20Votes.sol#L14 GolomToken.sol#L36-L38

Description: @openzeppelin/contracts/token/ERC20/extensions/ERC20Votes.sol contract supports token supply up to 2^224^ - 1 but GolomToken can mint 2^226^ - 1 . Naturally, it does not seem possible to reach this number, but it is mathematically possible, it can be a problem if the project team has a security vulnerability such as privatekey theft.

2-’0’ address Check

Context: 

RewardDistributor.sol#L74-L85) GolomToken.sol#L36-L38 GolomToken.sol#L28-L30

Description: Also check the length of the address to protect the code from short address problem just in case.This is best practice

Recommendation: use this ; require(_governance =! address(0x0));

3-Use Solidity Style Guide Format

Context: 

VoteEscrowDelegation.sol#L51 RewardDistributor.sol#L48 RewardDistributor.sol#L57 VoteEscrowCore.sol#L295-L298

Description - Recommendation: https://docs.soliditylang.org/en/v0.8.15/style-guide.html Must be written according to Style Guide Format rules as stated in Solidity documentation

• Constants should be named with all capital letters with underscores separating words.(Examples:  MAX_BLOCKS, TOKEN_NAME, TOKEN_TICKER, CONTRACT_VERSION)

4-Two Different variables have the same description

Context:  RewardDistributor.sol#L66-L67

Description For code readability, the explanations should be clear and distinct, the explanations in these two variables are the same, but they should have different functions

1-Setting The Constructor To Payable

Context:

GolomToken.sol#L28 ,

Description: 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 21💰 gas on deployment with no security risks.

Proof of Concept: https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5?u=pcaversaccio

Recommendation: Set the constructor to payable.

2-Remove : import 'hardhat/console.sol';

Context:

RewardDistributor.sol#L9 VoteEscrowDelegation.sol#L6

Description: It's used to print the values of variables while running tests to help debug and see what's happening inside your contracts But since it's a development tool, it serves no purpose on mainnet. This will save some gas💰

Recommendation: Use to import 'hardhat/console.sol'; only for tests, remove to mainnet deploy

3-There is no need to assign default values to variables

Context: 

VoteEscrowDelegation.sol#L51 RewardDistributor.sol#L257 RewardDistributor.sol#L222-L223 RewardDistributor.sol#L175-L176 RewardDistributor.sol#L156 RewardDistributor.sol#L142 RewardDistributor.sol#L45

Description: When a variable is declared solidity assigns the default value. In case the contract assigns the value again, it costs extra gas.

Recommendation: uint x = 0 costs more gas than uint x without having any different functionality.

4-Functions Visibility Can Be Declared External

Context: 

RewardDistributor.sol#L269 RewardDistributor.sol#L254 RewardDistributor.sol#L215 RewardDistributor.sol#L172 RewardDistributor.sol#L155 RewardDistributor.sol#L141

Description: Several functions across multiple contracts have a public visibility and can be marked with external visibility to save gas. This function is never called from within the contract

Recommendation: Change the functions visibility to external to save gas.

5-Catching The Array Length Prior To Loop

Context: 

VoteEscrowDelegation.sol#L200 VoteEscrowDelegation.sol#L190 VoteEscrowDelegation.sol#L172 RewardDistributor.sol#L157 RewardDistributor.sol#L183 RewardDistributor.sol#L180 RewardDistributor.sol#L143 GolomTrader.sol#L415

Description: One can save gas by caching the array length (in stack) and using that set variable in the loop. Replace state variable reads and writes within loops with local variable reads and writes. This is done by assigning state variable values to new local variables, reading and/or writing the local variables in a loop, then after the loop assigning any changed local variables to their equivalent state variables.

Recommendation: Simply do something like so before the for loop: uint length = variable.length Then add length in place of variable.length in the for loop.

6-Use ++index instead of index++ to increment a loop counter

Context: 

VoteEscrowDelegation.sol#L200 VoteEscrowDelegation.sol#L172 RewardDistributor.sol#L258 RewardDistributor.sol#L226 RewardDistributor.sol#L183 RewardDistributor.sol#L180 RewardDistributor.sol#L157 RewardDistributor.sol#L143 GolomTrader.sol#L415

Description: Due to reduced stack operations, using ++index saves 5💰 gas per iteration. Use ++index instead of index++ to increment a loop counter

Recommendation: Use ++index to increment a loop counter.

7- uint256 is Cheaper Than uint8

Context: VoteEscrowCore.sol#L320

Description: The EVM reads in 32 byte words if your data is smaller, further operations are needed to downscale from 256 bits to 8 bit. Since these uint8s are not packed with others to be read from the same slot it's cheaper to just use uint256 from them.

Recommendation: use uint256 instead of uint8

8- uint256 is Cheaper Than uint8 for ReentrancyGuard

Context: 

VoteEscrowCore.sol#L356-L358

Description:  The EVM reads in 32 byte words if your data is smaller, further operations are needed to downscale from 256 bits to 8 bit. Since these uint8s are not packed with others to be read from the same slot it's cheaper to just use uint256 from them.

Proof of Concept: 

ReentrancyGuard.sol#L34-L37 OpenZeppelin Reentrancy Guard use uint256 but VoteEscrowCore.sol use uint8

Recommendation:  use uint256 instead of uint8

9- Change ReentrancyGuard Pattern

Context:  VoteEscrowCore.sol#L356-L364

Description: Project Reentrancy Guard Pattern use 23888 gas , OpenZeppeling Reentrancy Guard use 23.537 gas It saves 351💰 gas in each operation, modifiers are designs with high usage, so gas saving is important

Proof of Concept: Project Reentrancy Code Pattern (23888 💰Gas);

pragma solidity ^0.8.10;
contract fooContract {
   uint8 internal constant _not_entered = 1;
    uint8 internal constant _entered = 2;
    uint8 internal _entered_state = 1;
  
    modifier nonreentrant() {
    require(_entered_state == _not_entered);
   _entered_state = _entered;
   _;
   _entered_state = _not_entered;
    }

 // 23888 Gas
   function foo() public nonreentrant {
   }
}

Openzeppelin Reentrancy Code Pattern (23537 💰Gas);

pragma solidity ^0.8.10;
contract fooContract {

    uint256 private constant _not_entered = 1;
    uint256 private constant _entered = 2;
    uint256 private _entered_state = 1;
  modifier nonreentrant() {
    require(_entered_state == _not_entered);
    _entered_state = _entered;
    _;
    _entered_state = _not_entered;
    }

  // 23537 Gas
    function foo() public nonreentrant {
    }
}

Recommendation: Use this OZ pattern ; ReentrancyGuard.sol#L34-L37

10- Function Ordering via Method ID

Context:  All Contracts

Description: Contracts most called functions could simply save gas by function ordering via Method ID. Calling a function at runtime will be cheaper if the function is positioned earlier in the order (has a relatively lower Method ID) because 22 gas are added to the cost of a function for every position that came before it. The caller can save on gas if you prioritize most called functions.

Recommendation: Find a lower method ID name for the most called functions for example Call() vs. Call1() is cheaper by 22 gas

Proof of Concept: https://coinsbench.com/advanced-gas-optimizations-tips-for-solidity-85c47f413dc5

11- use != 0 instead of > 0

Context: 

GolomTrader.sol#L152 GolomTrader.sol#L250 GolomTrader.sol#L387

Description:  When dealing with unsigned integer types, comparisons with != 0 are cheaper then with > 0 

Proof of Consept:  https://aws1.discourse-cdn.com/business6/uploads/zeppelin/original/2X/3/363a367d6d68851f27d2679d10706cd16d788b96.png

12- Using operator && used more gas

Context:

VoteEscrowCore.sol#L894 VoteEscrowCore.sol#L538

Description: Using double require instead of operator && can save more gas. (8 gas💰 cheaper)

Proof of Consept: Example:

// gas cost 21630 
function check(uint x)public view{ require(x == 0 && x < 1 ); } 


// using double require: 
// gas cost 21622
require(x == 0 ); 
require( x < 1); } } 

13- Reduce the size of error messages (Long revert Strings)

Context:

RewardDistributor.sol#L181 GolomToken.sol#L24

Description: require(msg.sender == minter, 'GolomToken: only reward distributor can enable’); ( require error string length 32>)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Recommendation: Revert strings > 32 bytes:

14- Do not calculate constants

Context:

VoteEscrowCore.sol#L296-L297 RewardDistributor.sol#L57 RewardDistributor.sol#L48

Description: Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas. If the developer wants to show the calculation in the Constant expression, he can annotate it with ‘//’

Proof Of Concept: RewardDistributor.sol#L48 : uint256 constant dailyEmission = 600000 * 10**18;

Recommendation: uint256 constant dailyEmission = 108000000; //(600000 * 10**18)

15- Switching between non-zero values saves gas

Context:

GolomTrader.sol#L158-L162

Description: SSTORE from 0 to 1 (or any non-zero value), the cost is 20000;SSTORE from 1 to 2 (or any other non-zero value), the cost is 5000. Therefore, switching between 1, 2 instead of 0, 1 will be more gas efficient.

Proof Of Concept: GolomTrader.sol#L158-L162 :

///      OrderStatus = 0 , if signature is invalid. (This is costly operator)
///      OrderStatus = 1 , if deadline has been
///      OrderStatus = 2 , order is filled or cancelled
///      OrderStatus = 3 , valid order

Recommendation:

///      OrderStatus = 1 , if signature is invalid
///      OrderStatus = 2 , if deadline has been
///      OrderStatus = 3 , order is filled or cancelled
///      OrderStatus = 4 , valid order

16- Unused Mapping State Variable

Context:

RewardDistributor.sol#L63

Description: This state variable is not used anywhere in Contracts:mapping(uint256 => uint256) public rewardLP;

Recommendation: Remove this line from contract

mapping(uint256 => uint256) public rewardLP; 

17- Define the data to be assigned value as Immutable in Constructor

Context: RewardDistributor.sol#L69 RewardDistributor.sol#L46

Description: Define the data to be assigned value as Immutable in Constructor Immutable state variables can be declared using the immutable keyword. They cannot be read during contract creation, but either have to be written to exactly once in the constructor or initialized directly in their declaration. Runtime code can only read immutable, but not write to them

Recommendation: Change to code like this for save gas ;

ERC20 public weth; ===> ERC20 public immutable weth;
uint256 public startTime; ===> uint256 public immutable startTime;