Olympus DAO contest - ReyAdmirado's results

1. typo in comments

numbe --> number

• PRICE.sol#L126

deactive --> deactivate

• Operator.sol#L295
• Operator.sol#L326

2. event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

• PRICE.sol#L26

• RANGE.sol#L20

• RANGE.sol#L21

• RANGE.sol#L22

• RANGE.sol#L24

• TRSRY.sol#L20

• TRSRY.sol#L27

• TRSRY.sol#L28

• TRSRY.sol#L29

• Kernel.sol#L203

• Governance.sol#L87

• Governance.sol#L89

• Operator.sol#L45

• Operator.sol#L52

• Operator.sol#L54

3. _safemint() should be used rather than _mint() wherever possible

• VOTES.sol#L36

4. constants should be defined rather than using magic numbers

Even assembly can benefit from using readable constants instead of hex/numeric literals

• PRICE.sol#L90

• PRICE.sol#L165

• RANGE.sol#L245-248

• RANGE.sol#L264

• Governance.sol#L164

• Governance.sol#L217

• Governance.sol#L268

• BondCallback.sol#L49

• BondCallback.sol#L71

5. lines are too long

Usually lines in source code are limited to 80 characters. Today’s screens are much larger so it’s reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length

• PRICE.sol#L31

• RANGE.sol#L40

• RANGE.sol#L45

• RANGE.sol#L61

• IOperator.sol#L15

• IOperator.sol#L90

• Operator.sol#L97

• Operator.sol#L675

6. event is missing old value

throughout the codebase, events are generally emitted when sensitive changes are made to the contracts. here emit is missing the old value.

• RANGE.sol#L256

• RANGE.sol#L267

• TRSRY.sol#L134

7. function should have emit

• Kernel.sol#L126

8. inconsistent use of named return variables

there is an inconsistent use of named return variables in the contract some functions return named variables, others return explicit values. consider adopting a consistent approach. this would improve both the explicitness and readability of the code, and it may also help reduce regressions during future code refactors.

most files have functions without named return but here they are not:

• Governance.sol#L61

• Governance.sol#L70

• Heart.sol#L69

• Heart.sol#L77

• Operator.sol#L154

• Operator.sol#L171

• Operator.sol#L276

9. open todos

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

• Operator.sol#L657

• TreasuryCustodian.sol#L51

• TreasuryCustodian.sol#L52

• TreasuryCustodian.sol#L56

1. <x> += <y> costs more gas than <x> = <x> + <y> for state variables (same with -= )

• PRICE.sol#L136

• PRICE.sol#L138

• PRICE.sol#L222

• TRSRY.sol#L96

• TRSRY.sol#L97

• TRSRY.sol#L115

• TRSRY.sol#L116

• TRSRY.sol#L131

• TRSRY.sol#L132

• VOTES.sol#L56

• VOTES.sol#L58

• Governance.sol#L198

• Governance.sol#L252

• Governance.sol#L254

• BondCallback.sol#L143

• BondCallback.sol#L144

• Heart.sol#L103

2. can make the variable outside the loop to save gas

• KernelUtils.sol#L44
• KernelUtils.sol#L59

3. ++i costs less gas than i++, especially when it’s used in for-loops (--i/i-- too)

Saves 6 gas per loop

• KernelUtils.sol#L49

• KernelUtils.sol#L64

• Operator.sol#L488

• Operator.sol#L670

• Operator.sol#L675

• Operator.sol#L686

• Operator.sol#L691

4. it costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied

• Kernel.sol#L397

• KernelUtils.sol#L43

• KernelUtils.sol#L58

5. using calldata instead of memory for read-only arguments in external functions saves gas

• PRICE.sol#L205

• Governance.sol#L159

• PriceConfig.sol#L45

• TreasuryCustodian.sol#L53

• BondCallback.sol#L152

6. using bools for storage incurs overhead

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past

• PRICE.sol#L62

• RANGE.sol#L127

• RANGE.sol#L184

• RANGE.sol#L216

• RANGE.sol#L281

• RANGE.sol#L291

• RANGE.sol#L302

• RANGE.sol#L320

• RANGE.sol#L330

• RANGE.sol#L340

• Kernel.sol#L113

• Kernel.sol#L126

• Kernel.sol#L181

• Kernel.sol#L194

• Kernel.sol#L197

• Kernel.sol#L394

• Governance.sol#L105

• Governance.sol#L117

• Governance.sol#L240

• BondCallback.sol#L24

• Heart.sol#L33

• Operator.sol#L63

• Operator.sol#L66

• Operator.sol#L363

• Operator.sol#L473

• Operator.sol#L618

• Operator.sol#L634

• Operator.sol#L699

• Operator.sol#L732

• Operator.sol#L735

• Operator.sol#L778

7. internal functions only called once can be inlined to save gas

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

• Kernel.sol#L266

• Kernel.sol#L279

• Kernel.sol#L295

• Kernel.sol#L325

• Kernel.sol#L351

• Kernel.sol#L378

• Kernel.sol#L409

• Heart.sol#L111

• Operator.sol#L652

8. usage of uint/int smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract’s 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 Use a larger size then downcast where needed

• PRICE.sol#L44

• PRICE.sol#L47

• PRICE.sol#L127

• PRICE.sol#L185

• PRICE.sol#L50

• PRICE.sol#L53

• PRICE.sol#L56

• PRICE.sol#L59

• PRICE.sol#L75

• PRICE.sol#L76

• PRICE.sol#L205

• PRICE.sol#L240

• PRICE.sol#L266

• PRICE.sol#L185

• PRICE.sol#L59

• PRICE.sol#L84

• PRICE.sol#L87

• PriceConfig.sol#L45

• PriceConfig.sol#L58

• PriceConfig.sol#L69

• Operator.sol#L89

• Operator.sol#L97

• Operator.sol#L516

• Operator.sol#L528

• Operator.sol#L529

• Operator.sol#L530

• Operator.sol#L548

• Operator.sol#L560

• Operator.sol#L561

• Operator.sol#L562

• Operator.sol#L665

• Operator.sol#L83

• Operator.sol#L86

• Operator.sol#L418

9. using private rather than public for constants, saves gas

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it’s used, and not adding another entry to the method ID table

• PRICE.sol#L59

• RANGE.sol#L65

• Governance.sol#L121

• Governance.sol#L124

• Governance.sol#L127

• Governance.sol#L130

• Governance.sol#L133

• Governance.sol#L137

• Operator.sol#L89

10. not using the named return variables when a function returns, wastes deployment gas

• INSTR.sol#L28

• MINTR.sol#L25

• PRICE.sol#L113

• Kernel.sol#L100

• TRSRY.sol#L51

• RANGE.sol#L115

• VOTES.sol#L27

• BondCallback.sol#L177

11. state variables only set in the constructor should be declared

avoids a gsset (20000 gas)

• Heart.sol#L48

• BondCallback.sol#L28

• BondCallback.sol#L32

12. <array>.length should not be looked up in every loop of a for-loop

This reduce gas cost as show here https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5

• Governance.sol#L278